feat(api): store labware objects in the stacker #18010

sfoster1 · 2025-04-08T16:58:45Z

We had this beautiful dream: the stacker wouldn't store labware. Instead, the stacker would know what kind of labware it should store, and how many of those labware it is storing, and objects would be created when you retrieved stuff. The problem with that is in the physical world there are labware objects in the stacker, and if we're not modeling them you can't know the state of those physical objects. So we have to reverse this decision and store the identity of the labware in the stacker.

This PR is the set of changes to accomplish that in the engine and account for it in the python API; the ts bindings; and at least somewhat in the app interface that already exists. It won't add any new API or app stuff.

The way that this works is:

The stacker contains a list of labware IDs that it contains
While a labware is in the stacker, its position in a stack (i.e. lid on labware, labware on adapter) is maintained but the stack as a whole is InStackerHopper(moduleId)
Labware must exist to go in to the stacker. That means that setStoredLabware and fill now handle making sure all the labware exists. They take lists of ids for PD compliance and also so that you can predefine labware if you want. They also still have a count argument. You can
- Say a number of labware with count, and the command will create that many labware with arbitrary IDs
- Not set count, and pass a list of IDs that don't currently exist, and the command will create labware with those IDs
- Not set count, and pass a list of IDs that do currently exist, and the command will move those labware in
  - Those labware must be in OFF_DECK or there's an error
Labware is moved out of the stacker, so retrieve doesn't create labware anymore
When you remove labware with empty, it's moved to OFF_DECK

fill, setStoredLabware, and empty all now specify lots of labware location sequences and ids in their results because they create or move labware where they previously do not. When fill and setStoredLabware create labware, they gloss those labware as having an original position descending to SYSTEM. Otherwise, they're the real original locations.

to come out of draft

Fix the tests that already exist
Write new tests to cover extra verification of labware state for the whole new-by-count/new-by-id/existing-id thing
Write new tests to make sure we do locations appropriately
Test on hardware
~~Make commit names that aren't so silly~~ that's what the squash is for

to merge

make a pr that implements this in the python api so it's testable

review requests

in addition to the standard stuff,

Do you feel like too much/too little/just enough is in stacker common
Is this an API the app can work with

codecov · 2025-04-10T21:28:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.85%. Comparing base (ff9e95b) to head (1ea73fb).
Report is 9 commits behind head on edge.

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #18010      +/-   ##
==========================================
- Coverage   61.87%   61.85%   -0.02%     
==========================================
  Files        2989     2996       +7     
  Lines      232727   234328    +1601     
  Branches    20112    20421     +309     
==========================================
+ Hits       143993   144949     +956     
- Misses      88557    89202     +645     
  Partials      177      177

