-
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(app, api, shared-data, robot-server): Add module fixtures to deck configuration #14684
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14684 +/- ##
==========================================
- Coverage 67.50% 67.20% -0.30%
==========================================
Files 2521 2495 -26
Lines 72090 71463 -627
Branches 9311 8987 -324
==========================================
- Hits 48666 48029 -637
- Misses 21228 21321 +93
+ Partials 2196 2113 -83
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -288,6 +288,7 @@ def load_module( | |||
model: ModuleModel, | |||
deck_slot: Optional[DeckSlotName], | |||
configuration: Optional[str], | |||
addressable_area: Optional[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.
this seems extraneous right? we aren't really loading a module into an addressable area. It's more that we're loading it into a cutout right?
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.
Could be moved elsewhere potentially. Right now the logic flow is:
- PAPI load_module command begins the load process and supplies a deck location.
- Module fixture location is converted to a valid addressable area for that Module, if one exists (could be None, like for OT-2). Up until this point we do the same thing for fixtures commands like load_trash_bin(...).
- Engine core load_module is passed the identified addressable_area value (Str | None)
- Legacy load_module engine method doesn't use this value, current load_module engine method will supply the engine client load_module method with the validated addressable area if it is not None
I could probably collapse this down to occur inside the engine-side load_module logic instead if we want.
""" | ||
|
||
if robot_type == "OT-2 Standard": | ||
# OT-2 Utilizes the existing compatibleModulelTypes list of traditional addressable areas |
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.
# OT-2 Utilizes the existing compatibleModulelTypes list of traditional addressable areas | |
# OT-2 Utilizes the existing compatibleModuleTypes list of traditional addressable areas |
@@ -64,6 +65,22 @@ def from_model(cls, model: ModuleModel) -> ModuleType: | |||
if isinstance(model, MagneticBlockModel): | |||
return cls.MAGNETIC_BLOCK | |||
|
|||
@classmethod | |||
def to_module_fixture_id(cls, module_type: ModuleType) -> 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.
looks like these cutout fixture ids still need the model versions added in
"height": 40 | ||
}, | ||
{ | ||
"id": "magneticBlockModuleV1", |
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.
for consistency, maybe we either rename this cutout fixture id to magneticBlockV1
or rename it's addressable Areas to magneticBlockModuleV1XY
? Slight preference for the former as we don't really call it the "magnetic block module" elsewhere
…ntrons into add_modules_to_deck_config
…rons/opentrons into add_modules_to_deck_config
…ntrons into add_modules_to_deck_config
…ntrons into add_modules_to_deck_config
…rons/opentrons into add_modules_to_deck_config
…ntrons into add_modules_to_deck_config
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.
DeckDefinition, | ||
} from '@opentrons/shared-data' | ||
|
||
// TODO(BC, 2024-03-21): This component is almost identical to TemperatureModuleFixture, consider consolidating? |
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.
there's a possible shared abstraction between all of these "config" fixtures but in thinking it over for deck config the benefit seems limited. a shared abstraction could be useful for quick transfer though
@@ -107,10 +91,7 @@ export function useDeckConfigurationCompatibility( | |||
cutoutFixtureId, | |||
requiredAddressableAreas: requiredAddressableAreasForCutoutId, | |||
// Thermocycler requires an "empty" (single slot) fixture in A1 that is not referenced directly in 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.
should delete this comment along with the rest of the excellent deletion in this file
@@ -60,6 +60,9 @@ export const PlaceAdapter = (props: PlaceAdapterProps): JSX.Element | null => { | |||
} = props | |||
const { t } = useTranslation('module_wizard_flows') | |||
const mount = attachedPipette.mount | |||
const slotName = deckConfig.find(cc => ( | |||
cc.opentronsModuleSerialNumber === attachedModule.serialNumber | |||
))?.cutoutId?.replace('cutout', '') ?? null |
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.
there's a mapping FLEX_SINGLE_SLOT_BY_CUTOUT_ID
in shared-data
…rons/opentrons into add_modules_to_deck_config
…ntrons into add_modules_to_deck_config
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.
The robot-server and protocol engine changes look good to me, we also spoke about some potential optimizations we could make in the future.
- The definition for each cutout might not be required for fixtures that have the same offsets/dimensions, ex "heaterShakerA1”, “heaterShakerA3”,…,”heaterShakerD1”. We might be able to have one definition that is referenced in software for multiple cutouts.
- OT-2 still uses the "legacy" deck definition, it would be good to eventually port over this deck config work to encompass the OT-2 and remove the 2 logic paths we currently have.
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.
Please review the snapshot changes that this branch causes.
These are reviewable in #14909
Do not merge it in, only use it as another point of data.
Ping me if you have questions.
…ons (#14962) # Overview With the large code change in #14684 we want to test it as thoroughly as possible. This PR adds generation of test cases with [hypothesis](https://hypothesis.readthedocs.io/en/latest/index.html) for deck configuration The idea is that hypothesis will generate DeckConfiguration objects that represent what a user defines in their protocol or their deck configuration in the UI. These objects will then end up being used to auto-generate Python protocols to pipe through analysis to exercise our deck configuration validation logic # Test Plan - I went through some of the generated deck configurations to verify they were being created correctly # Changelog - Create datashapes.py - This defines a simplified deck configuration model and all of its contents - Create helper_strategies.py - This file provides the building block strategies that are utilized to make a final strategy that makes a DeckConfiguration object - Create final_strategies.py - This contains the logic for generating the final DeckConfiguration objects # Review requests - Should I add some tests to confirm that DeckConfiguration objects are being generated as expected? # Risk assessment None.... yet
…k configuration (#14684) Build out the backend and frontend to support loading modules into the deck configuration, including tracking modules by serial number in the persistent deck configuration directory. Closes RESC-209, PLAT-247, PLAT-248, PLAT-249, PLAT-250, PLAT-251, PLAT-252, PLAT-254 --------- Co-authored-by: Brian Cooper <[email protected]> Co-authored-by: ahiuchingau <[email protected]>
…ons (#14962) # Overview With the large code change in #14684 we want to test it as thoroughly as possible. This PR adds generation of test cases with [hypothesis](https://hypothesis.readthedocs.io/en/latest/index.html) for deck configuration The idea is that hypothesis will generate DeckConfiguration objects that represent what a user defines in their protocol or their deck configuration in the UI. These objects will then end up being used to auto-generate Python protocols to pipe through analysis to exercise our deck configuration validation logic # Test Plan - I went through some of the generated deck configurations to verify they were being created correctly # Changelog - Create datashapes.py - This defines a simplified deck configuration model and all of its contents - Create helper_strategies.py - This file provides the building block strategies that are utilized to make a final strategy that makes a DeckConfiguration object - Create final_strategies.py - This contains the logic for generating the final DeckConfiguration objects # Review requests - Should I add some tests to confirm that DeckConfiguration objects are being generated as expected? # Risk assessment None.... yet
Overview
Covers RESC-209, PLAT-247, PLAT-248, PLAT-249 and PLAT-250
App side coverage also includes PLAT-251, PLAT-252 and PLAT-254
Build out the backend and frontend to support loading modules into the deck configuration, including tracking modules by serial number in the persistent deck configuration directory.
Known Issues to be addressed in follow-up
the Deck Configurator that is accessible from the ODD in the three dot menu of the robot dashboard, doesn't currently support adding and removing modules. Until this is added in a following PR, configure modules from the desktop app.(fixed)Test Plan
The following Flex protocol should execute without issue, allowing the user to resolve all deck conflicts and add all modules in order to run the protocol.
Changelog
opentronsModuleSerialNumber
fieldReview requests
Risk assessment
The addition of modules to the deck configuration has changed a signification amount of backend infrastructure regarding the method in which items are validated in the deck and the way in which deck configurations are validated as well. The front end flow for protocol setup regarding the deck, module installation and calibration, and protocol run has also been greatly modified. Thorough testing should be executed on the sustained behavior and usability of all modules and protocol types across OT2 and Flex.