Skip to content
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

Merged
merged 86 commits into from
Apr 15, 2024

Conversation

CaseyBatten
Copy link
Contributor

@CaseyBatten CaseyBatten commented Mar 18, 2024

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

  • During Protocol Setup on the ODD, the Modules and Deck section still separates out Modules and their Fixtures which leads to a confusing visual bug where modules are listed twice. This is non-blocking as the modules still need to be configured, calibrated, and connected. Once this is true you will be able to start the run. A following PR will implement the new designs that consolidate all deck hardware into a single table instead of splitting modules from fixtures.
  • the Deck map rendering shows white space beneath modules now, this is a visual only bug that will be fixed in a following PR
  • 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)
  • The Modules & Deck view on the ODD from the protocol setup view cannot be used to solve conflicts due to restrictions between the entanglement of module resolution and fixture resolution. Instead the APP should be used to resolve or the Deck Configuration view from the ODD main page.

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.


requirements = {
    "robotType": "Flex",
    "apiLevel": "2.17",
}


def run(protocol_context):
	tc_mod = protocol_context.load_module(module_name="thermocyclerModuleV2")
	tc_plate = protocol_context.load_labware("nest_96_wellplate_100ul_pcr_full_skirt", "C1")

	temp_mod = protocol_context.load_module(module_name="temperatureModuleV2", location="D3")
	temp_mod2 = protocol_context.load_module(module_name="temperatureModuleV2", location="D1")
	temp_adapter = temp_mod.load_adapter("opentrons_96_well_aluminum_block")
	temp_plate = protocol_context.load_labware("nest_96_wellplate_100ul_pcr_full_skirt", "D2")
	temp_adapter2 = temp_mod2.load_adapter("opentrons_96_well_aluminum_block")
	temp_plate2 = protocol_context.load_labware("nest_96_wellplate_100ul_pcr_full_skirt", "B2")
	magnetic_block = protocol_context.load_module(module_name="magneticBlockV1", location="C2")
	mag_plate = protocol_context.load_labware("nest_96_wellplate_100ul_pcr_full_skirt", "B3")

	tc_mod.open_lid()

	tiprack1 = protocol_context.load_labware("opentrons_flex_96_tiprack_1000ul", "C3")

	# move all labware into place on modules
	protocol_context.move_labware(tc_plate, tc_mod, use_gripper=True)
	protocol_context.move_labware(temp_plate, temp_adapter, use_gripper=True)
	protocol_context.move_labware(temp_plate2, temp_adapter2, use_gripper=True)
	protocol_context.move_labware(mag_plate, magnetic_block, use_gripper=True)

	pipette = protocol_context.load_instrument(
		"flex_1channel_1000", mount="left", tip_racks=[tiprack1]
	)
	pipette.pick_up_tip()

	pipette.aspirate(volume=10, location=tc_plate.wells()[0])
	pipette.aspirate(volume=10, location=temp_plate.wells()[0])
	pipette.aspirate(volume=10, location=temp_plate2.wells()[0])
	pipette.aspirate(volume=10, location=mag_plate.wells()[0])

	trash = protocol_context.load_trash_bin("A3")
	pipette.drop_tip()

Changelog

  • Updates /deck_configuration endpoint with optional opentronsModuleSerialNumber field
  • Update deck configuration JSON file for persistent storage
  • Introduces Deck Definition Schema V5 adding fixture definitions for modules
  • Validates that a given module exists within the deck configuration when it is loaded in a python protocol
  • Removes compatibleModuleTypes from OT3 deck definitions, maintains them for OT2 deck config support
  • Serial number from deck configuration now utilized in validation of modules during load/calibration solving support for multiples of a module across all networked modules
  • Introduces Fixture Groups to deck schema to allow a "fixture" that covers multiple cutouts to be broken up into individual per-cutout sections (each section a cutout fixture) and and those resulting sections be dependently grouped together into a map for load validation
  • App changes to support new fixtures defs and fixture grouping
  • App changes to module configuration during protocol setup to allow modules to be added to the deck configuration
  • App changes for submission of module serial to deck configuration upon confirmation

Review 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.

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.20%. Comparing base (8f50b08) to head (2c8fe4a).
Report is 38 commits behind head on edge.

❗ Current head 2c8fe4a differs from pull request most recent head 0c1759b. Consider uploading reports for the commit 0c1759b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)

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

Files Coverage Δ
api/src/opentrons/calibration_storage/types.py 100.00% <ø> (ø)
...pi/src/opentrons/hardware_control/modules/types.py 98.41% <ø> (ø)
...c/opentrons/protocol_engine/clients/sync_client.py 100.00% <ø> (ø)
.../opentrons/protocol_engine/commands/load_module.py 100.00% <ø> (ø)
...c/opentrons/protocol_engine/execution/equipment.py 100.00% <ø> (ø)
...ns/protocol_engine/resources/deck_data_provider.py 100.00% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/geometry.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/labware.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/modules.py 91.57% <ø> (+0.31%) ⬆️
api/src/opentrons/protocol_engine/state/state.py 100.00% <ø> (ø)
... and 23 more

... and 193 files with indirect coverage changes

@@ -288,6 +288,7 @@ def load_module(
model: ModuleModel,
deck_slot: Optional[DeckSlotName],
configuration: Optional[str],
addressable_area: Optional[str],
Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. PAPI load_module command begins the load process and supplies a deck location.
  2. 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(...).
  3. Engine core load_module is passed the identified addressable_area value (Str | None)
  4. 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# 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:
Copy link
Contributor

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",
Copy link
Contributor

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

@CaseyBatten CaseyBatten requested review from a team as code owners April 11, 2024 19:57
@CaseyBatten CaseyBatten requested review from smb2268 and removed request for a team April 11, 2024 19:57
@CaseyBatten CaseyBatten changed the title feat(api, shared-data, robot-server): Add module fixtures to deck configuration feat(app, api, shared-data, robot-server): Add module fixtures to deck configuration Apr 11, 2024
Copy link
Contributor

@brenthagen brenthagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very solid on the js side. there are some edge cases to fix but should be safe to merge

Screen Shot 2024-04-12 at 4 22 32 PM
Screen Shot 2024-04-15 at 10 33 02 AM

DeckDefinition,
} from '@opentrons/shared-data'

// TODO(BC, 2024-03-21): This component is almost identical to TemperatureModuleFixture, consider consolidating?
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

@vegano1 vegano1 left a 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.

  1. 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.
  2. 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.

Copy link
Collaborator

@y3rsh y3rsh left a 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.

@CaseyBatten CaseyBatten merged commit 4e895d4 into edge Apr 15, 2024
77 checks passed
@CaseyBatten CaseyBatten deleted the add_modules_to_deck_config branch April 15, 2024 21:14
DerekMaggio added a commit that referenced this pull request May 1, 2024
…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
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
…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]>
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants