-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(api,robot-server,shared-data): Specify tip overlap by version #15323
feat(api,robot-server,shared-data): Specify tip overlap by version #15323
Conversation
Tip overlap dictionaries now have a version. Which version gets loaded will depend on what protocol execution frontends request from the engine with a notAfter constraint.
PE commands that load or alter pipette configuration now allow specifying a version of the pipette tip overlap dictionary to use. This specification is on a "no version above <x>" basis, for future proofing. It's allowed to not specify a version; in this case, the _highest_ available tip overlap version is used as a default. This is different from similar implementations. It is done so that users of protocol interfaces that do not require bug-for-bug compatibility as the python protocol API does get the positioning changes for free - those protocols do not have to change to get the new positioning. Consistent behavior here is an opt-in. This will be true of JSON protocols that use the protocol engine, v7 and v8. The python protocol API, however, has to stay stable. For this reason, - The hardware controller continues to export v0 of tip overlap unconditionally in its pipette_dict - This guarantees same behavior for pre-engine protocol APIs - This guarantees same behavior for old robot server interfaces (i.e. OT-2 calibration) - This guarantees same behavior for old JSON protocol interfaces, which we don't actually care about - The _engine_, on the other hand, actively loads the latest values if a version isn't specified, which will happen when ingesting v7 and v8 PD protocols - The _engine core_ currently hard specifies v0, and that will change based on the API level when we actually add a new version of the overlap data. This is implemented at pipette configuration time so that it can stay stateless - the pipette configuration data that is loaded in the engine continues to present only one dictionary of tip overlap data, which is resolved at the point of loading.
This is the OT-2 calibration stack. This is probably not the right way to do it, but we're never going to have multiple versions of the tip overlap dict for the OT-2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this approach looks great. Just some more minor code-level comments.
"type": "object", | ||
"description": "Map of tip overlap values with defaults", | ||
"required": ["default"], | ||
"$comment": "Other keys in here should be labware URIs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we ever considered YAML or JSON5 for shared-data?
tip_overlap = self._hw_pipette.tip_overlap.get(self._tip_rack.uri, 0) | ||
tip_overlap = self._hw_pipette.tip_overlap["v0"].get(self._tip_rack.uri, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want these OT-2 calibration flows to use the oldest data instead of the newest data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of a coinflip honestly - it would make it consistent with PD protocols and not older python protocols, or vice versa. Either would be unacceptable. We don't ever intend to update tip length data for OT-2 pipettes, but if we ever did we'd have to store it separately for different versions.
@@ -28,6 +28,13 @@ class ConfigureForVolumeParams(PipetteIdMixin): | |||
"than a pipette-specific maximum volume.", | |||
ge=0, | |||
) | |||
tipOverlapNotAfterVersion: Optional[str] = Field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a Protocol Engine caller's perspective, does it make sense for configureForVolume
to deal with tip overlaps? Doesn't this command only affect plunger stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, really, certainly not with what's currently implemented. It is a minor sin.
"tip overlap data that does not exceed this version will be used. Versions are " | ||
"expressed as vN where N is an integer, counting up from v0. If None, the current " | ||
"highest version will be used.", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nit that doesn't really matter, but since these are always integers, should we make this field an int
instead of a str
?
I guess we're using "vN"
strings for readability in the underlying JSON files? I'm 50/50 on whether it's better to use a str
here to be consistent with those, or to translate to an int
for API niceness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to keep it a string to promote "this is an arbitrary value please do not do math on it"
_TIP_OVERLAP_VERSION_RE = re.compile(r"^v\d+$") | ||
|
||
|
||
def validate_and_default_tip_overlap_version(version_spec: Optional[str]) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is a good idea, but we could make a custom Pydantic type for TipOverlapVersion
, or even VString
if we want to be generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd much rather have kind of gross validation be in code we write, honestly. Maybe if we had a second use case.
overlap: Dict[str, Dict[str, float]], version: str | ||
) -> Dict[str, float]: | ||
"""Get the latest tip overlap definitions that are equal or older than the version.""" | ||
# TODO: make this less awful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I don't know if this is a good idea, but if we had a custom TipOverlapVersion
/VString
Pydantic type, we could overload the comparison operators on it for easier sorting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but that would just wash the awfulness around. Ultimately, the only thing that would fix it is using just numbers and I want to keep them hard to do math with.
# Compatibility note - this is the version of tip overlap data, it stays at 0 | ||
# so protocol behavior does not change when you run a legacy JSON protocol | ||
instrument_load_info.pipette_dict, | ||
"v0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, the stuff in here only affects how the protocol activity gets reported to the frontend. It doesn't affect actual robot behavior. So this string could be arbitrary, though I agree v0 is most correct.
We need to have multiple versions of the tip overlap data for at least flex pipettes so that we can ship updated overlap data without inadvertently changing the behavior of protocols on stable API versions. This PR accomplishes that.
It's probably best viewed commit-by-commit:
Details
separate versioning
The data in shared-data has a version, but that version is separate from versions in other parts of the stack. This is on purpose, because the version has to be written in shared-data, and read in protocol engine. I think it's gross to write API versions in shared data, but we've done it before and could do it again; the real sticking point was the engine. The engine, in modern protocols, loads the tip overlap and does all that data management itself; it would need to select the appropriate version from the dict. We really don't want API versions there.
Therefore, there's a separate simple versioning scheme that's a little more like the alternate liquid handling function scheme that we haven't used in years, but with an actual string key for the version instead of implicit versioning by positioning in an array.
There will instead be a specific map of API versions to overlap versions living inside the source of the engine core for the python protocol API (that's the only place it needs to be - for why, read on).
engine state management and data lifecycle
There were basically two ways to implement this. The first way is to save all the data - save the full versioned tip overlap dictionary, and save which version we've selected. Since I listed this first, you can guess I didn't do this. Instead, what I did was add a version to the two engine commands that load pipette data -
load_pipette
andconfigure_for_volume
- and then have the functions those commands call pull the specified version out of the full config, which means that we only have to save the specific version in state and state doesn't have to change. This also means that we don't have to worry about data storage migration and compatibility.One huge note here: the engine argument parsing gives you the MOST RECENT tip overlap if you don't specify one. That's in bold because it's not the way we've implemented similar things. It's necessary because we want PD protocols to get the new overlap values without having to change anything.
python api integration
The python API, however, really wants to keep the old values (until you're on a certain version - we'll implement that when we actually add the new values). We do that in a couple ways:
json protocol integration
New JSON protocols, any that directly create engine commands rather than being mapped through python protocol API values, will get the new overlaps because they won't specify the parameter to the load pipette or configure for volume engine commands. Older JSON protocols that do adapt the python protocol API commands will get the python behavior.
Testing
Closes EXEC-451