feat(api, shared-data): Expand Labware architecture to accommodate Lids #17072

CaseyBatten · 2024-12-10T16:48:17Z

Overview

Covers EXEC-1000, EXEC-1001, EXEC-1002, EXEC-1004

This PR seeks to remove Lids as a handled labware concept during a protocol, and treat them more as an attribute of a labware rather than a primary labware themselves.

TODO:

Add Unit Tests

Test Plan and Hands on Testing

(Assuming Max Supported Version is 2,22)

Existing TC Lid protocol PASSES analysis and executes without issue
load_lid_stack command causes analysis failure
load_labware with lid parameter causes analysis failure
(Assuming Max Supported Version bumped to 2,23)
Existing TC Lid protocol FAILS analysis
load_lid_stack command loads a stack of X lids at given location, passes analysis
load_labware command while utilizing the lid parameter results in a loaded labware and lid, passes analysis

Changelog

Expanded the Labware Schema to account for stackLimit, isDeckSlotCompatible, and compatibleParentLabware fields to allow for more accurate representation of lid behavior.
Expanded load_labware PAPI commands to accept optional lid field
Introduced load_lid_stack command to allow for the loading of lids in batches
Engine and state update expansion to account for changes in Lid Labware behavior
Addition of new Protocol engine "Lid Stack" Labware definition to create a handleable object to be returned to the user. In the follow up PR to introduce move lid we will be adding a method to "deallocate" these "Labwares" when they're "done"

Review requests

Do the command results we're returning make sense? Is there something more the client app will need?
We don't want to return a handleable labware PAPI context for lids to users ever, so I am not adding them to the core_map upon load, would this have any problematic knock on effects?

Risk assessment

Medium - we will be gating the behavior behind an API version so that users working with TC lids in existing protocols on existing API versions are unaffected, but future protocol drafting and labware behavior will be effected.

jbleon95

I still need to get through more of the protocol engine code, but I already have some comments and suggestions for change

jbleon95 · 2024-12-10T19:36:46Z

api/src/opentrons/protocol_api/protocol_context.py

+        :param str load_name: A string to use for looking up a lid definition.
+            You can find the ``load_name`` for any standard lid on the Opentrons
+            `Labware Library <https://labware.opentrons.com>`_.
+        :param location: ither a :ref:`deck slot <deck-slots>`,


Suggested change

:param location: ither a :ref:`deck slot <deck-slots>`,

:param location: Either a :ref:`deck slot <deck-slots>`,

jbleon95 · 2024-12-10T19:54:04Z

api/src/opentrons/protocol_api/protocol_context.py

+    def load_lid_stack(
+        self,
+        load_name: str,
+        location: Union[DeckLocation, Labware],


I feel like this load_lid_stack in ProtocolContext should only be responsible for loading on a DeckLocation, and there should be a separate load_lid_stack in the Labware context, which fits with our existing pattern of loading adapters and labware onto an existing labware

Hmm you're right that there should be one available for the labware context as well (to do things like adapter.load_lid_stack(load_name, quantity)), however in fitting with our current structure for load_labware, where we allow loading labware and it's adapter simultaneously, I would actually propose adding an optional adapter field to this new command so that we more closely mirror the setup of load_labware's current cross-context behavior.

The main difference there with the optional adapter field is it's also loading/creating the adapter along with the labware, not placing an existing adapter object on the labware. It's the reason why I have no issue with the optional lid field added to load_labware and the like. I'm not sure I'd like putting an optional labware name string here instead and loading that, than the lid stack on top of that, but it'd be closer to that pattern we've established

jbleon95 · 2024-12-10T19:56:13Z

api/src/opentrons/protocol_api/protocol_context.py

+        quantity: int,
+        namespace: Optional[str] = None,
+        version: Optional[int] = None,
+    ) -> Union[DeckLocation, Labware]:


Is there a reason why we are returning the location when it's already one of the arguments? This also is going against our pattern of returning the object being loaded, I'd rather just return nothing if we're not representing lids as Python object in the protocol.

This is one of the items currently in debate. Seth had an interesting proposal that the "Lid Stack" item should be a literal flat empty labware object that we return, which constitutes the "bottom" of the stack and is what will get handled when determining sources and destinations for lid stacks. The current approach is based on the idea that stacks are really just living dictionaries of "stacks" of lids around the deck, keyed by location. In theory both of these approaches get the same end result, however what the user handles in their protocol changes slightly between an Abstract "Labware" like object and a literal "Location" they'd refer to as a general alias for their stack. I'll be exploring what the former implementation idea would look like, though deallocating them or dealing with "empty" stacks might be a little awkward in places.

jbleon95 · 2024-12-10T19:57:12Z

api/src/opentrons/protocol_api/protocol_context.py

