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

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

Merged
merged 35 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
45d3338
WIP started check for deck conflict in ot3
TamarZanzouri Jul 26, 2023
8fbfd93
split _create_restrictions
TamarZanzouri Jul 27, 2023
7c77ebd
flex restrictions
TamarZanzouri Jul 27, 2023
f0cd5b5
Temp deck deck item and fixed tests for engine deck conflict
TamarZanzouri Jul 27, 2023
b9d77e4
added tests for raising when module is not allowed
TamarZanzouri Jul 27, 2023
a97512a
fixed bug with left and right sides of loading modules
TamarZanzouri Jul 28, 2023
815a764
failed deck conflict e2e test
TamarZanzouri Jul 28, 2023
b4aaa04
removed comment
TamarZanzouri Jul 28, 2023
f3a8400
WIP - use str/int as slots
TamarZanzouri Jul 28, 2023
7e643f8
wip fixing tests
TamarZanzouri Jul 28, 2023
17c6ed0
fixed logic and tests
TamarZanzouri Jul 31, 2023
b4fa2d0
fixed tavern test
TamarZanzouri Jul 31, 2023
937bdcc
wip use deckslotname isntead of union str/int
TamarZanzouri Jul 31, 2023
cd66144
wip use DeckSlotName in tests and parsing before and after adjcent slots
TamarZanzouri Aug 1, 2023
bfe6404
WIP updating tests
TamarZanzouri Aug 1, 2023
d9cdf99
fixed tests to use DeckSlotName - still a lot of other tests are failing
TamarZanzouri Aug 2, 2023
81c6fc3
fixed bug when parsing to enum from int
TamarZanzouri Aug 2, 2023
0a95b8c
fixed bugs with converting to DeckSlotName
TamarZanzouri Aug 2, 2023
351c369
removed print
TamarZanzouri Aug 2, 2023
51dae4c
added logic for loading thermo in a not allowed location
TamarZanzouri Aug 2, 2023
91d4e48
fixed deck definition. added tests and added logic for get_slot_defin…
TamarZanzouri Aug 3, 2023
62e1abf
added tests for raising if not allowed
TamarZanzouri Aug 3, 2023
09dc495
removed logic from deck_conflict.py and fixed tests
TamarZanzouri Aug 3, 2023
2e6a26f
removed temp block from deck item
TamarZanzouri Aug 3, 2023
c12bff6
added mag block to ot-3 fixture
TamarZanzouri Aug 3, 2023
0a7c60e
js formatting and added mag block to compatible types
TamarZanzouri Aug 3, 2023
6c6b86d
Update robot-server/tests/integration/protocols/deck_coordinate_load_…
TamarZanzouri Aug 4, 2023
28f6c75
Update robot-server/tests/integration/http_api/protocols/test_deck_co…
TamarZanzouri Aug 4, 2023
db8c98a
removed default arg for robot type and changed logic for slot assignment
TamarZanzouri Aug 4, 2023
6abab47
fixed trash option for ot3
TamarZanzouri Aug 4, 2023
3168268
change robot_type to RobotType
TamarZanzouri Aug 7, 2023
efbdc75
removed test for mag deck and ot-3
TamarZanzouri Aug 7, 2023
87b39fc
fixed bug with deck slot names
TamarZanzouri Aug 7, 2023
bf2810e
renamed to _ot2_slots_covered_by_thermocycler
TamarZanzouri Aug 7, 2023
759061c
documentation change
TamarZanzouri Aug 7, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 78 additions & 25 deletions api/src/opentrons/motion_planning/deck_conflict.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
get_adjacent_slots,
)

from opentrons.types import DeckSlotName

_FIXED_TRASH_SLOT: Final = 12
_FIXED_TRASH_SLOT: Final = DeckSlotName.FIXED_TRASH


# The maximum height allowed for items adjacent to a Heater-Shaker in the x-direction.
Expand Down Expand Up @@ -93,9 +94,9 @@ class OtherModule(_Module):
class _NothingAllowed(NamedTuple):
"""Nothing is allowed in this slot."""

location: int
location: DeckSlotName
source_item: DeckItem
source_location: int
source_location: DeckSlotName

def is_allowed(self, item: DeckItem) -> bool:
return False
Expand All @@ -104,9 +105,9 @@ def is_allowed(self, item: DeckItem) -> bool:
class _MaxHeight(NamedTuple):
"""Nothing over a certain height is allowed in this slot."""

