fix(api): enable deck conflict checker for the flex #13189

TamarZanzouri · 2023-07-28T15:20:18Z

Overview

closes https://opentrons.atlassian.net/browse/RSS-294 -> enable deck_conflict checker for the flex..
closes https://opentrons.atlassian.net/browse/RSS-273 -> fixes bug when loading a thermocycler in slots that are not allowed.
closes https://opentrons.atlassian.net/browse/RQA-993 -> does not allow loading mag deck's on an OT-3. just mag blocks.

Test Plan

Allow an OT-3 protocol with modules as described here. make sure no exception is being raise.

from opentrons import protocol_api, types

metadata = {
    "apiLevel": "2.15",
}

requirements = {
    "robotType": "OT-3"
}

def run(ctx: protocol_api.ProtocolContext) -> None:
    tip_rack_50 = ctx.load_labware("opentrons_ot3_96_tiprack_50ul", 4)
    tip_rack_200 = ctx.load_labware("opentrons_flex_96_tiprack_200ul", 5)
    tip_rack_1000 = ctx.load_labware("opentrons_ot3_96_tiprack_1000ul", 6)
    heatershaker = ctx.load_module('heaterShakerModuleV1',1)
    heatershaker = ctx.load_module('thermocyclerModuleV2')
    mag_block = ctx.load_module('magneticBlockV1',2)
    temp = ctx.load_module('temperatureModuleV2', 3)

Try loading a module on the flex in a middle slot. make sure you are getting an exception.

from opentrons import protocol_api, types

metadata = {
    "apiLevel": "2.15",
}

requirements = {
    "robotType": "OT-3"
}

def run(ctx: protocol_api.ProtocolContext) -> None:
    # this should fail bc h/s is not allowed in the middle.
    mag_block = ctx.load_module('heaterShakerModuleV1',2)

Try loading 2 modules on the flex in same slot slot. make sure you are getting an exception.

from opentrons import protocol_api, types

metadata = {
    "apiLevel": "2.15",
}

requirements = {
    "robotType": "OT-3"
}

def run(ctx: protocol_api.ProtocolContext) -> None:
    tip_rack_50 = ctx.load_labware("opentrons_ot3_96_tiprack_50ul", 4)
    tip_rack_200 = ctx.load_labware("opentrons_flex_96_tiprack_200ul", 5)
    tip_rack_1000 = ctx.load_labware("opentrons_ot3_96_tiprack_1000ul", 2)
    heatershaker = ctx.load_module('heaterShakerModuleV1',1)
    # this should fail bc h/s is already loaded in slot 1
    mag_block = ctx.load_module('magneticBlockV1',1)

Should fail bc mag decks are not allowed on the flex.

from opentrons import protocol_api, types

metadata = {
    "apiLevel": "2.15",
}

requirements = {
    "robotType": "OT-3"
}

def run(ctx: protocol_api.ProtocolContext) -> None:
    # this should fail bc h/s is not allowed in the middle.
    mag_deck = ctx.load_module('magneticModuleV2',1)

Should fail bc thermocyclers are not allowed in slot 1.

from opentrons import protocol_api, types

metadata = {
    "apiLevel": "2.15",
}

requirements = {
    "robotType": "OT-3"
}

def run(ctx: protocol_api.ProtocolContext) -> None:
    # this should fail bc h/s is not allowed in the middle.
    thermo = ctx.load_module('thermocyclerModuleV2',1)

Smoke test OT-2 protocols and make sure it all works as expected.
I have also created an integration test for that
We should test this with the app and a mag block in a protocol to make sure that it appears properly in the app

Changelog

Change deck_conflict.check to get DeckSlotName for the new location and and existing location instead of int.
Added robot_type arg inorder to know what restrictions to load.
Changed all access with method to DeckSlotName instead of int.
Added integration test for a failed protocol
Updated all tests to use DeckSlotName instead of int/str
Make sure you are able to load the thermocycler in allowed slots.
Added slot validation for load_module in protocol core

Review requests

Changes make sense? Did I forget anything?

Risk assessment