Flag	Coverage Δ
protocol-designer	`19.17% <ø> (+0.31%)`	⬆️
step-generation	`4.37% <ø> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 55 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py

TamarZanzouri · 2025-04-11T15:17:41Z

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py

+    labware: LoadedLabware,
+) -> None:
+    definitions_by_id[labware.id] = definition
+    display_names_by_id[labware.id] = None


should this be None? we can get int from the definition?

This field is just for the user-specified display name, if any - the label argument to ProtocolContext.load_labware. At least that's how it is in the load labware engine command

CaseyBatten

Refactor and new features look really great overall, thanks Seth! Stacker common is a bit massive now, but I think it makes sense for the logic to live there as opposed to split between retrieve/unsafe retrieve and store as it was. Logistical question, do we want to wait on this until the PAPI PR is up and merge them into eachother first before merging this to edge?

The structure for Labware group and location sequencing logic, as well as the changes to Store and Retrieve to track the contained labware groups make sense. Some notes below on some things I wanted to verify and also where we might need to change the app end logic since retrieve isn't the "loader" for labware anymore.

CaseyBatten · 2025-04-11T15:02:44Z

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py

+    return group.lid.locationSequence
+
+
+def _check_one_preloaded_labware_known(


This all seems like solid validation, should be zero cases of mixed state labware groups anyways so nice!

CaseyBatten · 2025-04-11T15:14:27Z

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py

+        return False
+
+
+def _check_one_preloaded_labware(  # noqa: C901


Looks good, but just wanted to verify one thing regarding lids. We are validating that lids must be on a labware location, but not that solo lids must be off deck. Are we fully planning to not distribute lids/lid stacks out of the stacker? We can always make that change later, not blocking.

Oh that's sort of a nightmare. I think we should leave that to a followup. I guess we could implement it in one of the following ways:

in a stacker, the primary labware is a lid stack system object (0 height), each of which contains one lid. downside: can only retrieve one lid at a time. upside: follows our current system all the way through

in a stacker, lids can be primary labware, so the stacker contains many StackerStoredLabwareGroups with one lid as the primary. downside: can only retrieve one lid at a time, doesn't follow our current pattern. upside: no synthetic labware

we make a new kind of StackedStoredLabwareGroup or enhance the current one to allow for multiple lids in some way. downside: big change, upside: follows our current pattern and can retrieve more than one lid

(2) sounds the easiest to implement but with restrictions
(3) is extendable and will follow our retrieve guidelines

Definitely leaning approach 3, given users will want to move lids in "stacks". From a functional standpoint though, they'd have to move each lid off their stack to its destination one at a time, or rebuild the stack piecemeal elsewhere on the deck lest they leave their stacker unusable until all labware is consumed from the stack. The other option would be giving them some way of moving the whole stack at once (not a fan).

CaseyBatten · 2025-04-11T15:39:03Z

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py

+        .set_batch_loaded_labware(
+            definitions_by_id=definitions_by_id,
+            offset_ids_by_id=offset_ids_by_id,
+            display_names_by_id=display_names_by_id,
+            new_locations_by_id=new_locations_by_id,


This is fine as is, but last time I was modifying how we split out some of the batch loaded labware behavior I was wondering if it would be better to just send a set of LoadedLabware objects right down the line through state update all the way to the labware store. No need to gut it now, but these various dicts just do the "breaking up" work here instead of down at the handler in the end, so we could in theory save all this for that end step. I think the only reason I didn't at the time is that the new_location_by_id dict had to exist for location sequences so I left it all be.

yeah. we have a soft preference to have the data in state updates and results be native data types, but I think that's just people pattern matching - don't think there's a big reason.

CaseyBatten · 2025-04-11T16:00:59Z

api/src/opentrons/protocol_engine/commands/flex_stacker/fill.py

+                    self._state_view,
+                    [
+                        NotOnDeckLocationSequenceComponent(
+                            logicalLocationName=SYSTEM_LOCATION


Excellent, this aligns with the way in which we spawn new lid stack objects onto the deck during a move lid to empty deck slot command.

CaseyBatten · 2025-04-11T16:11:46Z

api/src/opentrons/protocol_engine/commands/flex_stacker/set_stored_labware.py

-                lid_def,
-            )
-            .update_flex_stacker_labware_pool_count(params.moduleId, count)
+        state_update, new_contained_labware = await build_or_assign_labware_to_hopper(


We will need to update the client and our starting deck state logic to be aware that this is our new "source" of a load labware type of action instead of retrieve.

TamarZanzouri · 2025-04-11T20:35:45Z

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py

+    ]
+
+
+def build_retrieve_labware_move_updates(


can you explain the method name?

build (retrieve labware) (move updates)

TamarZanzouri · 2025-04-11T20:43:10Z

api/src/opentrons/protocol_engine/commands/flex_stacker/set_stored_labware.py

-        ..., description="The number of labware loaded into the stacker labware pool."
+        ..., description="The number of labware now stored in the hopper."
+    )
+    originalPrimaryLabwareLocationSequences: (


I might be missing something but why do we need this prop in setStoredLabware command?

It lets clients tell the engine to build its own labware without specifying all the ids, which the PAPI will want to do.

sfoster1 · 2025-04-14T13:44:12Z

Logistical question, do we want to wait on this until the PAPI PR is up and merge them into eachother first before merging this to edge?

I think we want to wait on this until the PAPI PR is up so that we can actually test it, but I'd prefer to merge them independently so there isn't just a(n even more) giant lump of a commit in history for ever

TamarZanzouri · 2025-04-15T20:03:35Z

api/src/opentrons/protocol_engine/commands/flex_stacker/set_stored_labware.py

+                    group,
+                    self._state_view,
+                    [
+                        NotOnDeckLocationSequenceComponent(


TamarZanzouri · 2025-04-15T20:07:01Z

api/src/opentrons/protocol_engine/commands/flex_stacker/store.py

-            module_id=params.moduleId, count=stacker_state.pool_count + 1
+        state_update.update_flex_stacker_contained_labware(
+            module_id=params.moduleId,
+            contained_labware_bottom_first=(


beautiful ;-)

TamarZanzouri

a lot to read but its perfect. nice job! once ci passes of course :-)

TamarZanzouri · 2025-04-15T20:39:57Z

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py

+        return False
+
+
+def _check_one_preloaded_labware(  # noqa: C901


(2) sounds the easiest to implement but with restrictions
(3) is extendable and will follow our retrieve guidelines

ahiuchingau

Thank you for tackling this – it is a lot of work!!

All of the labware-handling logic makes sense to me. I can help test on hardware tomorrow if no one has gotten to it yet.

CaseyBatten

The test cases look good, the stacker common tests are very extensive and should give good resilience. Thank you for addressing my former comments as well, I especially think it makes sense to leave "independent lids" alone for now however I favor approach 3 from your suggestions.

A dream can still be beautiful, even if it is different!

CaseyBatten · 2025-04-16T14:32:49Z

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py

+        return False
+
+
+def _check_one_preloaded_labware(  # noqa: C901


Definitely leaning approach 3, given users will want to move lids in "stacks". From a functional standpoint though, they'd have to move each lid off their stack to its destination one at a time, or rebuild the stack piecemeal elsewhere on the deck lest they leave their stacker unusable until all labware is consumed from the stack. The other option would be giving them some way of moving the whole stack at once (not a fan).

sfoster1 force-pushed the exec-1394-restore-labware-ids-to-stacker-state branch 10 times, most recently from 3bac787 to d116172 Compare April 10, 2025 21:21

sfoster1 marked this pull request as ready for review April 10, 2025 21:22

sfoster1 requested a review from a team as a code owner April 10, 2025 21:22

sfoster1 requested review from vegano1, ahiuchingau and CaseyBatten April 10, 2025 21:22

TamarZanzouri reviewed Apr 11, 2025

View reviewed changes

api/src/opentrons/protocol_engine/commands/flex_stacker/common.py Show resolved Hide resolved

TamarZanzouri reviewed Apr 11, 2025

View reviewed changes

CaseyBatten reviewed Apr 11, 2025

View reviewed changes

TamarZanzouri reviewed Apr 11, 2025

View reviewed changes

update stacker stuff

31ca0ae

sfoster1 force-pushed the exec-1394-restore-labware-ids-to-stacker-state branch 3 times, most recently from 2801588 to ea1ac45 Compare April 14, 2025 21:42

sfoster1 requested a review from a team as a code owner April 14, 2025 21:42

sfoster1 force-pushed the exec-1394-restore-labware-ids-to-stacker-state branch 2 times, most recently from 6b24dbc to 5fc54ed Compare April 15, 2025 16:49

a couple slight changes

1ea73fb

sfoster1 force-pushed the exec-1394-restore-labware-ids-to-stacker-state branch from 5fc54ed to 1ea73fb Compare April 15, 2025 17:17

TamarZanzouri reviewed Apr 15, 2025

View reviewed changes

TamarZanzouri approved these changes Apr 15, 2025

View reviewed changes

ahiuchingau reviewed Apr 15, 2025

View reviewed changes

CaseyBatten approved these changes Apr 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): store labware objects in the stacker #18010

feat(api): store labware objects in the stacker #18010

sfoster1 commented Apr 8, 2025 •

edited

Loading

codecov bot commented Apr 10, 2025 •

edited

Loading

TamarZanzouri Apr 11, 2025

sfoster1 Apr 14, 2025

CaseyBatten left a comment

CaseyBatten Apr 11, 2025

CaseyBatten Apr 11, 2025

sfoster1 Apr 14, 2025

TamarZanzouri Apr 15, 2025

CaseyBatten Apr 16, 2025

CaseyBatten Apr 11, 2025

sfoster1 Apr 14, 2025

CaseyBatten Apr 11, 2025

CaseyBatten Apr 11, 2025

TamarZanzouri Apr 11, 2025

sfoster1 Apr 14, 2025

TamarZanzouri Apr 11, 2025

sfoster1 Apr 14, 2025

sfoster1 commented Apr 14, 2025

TamarZanzouri Apr 15, 2025

TamarZanzouri Apr 15, 2025

TamarZanzouri left a comment

TamarZanzouri Apr 15, 2025

ahiuchingau left a comment

CaseyBatten left a comment

CaseyBatten Apr 16, 2025

		return group.lid.locationSequence


		def _check_one_preloaded_labware_known(

feat(api): store labware objects in the stacker #18010

Are you sure you want to change the base?

feat(api): store labware objects in the stacker #18010

Conversation

sfoster1 commented Apr 8, 2025 • edited Loading

to come out of draft

to merge

review requests

codecov bot commented Apr 10, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyBatten 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

Choose a reason for hiding this comment

sfoster1 commented Apr 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamarZanzouri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahiuchingau left a comment

Choose a reason for hiding this comment

CaseyBatten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 commented Apr 8, 2025 •

edited

Loading

codecov bot commented Apr 10, 2025 •

edited

Loading