location: int
location: DeckSlotName
source_item: DeckItem
source_location: int
source_location: DeckSlotName
max_height: float
allowed_labware: List[LabwareUri]

Expand All @@ -123,9 +124,9 @@ def is_allowed(self, item: DeckItem) -> bool:
class _NoModule(NamedTuple):
"""No module of any kind is allowed in this slot."""

location: int
location: DeckSlotName
source_item: DeckItem
source_location: int
source_location: DeckSlotName

def is_allowed(self, item: DeckItem) -> bool:
return not isinstance(item, _Module)
Expand All @@ -134,9 +135,9 @@ def is_allowed(self, item: DeckItem) -> bool:
class _NoHeaterShakerModule(NamedTuple):
"""No Heater-Shaker module is allowed in this slot."""

location: int
location: DeckSlotName
source_item: DeckItem
source_location: int
source_location: DeckSlotName

def is_allowed(self, item: DeckItem) -> bool:
return not isinstance(item, HeaterShakerModule)
Expand All @@ -145,7 +146,7 @@ def is_allowed(self, item: DeckItem) -> bool:
class _FixedTrashOnly(NamedTuple):
"""Only fixed-trash labware is allowed in this slot."""

location: int = _FIXED_TRASH_SLOT
location: DeckSlotName = _FIXED_TRASH_SLOT

def is_allowed(self, item: DeckItem) -> bool:
return _is_fixed_trash(item)
Expand All @@ -169,9 +170,10 @@ class DeckConflictError(ValueError):
# things that don't fit into a single deck slot, like the Thermocycler.
# Refactor this interface to take a more symbolic location.
def check(
existing_items: Mapping[int, DeckItem],
existing_items: Mapping[DeckSlotName, DeckItem],
new_item: DeckItem,
new_location: int,
new_location: DeckSlotName,
robot_type: str = "OT-2 Standard",
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

TamarZanzouri marked this conversation as resolved.
Show resolved Hide resolved
) -> None:
"""Check a deck layout for conflicts.

Expand All @@ -184,10 +186,11 @@ def check(
DeckConflictError: Adding this item should not be allowed.
"""
restrictions: List[_DeckRestriction] = [_FixedTrashOnly()]
SyntaxColoring marked this conversation as resolved.
Show resolved Hide resolved

# build restrictions driven by existing items
for location, item in existing_items.items():
restrictions += _create_restrictions(item=item, location=location)
restrictions += _create_restrictions(
item=item, location=location, robot_type=robot_type
)