Medium. need to smoke test OT-2 protocols both json on python for all version (We care about) to make sure loading labware/modules on deck has been affected. need to test OT-3 protocols and to make sure load_module is raising an exception for the restrictions in the ticket.

codecov · 2023-07-28T15:32:06Z

Codecov Report

Merging #13189 (759061c) into edge (8d6e391) will increase coverage by 0.23%.
Report is 60 commits behind head on edge.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge   #13189      +/-   ##
==========================================
+ Coverage   72.18%   72.42%   +0.23%     
==========================================
  Files        1567     2393     +826     
  Lines       51763    66292   +14529     
  Branches     3268     7364    +4096     
==========================================
+ Hits        37366    48010   +10644     
- Misses      13881    16515    +2634     
- Partials      516     1767    +1251

Flag	Coverage Δ
app	`71.32% <ø> (+27.38%)`	⬆️
g-code-testing	`96.44% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`
protocol-designer	`47.13% <ø> (-0.16%)`	⬇️
shared-data	`75.11% <ø> (-3.06%)`	⬇️
step-generation	`87.18% <ø> (+<0.01%)`	⬆️

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

see 874 files with indirect coverage changes

TamarZanzouri · 2023-07-31T17:36:46Z

api/src/opentrons/motion_planning/deck_conflict.py

