refactor(api): Express labware offset locations as sequences #17363
Conversation
Make the current labware offset location type a scary legacy type, since we'll be improving it shortly.
This doesn't actually require a valid deck configuration so let's push it into static helpers so we can use it without one.
When we load and express labware offsets, we previously used an object that had explicit named members for a labware parent; a module parent; and a deck slot. This worked as long as we don't have heterogenous labware stacking (to include labware-on-labware-on-adapter) but it's inflexible, and maybe we do want that. Instead, let's have a sequence of little models that specify a parent geometry - a labware def uri, a module model, or an addressable area name - and use that both for specifying offset locations via the HTTP API, for comparing locations internally, and for downloading locations from the HTTP API. Importantly, these offset locations are currently in the database as parts of runs, so we have to keep the old version around when expressing labware data; this is annoying, but that's life. We can calculate the equivalent old data when we load a labware offset pretty easily, and this also lets us not change the legacy protocol core. We also are going to keep the old method of specifying them around, and convert them into the new format, which is also pretty easy, to preserve backwards compatibility and roundtripping older offsets from before this data was present.
Accept either old or new style offsets and adapt the internal APIs to provide the correct type of thing to the protocol engine internals. Note that the new data will essentially be lost when the data goes to the labware offsets API because that API does not currently support specifying (or returning) the location vector; that's fine in this PR because all the APIs still accept the old kind of offset. In a follow-up PR, we'll add support for the new-style locations to the offset api.
Add the new format to types and rename the old usages.
sfoster1
added
the
gen-analyses-snapshot-pr
| return sequence | ||
|
|
||
|
|
||
| def _offset_location_sequence_head_to_labware_and_module( |
There was a problem hiding this comment.
sorry but I don't understand the method name
There was a problem hiding this comment.
head is "head of the list", so it turns the first (well, all but the last) elements of the list into the labware and module parts of the offset - the addressable area is guaranteed to be the last element of the list.
|
|
||
|
|
||
| def _offset_location_sequence_head_to_labware_and_module( | ||
| location_sequence: LabwareOffsetLocationSequence, deck_definition: DeckDefinitionV5 |
There was a problem hiding this comment.
do we need deck_definition arg?
There was a problem hiding this comment.
Huh, doesn't look like it. I wonder why lint didn't care
| @@ -1304,7 +1309,7 @@ def ensure_and_convert_module_fixture_location( | |||
| """ | |||
| deck_type = self._state.deck_type | |||
|
|
|||
| if deck_type == DeckType.OT2_STANDARD or deck_type == DeckType.OT2_SHORT_TRASH: | |||
| if not self.get_deck_supports_module_fixtures(): | |||
| @@ -87,7 +89,7 @@ async def create( | |||
| self, | |||
| run_id: str, | |||
| created_at: datetime, | |||
| labware_offsets: List[LabwareOffsetCreate], | |||
| labware_offsets: Sequence[LabwareOffsetCreate | LegacyLabwareOffsetCreate], | |||
There was a problem hiding this comment.
not that it really matters but why was this changed to a Sequence?
There was a problem hiding this comment.
List is invariant, so if this was a list and you called it with a list of only LabwareOffsetCreate (or LegacyLabwareOffsetCreate) mypy would give you an error because the argument would need to be exactly of the union type. Because Sequence is covariant, calling it with a list of only LabwareOffsetCreate or LegacyLabwareOffsetCreate will pass mypy and be properly widened into a union.
We will not! This data isn't exposed in the API, and even if it was, we fully support both ingesting and outputting the old style of data. |
This is a pattern that we're going to adopt for symbolic geometry locations in HTTP API contexts - expressing them as a sequence of heterogenous unions that represent kinds of places things can be. We'll do this for locations of labware instances in a followup, but for now we're doing it for offset locations.
Offset locations were previously objects that had a slot name; an optional module model, for a location on a module; and an optional labware URI, for a location on a labware (possibly an adapter, possibly on a model). This can't represent labware stacks, and it's locked to slot names. These are more or less fine for now still, but it would be nice to support stacks in the general case, and it will be awkward for clients to use the object-with-optional-attributes model alongside the sequence-of-locations instance model. Plus, while this won't need a database schema change for the offsets stored in runs, it will for the offsets api - so let's make this change now while that schema and API haven't yet shipped.
Testing
Since the HTTP API still is the same and everything is tested internally, it is sufficient to do normal LPC testing - everything should still work.
Reviews