# check new item against existing restrictions
for r in restrictions:
Expand All @@ -198,7 +201,9 @@ def check(

# check new restrictions required by new item
# do not interfere with existing items
new_restrictions = _create_restrictions(item=new_item, location=new_location)
new_restrictions = _create_restrictions(
item=new_item, location=new_location, robot_type=robot_type
)

for r in new_restrictions:
existing_item = existing_items.get(r.location)
Expand All @@ -211,7 +216,9 @@ def check(
)


def _create_restrictions(item: DeckItem, location: int) -> List[_DeckRestriction]:
def _create_ot2_restrictions(
item: DeckItem, location: DeckSlotName
) -> List[_DeckRestriction]:
restrictions: List[_DeckRestriction] = []

if location != _FIXED_TRASH_SLOT:
Expand All @@ -228,11 +235,11 @@ def _create_restrictions(item: DeckItem, location: int) -> List[_DeckRestriction
# A Heater-Shaker can't safely be placed just south of the fixed trash,
# because the fixed trash blocks access to the screw that locks the
# Heater-Shaker onto the deck.
location_south_of_fixed_trash = get_south_slot(location)
location_south_of_fixed_trash = get_south_slot(location.as_int())
if location_south_of_fixed_trash is not None:
restrictions.append(
_NoHeaterShakerModule(
location=location_south_of_fixed_trash,
location=DeckSlotName.from_primitive(location_south_of_fixed_trash),
source_item=item,
source_location=location,
)
Expand All @@ -249,19 +256,19 @@ def _create_restrictions(item: DeckItem, location: int) -> List[_DeckRestriction
)

if isinstance(item, HeaterShakerModule):
for covered_location in get_adjacent_slots(location):
for hs_covered_location in get_adjacent_slots(location.as_int()):
restrictions.append(
_NoModule(
location=covered_location,
location=DeckSlotName.from_primitive(hs_covered_location),
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()):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

restrictions.append(
_MaxHeight(
location=covered_location,
location=DeckSlotName.from_primitive(hs_covered_location),
source_item=item,
source_location=location,
max_height=HS_MAX_X_ADJACENT_ITEM_HEIGHT,
Expand All @@ -272,6 +279,41 @@ def _create_restrictions(item: DeckItem, location: int) -> List[_DeckRestriction
return restrictions


def _create_flex_restrictions(
item: DeckItem, location: DeckSlotName
) -> List[_DeckRestriction]:
restrictions: List[_DeckRestriction] = []

if isinstance(item, ThermocyclerModule):
for covered_location in _flex_slots_covered_by_thermocycler():
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

restrictions.append(
_NothingAllowed(
location=covered_location,
source_item=item,
source_location=location,
)
)
else:
restrictions.append(
_NothingAllowed(
location=location,
source_item=item,
source_location=location,
)
)
return restrictions


def _create_restrictions(
item: DeckItem, location: DeckSlotName, robot_type: str
) -> List[_DeckRestriction]:

if robot_type == "OT-2 Standard":
return _create_ot2_restrictions(item, location)
else:
return _create_flex_restrictions(item, location)


def _create_deck_conflict_error_message(
restriction: _DeckRestriction,
new_item: Optional[DeckItem] = None,
Expand Down Expand Up @@ -302,11 +344,22 @@ def _create_deck_conflict_error_message(
return message


def _slots_covered_by_thermocycler(thermocycler: ThermocyclerModule) -> Set[int]:
def _slots_covered_by_thermocycler(
TamarZanzouri marked this conversation as resolved.
Show resolved Hide resolved
thermocycler: ThermocyclerModule,
) -> Set[DeckSlotName]:
if thermocycler.is_semi_configuration:
return {7, 10}
return {DeckSlotName.SLOT_7, DeckSlotName.SLOT_10}
else:
return {7, 8, 10, 11}
return {
DeckSlotName.SLOT_7,
DeckSlotName.SLOT_10,
DeckSlotName.SLOT_8,
DeckSlotName.SLOT_11,
}


def _flex_slots_covered_by_thermocycler() -> Set[DeckSlotName]:
return {DeckSlotName.SLOT_B1, DeckSlotName.SLOT_A1}


def _is_fixed_trash(item: DeckItem) -> bool:
Expand Down
21 changes: 7 additions & 14 deletions api/src/opentrons/protocol_api/core/engine/deck_conflict.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
OFF_DECK_LOCATION,
)
from opentrons.protocol_engine.errors.exceptions import LabwareNotLoadedOnModuleError
from opentrons.types import DeckSlotName


@overload
Expand Down Expand Up @@ -67,13 +68,6 @@ def check(
opentrons.motion_planning.deck_conflict.DeckConflictError:
If the newly-added item conflicts with one of the existing items.
"""
if engine_state.config.robot_type == "OT-3 Standard":
# No-op if this is an OT-3 deck, for now.
#
# todo(mm, 2023-02-24): Support deck conflict checking for the OT-3.
# This will likely require adding support for it in the underlying
# wrapped_deck_conflict.check() function.
return

if new_labware_id is not None:
new_location_and_item = _map_labware(engine_state, new_labware_id)
Expand All @@ -96,7 +90,7 @@ def check(
)
mapped_existing_modules = (m for m in all_existing_modules if m is not None)

existing_items: Dict[int, wrapped_deck_conflict.DeckItem] = {}
existing_items: Dict[DeckSlotName, wrapped_deck_conflict.DeckItem] = {}
for existing_location, existing_item in itertools.chain(
mapped_existing_labware, mapped_existing_modules
):
Expand All @@ -107,20 +101,21 @@ def check(
existing_items=existing_items,
new_item=new_item,
new_location=new_location,
robot_type=engine_state.config.robot_type,
)


def _map_labware(
engine_state: StateView,
labware_id: str,
) -> Optional[Tuple[int, wrapped_deck_conflict.DeckItem]]:
) -> Optional[Tuple[DeckSlotName, wrapped_deck_conflict.DeckItem]]:
location_from_engine = engine_state.labware.get_location(labware_id=labware_id)

if isinstance(location_from_engine, DeckSlotLocation):
# This labware is loaded directly into a deck slot.
# Map it to a wrapped_deck_conflict.Labware.
return (
_deck_slot_to_int(location_from_engine),
location_from_engine.slotName,
wrapped_deck_conflict.Labware(
name_for_errors=engine_state.labware.get_load_name(
labware_id=labware_id
Expand Down Expand Up @@ -153,12 +148,10 @@ def _map_labware(
def _map_module(
engine_state: StateView,
module_id: str,
) -> Optional[Tuple[int, wrapped_deck_conflict.DeckItem]]:
) -> Optional[Tuple[DeckSlotName, wrapped_deck_conflict.DeckItem]]:
module_model = engine_state.modules.get_connected_model(module_id=module_id)
module_type = module_model.as_type()
mapped_location = _deck_slot_to_int(
engine_state.modules.get_location(module_id=module_id)
)
mapped_location = engine_state.modules.get_location(module_id=module_id).slotName

# Use the module model (e.g. "temperatureModuleV1") as the name for error messages
# because it's convenient for us. Unfortunately, this won't necessarily match
Expand Down
24 changes: 21 additions & 3 deletions api/src/opentrons/protocol_api/core/engine/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from typing import Dict, Optional, Type, Union, List, Tuple

from opentrons.protocol_engine.commands import LoadModuleResult
from opentrons_shared_data.deck.dev_types import DeckDefinitionV3
from opentrons_shared_data.deck.dev_types import DeckDefinitionV3, SlotDefV3
from opentrons_shared_data.labware.labware_definition import LabwareDefinition
from opentrons_shared_data.labware.dev_types import LabwareDefinition as LabwareDefDict
from opentrons_shared_data.pipette.dev_types import PipetteNameType
Expand Down Expand Up @@ -314,14 +314,21 @@ def load_module(
"""Load a module into the protocol."""
assert configuration is None, "Module `configuration` is deprecated"

module_type = ModuleType.from_model(model)
# TODO(mc, 2022-10-20): move to public ProtocolContext
# once `Deck` and `ProtocolEngine` play nicely together
if deck_slot is None:
if ModuleType.from_model(model) == ModuleType.THERMOCYCLER:
deck_slot = DeckSlotName.SLOT_7
if module_type == ModuleType.THERMOCYCLER:
deck_slot = (
SyntaxColoring marked this conversation as resolved.
Show resolved Hide resolved
DeckSlotName.SLOT_7
if self._engine_client.state.config.robot_type == "OT-2 Standard"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

else DeckSlotName.SLOT_B1
)
else:
raise InvalidModuleLocationError(deck_slot, model.name)

self._ensure_module_location(deck_slot, module_type)

result = self._engine_client.load_module(
model=EngineModuleModel(model),
location=DeckSlotLocation(slotName=deck_slot),
Expand Down Expand Up @@ -473,6 +480,17 @@ def get_deck_definition(self) -> DeckDefinitionV3:
"""Get the geometry definition of the robot's deck."""
return self._engine_client.state.labware.get_deck_definition()

def get_slot_definition(self, slot: DeckSlotName) -> SlotDefV3:
return self._engine_client.state.labware.get_slot_definition(slot)

def _ensure_module_location(
self, slot: DeckSlotName, module_type: ModuleType
) -> None:
slot_def = self.get_slot_definition(slot)
compatible_modules = slot_def["compatibleModuleTypes"]
if module_type.value not in compatible_modules:
raise ValueError(f"A {module_type.value} cannot be loaded into slot {slot}")

def get_slot_item(
self, slot_name: DeckSlotName
) -> Union[LabwareCore, ModuleCore, NonConnectedModuleCore, None]:
Expand Down
6 changes: 3 additions & 3 deletions api/src/opentrons/protocol_api/core/legacy/deck.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from opentrons.hardware_control.modules.types import ModuleType
from opentrons.motion_planning import deck_conflict
from opentrons.protocols.api_support.labware_like import LabwareLike
from opentrons.types import DeckLocation, Location, Mount, Point
from opentrons.types import DeckLocation, Location, Mount, Point, DeckSlotName

from opentrons.protocol_api.core.labware import AbstractLabware
from opentrons.protocol_api.deck import CalibrationPosition
Expand Down Expand Up @@ -167,15 +167,15 @@ def __delitem__(self, key: DeckLocation) -> None:
def __setitem__(self, key: DeckLocation, val: DeckItem) -> None:
slot_key_int = self._check_name(key)
existing_items = {
slot: self._map_to_conflict_checker_item(item)
DeckSlotName.from_primitive(slot): self._map_to_conflict_checker_item(item)
for slot, item in self.data.items()
if item is not None
}

# will raise DeckConflictError if items conflict
deck_conflict.check(
existing_items=existing_items,
new_location=slot_key_int,
new_location=DeckSlotName.from_primitive(slot_key_int),
new_item=self._map_to_conflict_checker_item(val),
)

Expand Down
Loading
Loading