@@ -184,10 +191,13 @@ def check(
        DeckConflictError: Adding this item should not be allowed.
    """
    restrictions: List[_DeckRestriction] = [_FixedTrashOnly()]
-
+    if robot_type == "OT-2 Standard":


Laura-Danielle

Initial comments, but overall looks pretty good to me. Agree w/ Max that you should not be using Unions here though.

Laura-Danielle · 2023-07-31T18:29:25Z

api/src/opentrons/motion_planning/deck_conflict.py

+                )
+            )
+
+    elif isinstance(item, (HeaterShakerModule, TemperatureModule)):


Are these restrictions still valid? The spacing between slots is very different on the flex.

Confirmed with Chris that this is still mandatory. also PD still has restrictions.

Laura-Danielle · 2023-07-31T18:31:32Z

api/src/opentrons/motion_planning/deck_conflict.py

    new_item: DeckItem,
-    new_location: int,
+    new_location: Union[int, str],
+    robot_type: str = "OT-2 Standard",


I think this is totally reasonable to do, but if you really hate it you could create a enum and/or bitfield in which you can specify what items you want to check for collisions and push up the robot type check higher.

Laura-Danielle · 2023-07-31T18:34:58Z

api/src/opentrons/motion_planning/deck_conflict.py

+    restrictions: List[_DeckRestriction] = []
+
+    if isinstance(item, ThermocyclerModule):
+        for covered_location in _flex_slots_covered_by_thermocycler():


I thought the TC could be in multiple slots, is this not the case?

its no slot 7,10. I am setting both slots as occupied if we are in the TC case

api/src/opentrons/motion_planning/deck_conflict.py

…ition in engine core

TamarZanzouri · 2023-08-03T20:36:54Z

api/src/opentrons/protocol_api/core/engine/protocol.py

+            if module_type == ModuleType.THERMOCYCLER:
+                deck_slot = (
+                    DeckSlotName.SLOT_7
+                    if self._engine_client.state.config.robot_type == "OT-2 Standard"


can also use deck robot property but this feels the same. comparing a string.

TamarZanzouri · 2023-08-03T21:01:41Z

shared-data/deck/schemas/3.json

@@ -167,7 +167,8 @@
                    "magneticModuleType",
                    "temperatureModuleType",
                    "thermocyclerModuleType",
-                    "heaterShakerModuleType"
+                    "heaterShakerModuleType",
+                    "magneticBlockType"


what do we think about this?

I'm OK with it. We treat it as a module essentially anyways.

Laura-Danielle

Looks great. Have some small comments that you can ignore.

Laura-Danielle · 2023-08-04T14:53:42Z

api/src/opentrons/motion_planning/deck_conflict.py

                    source_item=item,
                    source_location=location,
                )
            )

-        for covered_location in get_east_west_slots(location):
+        for hs_covered_location in get_east_west_slots(location.as_int()):


Should these functions also take in DeckSlotNames and perform the check on that type?

I tired doing it but its tied to much to the legacy implementation and we are already parsing this as int in other places so might as well leave it

api/src/opentrons/protocol_api/core/engine/protocol.py

Laura-Danielle · 2023-08-04T15:07:00Z

shared-data/deck/schemas/3.json

@@ -167,7 +167,8 @@
                    "magneticModuleType",
                    "temperatureModuleType",
                    "thermocyclerModuleType",
-                    "heaterShakerModuleType"
+                    "heaterShakerModuleType",
+                    "magneticBlockType"


I'm OK with it. We treat it as a module essentially anyways.

SyntaxColoring

This looks great! One question about the fixed trash, and a few other minor suggestions.

robot-server/tests/integration/protocols/deck_coordinate_load_fail.py

robot-server/tests/integration/http_api/protocols/test_deck_coordinate_load_fail.tavern.yaml

api/src/opentrons/motion_planning/deck_conflict.py

api/src/opentrons/protocol_api/core/engine/protocol.py

…fail.py Co-authored-by: Max Marrone <[email protected]>

…ordinate_load_fail.tavern.yaml Co-authored-by: Max Marrone <[email protected]>

api/src/opentrons/motion_planning/deck_conflict.py

SyntaxColoring

Code looks good to me. Thanks!

SyntaxColoring · 2023-08-07T16:04:14Z

api/src/opentrons/motion_planning/deck_conflict.py

 ) -> None:
    """Check a deck layout for conflicts.

    Args:
        existing_items: Existing items on the deck, assumed to be valid.
        new_item: New item to add to the deck.
        new_location: Location where the new item will be added.
+        robot_type: Robot deck definition.


This is a nitpick and I think we should merge this now anyway, but for future reference, the RobotType is not exactly the deck definition.

The OT-2 has one RobotType ("OT-2 Standard"), but two possible deck definitions (ot2_standard and ot2_short_trash). The names are very confusing.

TamarZanzouri · 2023-08-07T17:28:12Z

Tested ot OT-3:
loading modules where they are allowed
loading modules where they are not allowed and getting the proper error.
loading mag decks will raise an error.

Smoke tested OT-2 as well. Works as expected!

SyntaxColoring

Ah, yep. Fix makes sense to me. Nice catch.

sanni-t

LGTM! 🙌

api/src/opentrons/motion_planning/deck_conflict.py

TamarZanzouri added 5 commits July 26, 2023 16:57

WIP started check for deck conflict in ot3

45d3338

split _create_restrictions

8fbfd93

flex restrictions

7c77ebd

Temp deck deck item and fixed tests for engine deck conflict

f0cd5b5

added tests for raising when module is not allowed

b9d77e4

TamarZanzouri added 6 commits July 28, 2023 14:32

fixed bug with left and right sides of loading modules

a97512a

failed deck conflict e2e test

815a764

removed comment

b4aaa04

WIP - use str/int as slots

f3a8400

wip fixing tests

7e643f8

fixed logic and tests

17c6ed0

TamarZanzouri commented Jul 31, 2023

View reviewed changes

fixed tavern test

b4fa2d0

Laura-Danielle reviewed Jul 31, 2023

View reviewed changes

TamarZanzouri added 7 commits July 31, 2023 15:44

wip use deckslotname isntead of union str/int

937bdcc

wip use DeckSlotName in tests and parsing before and after adjcent slots

cd66144

WIP updating tests

bfe6404

fixed tests to use DeckSlotName - still a lot of other tests are failing

d9cdf99

fixed bug when parsing to enum from int

81c6fc3

fixed bugs with converting to DeckSlotName

0a95b8c

removed print

351c369

TamarZanzouri changed the title ~~Rss 294 deck conflict checker for flex~~ fix(api): enable deck conflict checker for the flex Aug 2, 2023

TamarZanzouri marked this pull request as ready for review August 2, 2023 18:17

TamarZanzouri requested review from a team as code owners August 2, 2023 18:17

TamarZanzouri requested a review from Laura-Danielle August 2, 2023 18:19

added logic for loading thermo in a not allowed location

51dae4c

TamarZanzouri marked this pull request as draft August 3, 2023 01:07

fixed deck definition. added tests and added logic for get_slot_defin…

91d4e48

…ition in engine core

TamarZanzouri added 4 commits August 3, 2023 15:30

added tests for raising if not allowed

62e1abf

removed logic from deck_conflict.py and fixed tests

09dc495

removed temp block from deck item

2e6a26f

added mag block to ot-3 fixture

c12bff6

TamarZanzouri commented Aug 3, 2023

View reviewed changes

TamarZanzouri marked this pull request as ready for review August 3, 2023 20:39

js formatting and added mag block to compatible types

0a7c60e

TamarZanzouri commented Aug 3, 2023

View reviewed changes

Laura-Danielle approved these changes Aug 4, 2023

View reviewed changes

SyntaxColoring reviewed Aug 4, 2023

View reviewed changes

TamarZanzouri and others added 4 commits August 4, 2023 14:43

Update robot-server/tests/integration/protocols/deck_coordinate_load_…

6c6b86d

…fail.py Co-authored-by: Max Marrone <[email protected]>

Update robot-server/tests/integration/http_api/protocols/test_deck_co…

28f6c75

…ordinate_load_fail.tavern.yaml Co-authored-by: Max Marrone <[email protected]>

removed default arg for robot type and changed logic for slot assignment

db8c98a

fixed trash option for ot3

6abab47

TamarZanzouri requested a review from SyntaxColoring August 4, 2023 21:02

SyntaxColoring reviewed Aug 7, 2023

View reviewed changes

api/src/opentrons/motion_planning/deck_conflict.py Outdated Show resolved Hide resolved

change robot_type to RobotType

3168268

TamarZanzouri requested a review from SyntaxColoring August 7, 2023 15:24

removed test for mag deck and ot-3

efbdc75

SyntaxColoring approved these changes Aug 7, 2023

View reviewed changes

fixed bug with deck slot names

87b39fc

SyntaxColoring approved these changes Aug 7, 2023

View reviewed changes

sanni-t approved these changes Aug 7, 2023

View reviewed changes

api/src/opentrons/motion_planning/deck_conflict.py Outdated Show resolved Hide resolved

TamarZanzouri added 2 commits August 7, 2023 14:37

renamed to _ot2_slots_covered_by_thermocycler

bf2810e

documentation change

759061c

TamarZanzouri merged commit 136f933 into edge Aug 7, 2023
48 checks passed

TamarZanzouri deleted the RSS-294-deck-conflict-checker-for-flex branch August 7, 2023 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): enable deck conflict checker for the flex #13189