+        """
+        load_location: Union[DeckSlotName, StagingSlotName, Labware]
+        if isinstance(location, Labware):
+            load_location = location


if we are going to be passing a labware here (see my above comment for why I think we shouldn't), it should be a labware core

jbleon95 · 2024-12-10T20:05:04Z

api/src/opentrons/protocol_engine/commands/load_lid_stack.py

+            quantity=params.quantity,
+            labware_ids=None,
+        )
+        for i in range(params.quantity):


this whole loop is confusing to read I feel like this whole thing can be rewritten as a sequence of list comprehensions, which might be easier to comprehend (pun semi-intended). As it is it's a lot of index manipulation on very similarly named lists and sets in a single block

jbleon95 · 2024-12-10T20:10:47Z

api/src/opentrons/protocol_engine/execution/equipment.py

+            )
+
+        if labware_ids is None:
+            labware_ids = []


Easy list comprehension rewrite

jbleon95 · 2024-12-10T20:11:00Z

api/src/opentrons/protocol_engine/execution/equipment.py

+                version=version,
+            )
+
+        if definition.stackLimit is not None and quantity > definition.stackLimit:


Do we not have a default stack limit?

jbleon95 · 2024-12-10T20:17:01Z

api/src/opentrons/protocol_engine/execution/equipment.py

+
+        # Allow propagation of ModuleNotLoadedError.
+        load_labware_data_list = []
+        for i in range(quantity):


Again, I dislike the pattern of using for i in range(...) and special logic for a specific index number, since most of them can be rewritten as list comprehensions. This one for instance could easily be (after checking the initial location)

locations = [location] locations.extend([OnLabwareLocation(labwareId=labware_id) for labware_id in labware_Ids) load_labware_data_list = [LoadedLabwareData(....) for location in locations]

Something like this

jbleon95 · 2024-12-10T20:18:41Z

api/src/opentrons/protocol_engine/state/labware.py

+    def _add_loaded_lid_stack(self, state_update: update_types.StateUpdate) -> None:
+        loaded_lid_stack_update = state_update.loaded_lid_stack
+        if loaded_lid_stack_update != update_types.NO_CHANGE:
+            for i in range(len(loaded_lid_stack_update.labware_ids)):


Suggested change

for i in range(len(loaded_lid_stack_update.labware_ids)):

for labware_id in loaded_lid_stack_update.labware_ids:

CaseyBatten added 4 commits December 5, 2024 10:44

infrastructure for lid stacking and load labware modificaitons

adbfb05

labware load lid update

7a28027

labware and lid loading and update infrastructure

34d3d9f

lid loading api extensions to subsidiary load labware commands

5ba741f

CaseyBatten requested review from sfoster1, smb2268 and ahiuchingau December 10, 2024 16:48

CaseyBatten requested review from a team as code owners December 10, 2024 16:48

api version correction

aa3496a

CaseyBatten requested a review from vegano1 December 10, 2024 16:49

jbleon95 reviewed Dec 10, 2024

View reviewed changes

CaseyBatten added 4 commits December 11, 2024 18:10

cleanup and corrections and enforcement of API gating

da393b8

breakout lid loading behavior to individual command

77e5409

lid stack ghost labware addition and store cleanup

fad5d38

cleanup and api version correction

3f1402a

CaseyBatten requested a review from a team as a code owner December 13, 2024 20:07

CaseyBatten added 3 commits December 13, 2024 15:39

api version error cleanup

15f606c

Merge branch 'edge' into lid_store_architecture_EXEC_1004

04fb06e

unit tests

b7e2615

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api, shared-data): Expand Labware architecture to accommodate Lids #17072

feat(api, shared-data): Expand Labware architecture to accommodate Lids #17072

CaseyBatten commented Dec 10, 2024 •

edited

Loading

jbleon95 left a comment

jbleon95 Dec 10, 2024

jbleon95 Dec 10, 2024

CaseyBatten Dec 11, 2024

jbleon95 Dec 11, 2024

jbleon95 Dec 10, 2024

CaseyBatten Dec 11, 2024

jbleon95 Dec 10, 2024

jbleon95 Dec 10, 2024

jbleon95 Dec 10, 2024

jbleon95 Dec 10, 2024

jbleon95 Dec 10, 2024

jbleon95 Dec 10, 2024

	:param location: ither a :ref:`deck slot <deck-slots>`,
	:param location: Either a :ref:`deck slot <deck-slots>`,

	for i in range(len(loaded_lid_stack_update.labware_ids)):
	for labware_id in loaded_lid_stack_update.labware_ids:

feat(api, shared-data): Expand Labware architecture to accommodate Lids #17072

Are you sure you want to change the base?

feat(api, shared-data): Expand Labware architecture to accommodate Lids #17072

Conversation

CaseyBatten commented Dec 10, 2024 • edited Loading

Overview

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

jbleon95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyBatten commented Dec 10, 2024 •

edited

Loading