-
Notifications
You must be signed in to change notification settings - Fork 179
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): Flex Stacker Module Support for EVT #17300
Conversation
module_core: legacy_module_core.LegacyModuleCore, | ||
load_name: str, | ||
quantity: int, | ||
label: str | None = None, |
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.
Let's have this (and the ones below) Optional[str]
for consistency
A PR has been opened to address analyses snapshot changes. Please review the changes here: #17304 |
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.
Looks solid, added some notes below on places for future improvements to allow us to consolidate this down to the labware state store instead of spread across the module substate. A true labware stack would take quite a bit of rearchitecting elsewhere, but might be worthwhile in the long run.
if ( | ||
self._is_loading_to_module( | ||
params.location, ModuleModel.FLEX_STACKER_MODULE_V1 | ||
) | ||
and not self._state_view.modules.get_flex_stacker_substate( | ||
params.location.moduleId | ||
).in_static_mode | ||
): |
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.
Looks good for EVT purposes, eventually we might want to de-coordinate these things if we're able to source the labware locations to a true stack in the tower.
if self._is_loading_to_module( | ||
params.location, ModuleModel.FLEX_STACKER_MODULE_V1 | ||
): | ||
state_update.load_flex_stacker_hopper_labware( | ||
module_id=params.location.moduleId, | ||
labware_id=loaded_labware.labware_id, | ||
) |
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.
Makes sense for this build.
new_labware_ids.append(lw_change.labware_id) | ||
elif isinstance(lw_change, FlexStackerRetrieveLabware): | ||
new_labware_ids.remove(lw_change.labware_id) | ||
elif isinstance(lw_change, FlexStackerStoreLabware): |
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'm cool with the way these lists work for this build. Eventually we probably want to transition to letting the labware state store manage this by having them be part of a location hierarchy/stack. This can come later though as theres more architecture that would need changing to allow that to work.
# loaded to column 3 but the addressable area is in column 4 | ||
assert deck_slot.value[-1] == "3" | ||
return f"flexStackerModuleV1{deck_slot.value[0]}4" |
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 fine for the time being, but eventually we'll want to rethink the architecture below this to allow the module to actually load into D4 instead of a fake D4.
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'm about halfway through and this all looks good so far, thanks. Just various small things.
|
||
@requires_version(2, 23) | ||
def store(self, labware: Labware) -> None: | ||
"""Store a labware at the bottom of the labware stack. | ||
|
||
:param labware: The labware object to store. | ||
""" | ||
assert labware._core is not None |
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.
Is this doing anything? It looks like labware._core
is statically typechecked to never be None
.
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 so that user can call this function:
stacker.store(plate)
In a refactor, we will add the labware id as a store command param so we can make sure the labware has already been loaded on the stacker slot.
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.
Right but I mean, is the assert
statement on line 1200 doing anything?
@@ -901,7 +920,11 @@ def load_module( | |||
) | |||
) | |||
if isinstance(deck_slot, StagingSlotName): | |||
raise ValueError("Cannot load a module onto a staging slot.") | |||
# flex stacker modules can only be loaded into staging slot inside a protocol |
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.
Is this the right layer to enforce this? We don't want it in the Protocol Engine loadModule
command?
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.
So we had to do this because the flex stacker is the very first module that provides an addressable area (deck column 4) in a different slot than its cutout mount (column 3).
Technically we are still loading the module to column 3 of the deck in the engine (because the engine expects only deck slots). But to users, they wouldn't care which deck cutout the module is being loaded into. It would be weird have user load the module into column 3 in the protocol when the observable addressable area is in column 4.
We did this here so that we can keep the same engine behavior for the flex stacker for fast development. In a future refactor, we should use addressable areas as the load location on the context level, instead of physical deck slots.
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.
OK, I'm only tenuously following, but I can catch up on this later. It doesn't seem like anything that should block this PR. Thanks for explaining!
api/src/opentrons/protocol_engine/commands/flex_stacker/configure.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/commands/flex_stacker/retrieve.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/commands/flex_stacker/retrieve.py
Outdated
Show resolved
Hide resolved
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.
Looks like a great first pass!
@@ -700,8 +700,17 @@ class FlexStackerCore(ModuleCore, AbstractFlexStackerCore): | |||
|
|||
_sync_module_hardware: SynchronousAdapter[hw_modules.FlexStacker] | |||
|
|||
def set_static_mode(self, static: bool) -> None: | |||
"""Set the Flex Stacker's static mode.""" |
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.
what's a static mode?
api/src/opentrons/protocol_engine/commands/flex_stacker/store.py
Outdated
Show resolved
Hide resolved
# labware loaded to the flex stacker hopper is considered offdeck. This is | ||
# a temporary solution until the hopper can be represented as non-addressable | ||
# addressable area in the deck configuration. |
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.
non-addressable addressable area in the deck configuration
Is that the direction we're headed? I thought we wanted something that's like OFF_DECK
but not OFF_DECK
?
This PR was requested on the PR #17300
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.
Thank you!
labware_core = self._protocol_core.get_labware_on_module(self._core) | ||
assert labware_core is not None, "Retrieve failed to return labware" |
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.
Aah, so any time this assert
would have raised, you're expecting the self._core.retrieve()
call a couple lines above to have raised, anyway?
Okay, that makes sense, and no, I don't think you need to change this assert
in that case. I would probably leave a comment explaining this, though, or maybe elaborate on it in the assert
message.
@@ -901,7 +920,11 @@ def load_module( | |||
) | |||
) | |||
if isinstance(deck_slot, StagingSlotName): | |||
raise ValueError("Cannot load a module onto a staging slot.") | |||
# flex stacker modules can only be loaded into staging slot inside a protocol |
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.
OK, I'm only tenuously following, but I can catch up on this later. It doesn't seem like anything that should block this PR. Thanks for explaining!
Overview
Covers EXEC-967, EXEC-965, EXEC-946, EXEC-1078
This PR introduces the .store(), .retrieve(), and .load_labware_to_hopper(...) commands. These commands allow the declaration of labware inside the hopper, retrieval of a handleable labware core from the stacker, and storage of labware into the stacker.
Test Plan and Hands on Testing
Changelog
Review requests
Outside of the known unintentional allowance of loading multiple stackers in the same slot (due to skipping the modules map on the deck conflict checker), this EVT version is meant to track labware through an internally managed labware list in the modules substate. Future versions may not reflect that practice.
Are there problems with the remaining approaches in this PR, whose purpose is to enable to use of the Flex stacker alongside all 12 base deck slots for nominal protocol operation?
Risk assessment
Low - Mostly introduction of new behavior with limited overlap to existing features. One existing risk is that in this EVT build multiple Flex Stackers can be loaded to the same slot due to the Stacker skipping the deck map conflict checker for the modules map.