fix(api): enable deck conflict checker for the flex #13189

TamarZanzouri commented Jul 28, 2023 •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading

TamarZanzouri Jul 31, 2023

Laura-Danielle left a comment

Laura-Danielle Jul 31, 2023

TamarZanzouri Jul 31, 2023

Laura-Danielle Jul 31, 2023

Laura-Danielle Jul 31, 2023

TamarZanzouri Jul 31, 2023

TamarZanzouri Aug 3, 2023

TamarZanzouri Aug 3, 2023

Laura-Danielle Aug 4, 2023

Laura-Danielle left a comment

Laura-Danielle Aug 4, 2023

TamarZanzouri Aug 4, 2023

Laura-Danielle Aug 4, 2023

SyntaxColoring left a comment

SyntaxColoring left a comment

SyntaxColoring Aug 7, 2023

TamarZanzouri commented Aug 7, 2023 •

edited

Loading

SyntaxColoring left a comment

sanni-t left a comment •

edited

Loading

fix(api): enable deck conflict checker for the flex #13189

fix(api): enable deck conflict checker for the flex #13189

Conversation

TamarZanzouri commented Jul 28, 2023 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Jul 28, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Laura-Danielle 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

Laura-Danielle 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

SyntaxColoring left a comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamarZanzouri commented Aug 7, 2023 • edited Loading

SyntaxColoring left a comment

Choose a reason for hiding this comment

sanni-t left a comment • edited Loading

Choose a reason for hiding this comment

TamarZanzouri commented Jul 28, 2023 •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading

TamarZanzouri commented Aug 7, 2023 •

edited

Loading

sanni-t left a comment •

edited

Loading