From 7d0d449dd50aa5a1c566cf4a9263d5a7b4302399 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 25 Nov 2024 17:12:51 -0500 Subject: [PATCH 01/16] Rename _check_location_is_addressable_area() -> _add_addressable_area(). Based on the apparent current behavior. --- .../protocol_engine/state/addressable_areas.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index bd7d8de0188..235a052dfe6 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -196,7 +196,7 @@ def handle_action(self, action: Action) -> None: if isinstance(action, SucceedCommandAction): self._handle_command(action.command) elif isinstance(action, AddAddressableAreaAction): - self._check_location_is_addressable_area(action.addressable_area) + self._add_addressable_area(action.addressable_area) elif isinstance(action, SetDeckConfigurationAction): current_state = self._state if ( @@ -216,22 +216,22 @@ def _handle_command(self, command: Command) -> None: if isinstance(command.result, LoadLabwareResult): location = command.params.location if isinstance(location, (DeckSlotLocation, AddressableAreaLocation)): - self._check_location_is_addressable_area(location) + self._add_addressable_area(location) elif isinstance(command.result, MoveLabwareResult): location = command.params.newLocation if isinstance(location, (DeckSlotLocation, AddressableAreaLocation)): - self._check_location_is_addressable_area(location) + self._add_addressable_area(location) elif isinstance(command.result, LoadModuleResult): - self._check_location_is_addressable_area(command.params.location) + self._add_addressable_area(command.params.location) elif isinstance( command.result, (MoveToAddressableAreaResult, MoveToAddressableAreaForDropTipResult), ): addressable_area_name = command.params.addressableAreaName - self._check_location_is_addressable_area(addressable_area_name) + self._add_addressable_area(addressable_area_name) @staticmethod def _get_addressable_areas_from_deck_configuration( @@ -260,7 +260,7 @@ def _get_addressable_areas_from_deck_configuration( ) return {area.area_name: area for area in addressable_areas} - def _check_location_is_addressable_area( + def _add_addressable_area( self, location: Union[DeckSlotLocation, AddressableAreaLocation, str] ) -> None: if isinstance(location, DeckSlotLocation): From 714e9f58f4f8fb254521b3fb1576fece27ee33de Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 26 Nov 2024 12:46:35 -0500 Subject: [PATCH 02/16] Remove Pydantic model from AddAddressableAreaAction. AddressableAreaAction's callers already supply it with the bare addressable_area_name string. Wrapping that in a Pydantic model doesn't seem to be useful anymore. --- api/src/opentrons/protocol_engine/actions/actions.py | 5 ++--- api/src/opentrons/protocol_engine/protocol_engine.py | 4 +--- .../opentrons/protocol_engine/state/addressable_areas.py | 2 +- .../state/test_addressable_area_store_old.py | 6 +----- .../opentrons/protocol_engine/test_protocol_engine.py | 7 +------ 5 files changed, 6 insertions(+), 18 deletions(-) diff --git a/api/src/opentrons/protocol_engine/actions/actions.py b/api/src/opentrons/protocol_engine/actions/actions.py index 15b04048699..a9dcc3e7dc3 100644 --- a/api/src/opentrons/protocol_engine/actions/actions.py +++ b/api/src/opentrons/protocol_engine/actions/actions.py @@ -27,7 +27,6 @@ ModuleDefinition, Liquid, DeckConfigurationType, - AddressableAreaLocation, ) @@ -235,12 +234,12 @@ class SetDeckConfigurationAction: class AddAddressableAreaAction: """Add a single addressable area to state. - This differs from the deck configuration in ProvideDeckConfigurationAction which + This differs from the deck configuration in SetDeckConfigurationAction which sends over a mapping of cutout fixtures. This action will only load one addressable area and that should be pre-validated before being sent via the action. """ - addressable_area: AddressableAreaLocation + addressable_area_name: str @dataclasses.dataclass(frozen=True) diff --git a/api/src/opentrons/protocol_engine/protocol_engine.py b/api/src/opentrons/protocol_engine/protocol_engine.py index 3479e0a295b..92d992016cd 100644 --- a/api/src/opentrons/protocol_engine/protocol_engine.py +++ b/api/src/opentrons/protocol_engine/protocol_engine.py @@ -30,7 +30,6 @@ HexColor, PostRunHardwareState, DeckConfigurationType, - AddressableAreaLocation, ) from .execution import ( QueueWorker, @@ -574,9 +573,8 @@ def add_liquid( def add_addressable_area(self, addressable_area_name: str) -> None: """Add an addressable area to state.""" - area = AddressableAreaLocation(addressableAreaName=addressable_area_name) self._action_dispatcher.dispatch( - AddAddressableAreaAction(addressable_area=area) + AddAddressableAreaAction(addressable_area_name) ) def reset_tips(self, labware_id: str) -> None: diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index 235a052dfe6..e8942c37674 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -196,7 +196,7 @@ def handle_action(self, action: Action) -> None: if isinstance(action, SucceedCommandAction): self._handle_command(action.command) elif isinstance(action, AddAddressableAreaAction): - self._add_addressable_area(action.addressable_area) + self._add_addressable_area(action.addressable_area_name) elif isinstance(action, SetDeckConfigurationAction): current_state = self._state if ( diff --git a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py index b9e3e8f4e78..94087aa6d48 100644 --- a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py @@ -308,10 +308,6 @@ def test_add_addressable_area_action( ) -> None: """It should add the addressable area to the store.""" simulated_subject.handle_action( - AddAddressableAreaAction( - addressable_area=AddressableAreaLocation( - addressableAreaName="movableTrashA1" - ) - ) + AddAddressableAreaAction(addressable_area_name="movableTrashA1") ) assert "movableTrashA1" in simulated_subject.state.loaded_addressable_areas_by_name diff --git a/api/tests/opentrons/protocol_engine/test_protocol_engine.py b/api/tests/opentrons/protocol_engine/test_protocol_engine.py index 34dac853ef8..cd6ffeb99cb 100644 --- a/api/tests/opentrons/protocol_engine/test_protocol_engine.py +++ b/api/tests/opentrons/protocol_engine/test_protocol_engine.py @@ -32,7 +32,6 @@ ModuleModel, Liquid, PostRunHardwareState, - AddressableAreaLocation, ) from opentrons.protocol_engine.execution import ( QueueWorker, @@ -1119,11 +1118,7 @@ def test_add_addressable_area( decoy.verify( action_dispatcher.dispatch( - AddAddressableAreaAction( - addressable_area=AddressableAreaLocation( - addressableAreaName="my_funky_area" - ) - ) + AddAddressableAreaAction(addressable_area_name="my_funky_area") ) ) From ed486032a8839b3fc661065a20dd9d5097298602 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 26 Nov 2024 13:04:01 -0500 Subject: [PATCH 03/16] Avoid unnecessary private member access. --- .../opentrons/protocol_engine/commands/load_module.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/api/src/opentrons/protocol_engine/commands/load_module.py b/api/src/opentrons/protocol_engine/commands/load_module.py index 79e67182666..2eae90c134a 100644 --- a/api/src/opentrons/protocol_engine/commands/load_module.py +++ b/api/src/opentrons/protocol_engine/commands/load_module.py @@ -124,10 +124,12 @@ async def execute(self, params: LoadModuleParams) -> SuccessData[LoadModuleResul params.location.slotName.id ) else: - addressable_area = self._state_view.geometry._modules.ensure_and_convert_module_fixture_location( - deck_slot=params.location.slotName, - deck_type=self._state_view.config.deck_type, - model=params.model, + addressable_area = ( + self._state_view.modules.ensure_and_convert_module_fixture_location( + deck_slot=params.location.slotName, + deck_type=self._state_view.config.deck_type, + model=params.model, + ) ) self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( addressable_area From 2c93ede0b01b4ed6fd5be184f858d5ff8c376cdb Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 26 Nov 2024 13:04:28 -0500 Subject: [PATCH 04/16] Add `StateUpdate.addressable_area_used`. --- .../protocol_engine/state/update_types.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/api/src/opentrons/protocol_engine/state/update_types.py b/api/src/opentrons/protocol_engine/state/update_types.py index 1e81881a2b4..25b7802976c 100644 --- a/api/src/opentrons/protocol_engine/state/update_types.py +++ b/api/src/opentrons/protocol_engine/state/update_types.py @@ -268,6 +268,13 @@ class FilesAddedUpdate: file_ids: list[str] +@dataclasses.dataclass +class AddressableAreaUsedUpdate: + """An update that says an addressable area has been used.""" + + addressable_area_name: str + + @dataclasses.dataclass class StateUpdate: """Represents an update to perform on engine state.""" @@ -308,6 +315,8 @@ class StateUpdate: files_added: FilesAddedUpdate | NoChangeType = NO_CHANGE + addressable_area_used: AddressableAreaUsedUpdate | NoChangeType = NO_CHANGE + def append(self, other: Self) -> Self: """Apply another `StateUpdate` "on top of" this one. @@ -334,7 +343,8 @@ def reduce(cls: typing.Type[Self], *args: Self) -> Self: return accumulator # These convenience functions let the caller avoid the boilerplate of constructing a - # complicated dataclass tree. + # complicated dataclass tree, and allow chaining. + @typing.overload def set_pipette_location( self: Self, *, pipette_id: str, new_deck_point: DeckPoint @@ -567,3 +577,10 @@ def set_absorbance_reader_lid(self: Self, module_id: str, is_lid_on: bool) -> Se module_id=module_id, is_lid_on=is_lid_on ) return self + + def set_addressable_area_used(self: Self, addressable_area_name: str) -> Self: + """Mark that an addressable area has been used. See `AddressableAreaUsedUpdate`.""" + self.addressable_area_used = AddressableAreaUsedUpdate( + addressable_area_name=addressable_area_name + ) + return self From a4d348eccc466a7adc5a6504af598cf2e7a3e29b Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 2 Dec 2024 12:39:02 -0500 Subject: [PATCH 05/16] Handle StateUpdates in AddressableAreaStore. fixup --- .../state/addressable_areas.py | 10 +++ .../state/test_addressable_area_store_old.py | 70 ++++++++++++++++++- 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index e8942c37674..48b0b853856 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -36,12 +36,14 @@ DeckConfigurationType, Dimensions, ) +from ..actions.get_state_update import get_state_updates from ..actions import ( Action, SucceedCommandAction, SetDeckConfigurationAction, AddAddressableAreaAction, ) +from . import update_types from .config import Config from ._abstract_store import HasState, HandlesActions @@ -191,8 +193,16 @@ def __init__( robot_definition=robot_definition, ) + # TODO: Port loadLabware, moveLabware, loadModule, moveToAddressableArea, and moveToAddressableAreaForDropTip + # and their tests def handle_action(self, action: Action) -> None: """Modify state in reaction to an action.""" + for state_update in get_state_updates(action): + if state_update.addressable_area_used != update_types.NO_CHANGE: + self._add_addressable_area( + state_update.addressable_area_used.addressable_area_name + ) + if isinstance(action, SucceedCommandAction): self._handle_command(action.command) elif isinstance(action, AddAddressableAreaAction): diff --git a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py index 94087aa6d48..29bed531322 100644 --- a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py @@ -9,14 +9,16 @@ from opentrons_shared_data.deck.types import DeckDefinitionV5 from opentrons_shared_data.labware.labware_definition import Parameters + from opentrons.protocols.models import LabwareDefinition from opentrons.types import DeckSlotName -from opentrons.protocol_engine.commands import Command +from opentrons.protocol_engine.commands import Command, Comment from opentrons.protocol_engine.actions import ( SucceedCommandAction, AddAddressableAreaAction, ) +from opentrons.protocol_engine.state import update_types from opentrons.protocol_engine.state.config import Config from opentrons.protocol_engine.state.addressable_areas import ( AddressableAreaStore, @@ -56,6 +58,11 @@ def _make_deck_config() -> DeckConfigurationType: ] +def _dummy_command() -> Command: + """Return a placeholder command.""" + return Comment.construct() # type: ignore[call-arg] + + @pytest.fixture def simulated_subject( ot3_standard_deck_def: DeckDefinitionV5, @@ -167,6 +174,8 @@ def test_initial_state( assert len(subject.state.loaded_addressable_areas_by_name) == 16 +# todo(mm, 2024-12-02): Delete in favor of test_addressable_area_usage_in_simulation() +# when all of these commands have been ported to StateUpdate. @pytest.mark.parametrize( ("command", "expected_area"), ( @@ -238,6 +247,34 @@ def test_addressable_area_referencing_commands_load_on_simulated_deck( assert expected_area in simulated_subject.state.loaded_addressable_areas_by_name +@pytest.mark.parametrize("addressable_area_name", ["A1", "A4", "gripperWasteChute"]) +def test_addressable_area_usage_in_simulation( + simulated_subject: AddressableAreaStore, + addressable_area_name: str, +) -> None: + """Simulating stores should correctly handle `StateUpdate`s with addressable areas.""" + assert ( + addressable_area_name + not in simulated_subject.state.loaded_addressable_areas_by_name + ) + simulated_subject.handle_action( + SucceedCommandAction( + command=_dummy_command(), + state_update=update_types.StateUpdate( + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name + ) + ), + ) + ) + assert ( + addressable_area_name + in simulated_subject.state.loaded_addressable_areas_by_name + ) + + +# todo(mm, 2024-12-02): Delete in favor of test_addressable_area_usage() +# when all of these commands have been ported to StateUpdate. @pytest.mark.parametrize( ("command", "expected_area"), ( @@ -303,6 +340,37 @@ def test_addressable_area_referencing_commands_load( assert expected_area in subject.state.loaded_addressable_areas_by_name +@pytest.mark.parametrize("addressable_area_name", ["A1", "C4"]) +def test_addressable_area_usage( + subject: AddressableAreaStore, + addressable_area_name: str, +) -> None: + """Non-simulating stores should correctly handle `StateUpdate`s with addressable areas. + + todo(mm, 2024-12-02): This is ported from an older test that said the + subject "should check that the addressable area is in the deck config." But + AddressableAreaStore does not do that--that is the job of AddressableAreaView--and + the original test did not cover that. Do we still need to test anything here, or + can this be deleted? + """ + # The addressable area should have been added by the deck configuration. + # (Tested more explicitly elsewhere.) + assert addressable_area_name in subject.state.loaded_addressable_areas_by_name + + subject.handle_action( + SucceedCommandAction( + command=_dummy_command(), + state_update=update_types.StateUpdate( + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name + ) + ), + ) + ) + # The addressable area should still be there after handling the action. + assert addressable_area_name in subject.state.loaded_addressable_areas_by_name + + def test_add_addressable_area_action( simulated_subject: AddressableAreaStore, ) -> None: From 4469b50319a26569adb67fcf6c8a07c47da4de42 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 26 Nov 2024 13:22:04 -0500 Subject: [PATCH 06/16] Port loadLabware commands. --- .../protocol_engine/commands/load_labware.py | 6 +- .../state/addressable_areas.py | 8 +-- .../commands/test_load_labware.py | 4 +- .../state/test_addressable_area_store_old.py | 59 ------------------- 4 files changed, 8 insertions(+), 69 deletions(-) diff --git a/api/src/opentrons/protocol_engine/commands/load_labware.py b/api/src/opentrons/protocol_engine/commands/load_labware.py index fb97f5d2c87..2d8b8c3df78 100644 --- a/api/src/opentrons/protocol_engine/commands/load_labware.py +++ b/api/src/opentrons/protocol_engine/commands/load_labware.py @@ -104,6 +104,8 @@ async def execute( self, params: LoadLabwareParams ) -> SuccessData[LoadLabwareResult]: """Load definition and calibration data necessary for a labware.""" + state_update = StateUpdate() + # TODO (tz, 8-15-2023): extend column validation to column 1 when working # on https://opentrons.atlassian.net/browse/RSS-258 and completing # https://opentrons.atlassian.net/browse/RSS-255 @@ -128,10 +130,12 @@ async def execute( self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( area_name ) + state_update.set_addressable_area_used(area_name) elif isinstance(params.location, DeckSlotLocation): self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( params.location.slotName.id ) + state_update.set_addressable_area_used(params.location.slotName.id) verified_location = self._state_view.geometry.ensure_location_not_occupied( params.location @@ -144,8 +148,6 @@ async def execute( labware_id=params.labwareId, ) - state_update = StateUpdate() - state_update.set_loaded_labware( labware_id=loaded_labware.labware_id, offset_id=loaded_labware.offsetId, diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index 48b0b853856..2738340afa8 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -14,7 +14,6 @@ from ..commands import ( Command, - LoadLabwareResult, LoadModuleResult, MoveLabwareResult, MoveToAddressableAreaResult, @@ -223,12 +222,7 @@ def handle_action(self, action: Action) -> None: def _handle_command(self, command: Command) -> None: """Modify state in reaction to a command.""" - if isinstance(command.result, LoadLabwareResult): - location = command.params.location - if isinstance(location, (DeckSlotLocation, AddressableAreaLocation)): - self._add_addressable_area(location) - - elif isinstance(command.result, MoveLabwareResult): + if isinstance(command.result, MoveLabwareResult): location = command.params.newLocation if isinstance(location, (DeckSlotLocation, AddressableAreaLocation)): self._add_addressable_area(location) diff --git a/api/tests/opentrons/protocol_engine/commands/test_load_labware.py b/api/tests/opentrons/protocol_engine/commands/test_load_labware.py index 3873f9854b4..811923bb0ab 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_load_labware.py +++ b/api/tests/opentrons/protocol_engine/commands/test_load_labware.py @@ -2,6 +2,7 @@ import inspect from typing import Optional from opentrons.protocol_engine.state.update_types import ( + AddressableAreaUsedUpdate, LoadedLabwareUpdate, StateUpdate, ) @@ -101,7 +102,8 @@ async def test_load_labware_implementation( offset_id="labware-offset-id", new_location=DeckSlotLocation(slotName=DeckSlotName.SLOT_4), display_name=display_name, - ) + ), + addressable_area_used=AddressableAreaUsedUpdate(addressable_area_name="3"), ), ) diff --git a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py index 29bed531322..8c578da6c05 100644 --- a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py @@ -8,9 +8,7 @@ import pytest from opentrons_shared_data.deck.types import DeckDefinitionV5 -from opentrons_shared_data.labware.labware_definition import Parameters -from opentrons.protocols.models import LabwareDefinition from opentrons.types import DeckSlotName from opentrons.protocol_engine.commands import Command, Comment @@ -34,7 +32,6 @@ ) from .command_fixtures import ( - create_load_labware_command, create_load_module_command, create_move_labware_command, create_move_to_addressable_area_command, @@ -179,34 +176,6 @@ def test_initial_state( @pytest.mark.parametrize( ("command", "expected_area"), ( - ( - create_load_labware_command( - location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), - labware_id="test-labware-id", - definition=LabwareDefinition.construct( # type: ignore[call-arg] - parameters=Parameters.construct(loadName="blah"), # type: ignore[call-arg] - namespace="bleh", - version=123, - ), - offset_id="offset-id", - display_name="display-name", - ), - "A1", - ), - ( - create_load_labware_command( - location=AddressableAreaLocation(addressableAreaName="A4"), - labware_id="test-labware-id", - definition=LabwareDefinition.construct( # type: ignore[call-arg] - parameters=Parameters.construct(loadName="blah"), # type: ignore[call-arg] - namespace="bleh", - version=123, - ), - offset_id="offset-id", - display_name="display-name", - ), - "A4", - ), ( create_load_module_command( location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), @@ -278,34 +247,6 @@ def test_addressable_area_usage_in_simulation( @pytest.mark.parametrize( ("command", "expected_area"), ( - ( - create_load_labware_command( - location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), - labware_id="test-labware-id", - definition=LabwareDefinition.construct( # type: ignore[call-arg] - parameters=Parameters.construct(loadName="blah"), # type: ignore[call-arg] - namespace="bleh", - version=123, - ), - offset_id="offset-id", - display_name="display-name", - ), - "A1", - ), - ( - create_load_labware_command( - location=AddressableAreaLocation(addressableAreaName="C4"), - labware_id="test-labware-id", - definition=LabwareDefinition.construct( # type: ignore[call-arg] - parameters=Parameters.construct(loadName="blah"), # type: ignore[call-arg] - namespace="bleh", - version=123, - ), - offset_id="offset-id", - display_name="display-name", - ), - "C4", - ), ( create_load_module_command( location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), From 252167af487c0059a164e6cc74ae144bbe5ada94 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 2 Dec 2024 12:04:14 -0500 Subject: [PATCH 07/16] Update loadLabware test to check addressable area locations. --- .../commands/test_load_labware.py | 46 ++++++++++++------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/api/tests/opentrons/protocol_engine/commands/test_load_labware.py b/api/tests/opentrons/protocol_engine/commands/test_load_labware.py index 811923bb0ab..8229d7f4265 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_load_labware.py +++ b/api/tests/opentrons/protocol_engine/commands/test_load_labware.py @@ -1,13 +1,9 @@ """Test load labware commands.""" import inspect from typing import Optional -from opentrons.protocol_engine.state.update_types import ( - AddressableAreaUsedUpdate, - LoadedLabwareUpdate, - StateUpdate, -) -import pytest +from unittest.mock import sentinel +import pytest from decoy import Decoy from opentrons.types import DeckSlotName @@ -19,12 +15,19 @@ ) from opentrons.protocol_engine.types import ( + AddressableAreaLocation, DeckSlotLocation, + LabwareLocation, OnLabwareLocation, ) from opentrons.protocol_engine.execution import LoadedLabwareData, EquipmentHandler from opentrons.protocol_engine.resources import labware_validation from opentrons.protocol_engine.state.state import StateView +from opentrons.protocol_engine.state.update_types import ( + AddressableAreaUsedUpdate, + LoadedLabwareUpdate, + StateUpdate, +) from opentrons.protocol_engine.commands.command import SuccessData from opentrons.protocol_engine.commands.load_labware import ( @@ -43,33 +46,40 @@ def patch_mock_labware_validation( monkeypatch.setattr(labware_validation, name, decoy.mock(func=func)) -@pytest.mark.parametrize("display_name", [("My custom display name"), (None)]) -async def test_load_labware_implementation( +@pytest.mark.parametrize("display_name", ["My custom display name", None]) +@pytest.mark.parametrize( + ("location", "expected_addressable_area_name"), + [ + (DeckSlotLocation(slotName=DeckSlotName.SLOT_3), "3"), + (AddressableAreaLocation(addressableAreaName="3"), "3"), + ], +) +async def test_load_labware_on_slot_or_addressable_area( decoy: Decoy, well_plate_def: LabwareDefinition, equipment: EquipmentHandler, state_view: StateView, display_name: Optional[str], + location: LabwareLocation, + expected_addressable_area_name: str, ) -> None: """A LoadLabware command should have an execution implementation.""" subject = LoadLabwareImplementation(equipment=equipment, state_view=state_view) data = LoadLabwareParams( - location=DeckSlotLocation(slotName=DeckSlotName.SLOT_3), + location=location, loadName="some-load-name", namespace="opentrons-test", version=1, displayName=display_name, ) - decoy.when( - state_view.geometry.ensure_location_not_occupied( - DeckSlotLocation(slotName=DeckSlotName.SLOT_3) - ) - ).then_return(DeckSlotLocation(slotName=DeckSlotName.SLOT_4)) + decoy.when(state_view.geometry.ensure_location_not_occupied(location)).then_return( + sentinel.validated_empty_location + ) decoy.when( await equipment.load_labware( - location=DeckSlotLocation(slotName=DeckSlotName.SLOT_4), + location=sentinel.validated_empty_location, load_name="some-load-name", namespace="opentrons-test", version=1, @@ -100,10 +110,12 @@ async def test_load_labware_implementation( labware_id="labware-id", definition=well_plate_def, offset_id="labware-offset-id", - new_location=DeckSlotLocation(slotName=DeckSlotName.SLOT_4), + new_location=sentinel.validated_empty_location, display_name=display_name, ), - addressable_area_used=AddressableAreaUsedUpdate(addressable_area_name="3"), + addressable_area_used=AddressableAreaUsedUpdate( + addressable_area_name=expected_addressable_area_name + ), ), ) From a90f7f8349a40fc2d1973958ecd1382552ca66d1 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 2 Dec 2024 13:22:30 -0500 Subject: [PATCH 08/16] Port moveToAddressableArea and moveToAddressableAreaForDropTip. --- .../protocol_engine/commands/movement_common.py | 10 +++++++--- .../protocol_engine/state/addressable_areas.py | 9 --------- .../commands/test_move_to_addressable_area.py | 17 ++++++++++++++--- ...est_move_to_addressable_area_for_drop_tip.py | 12 ++++++++++-- .../state/test_addressable_area_store_old.py | 7 ------- 5 files changed, 31 insertions(+), 24 deletions(-) diff --git a/api/src/opentrons/protocol_engine/commands/movement_common.py b/api/src/opentrons/protocol_engine/commands/movement_common.py index 7917daa8613..ca12d2d1ad8 100644 --- a/api/src/opentrons/protocol_engine/commands/movement_common.py +++ b/api/src/opentrons/protocol_engine/commands/movement_common.py @@ -265,17 +265,21 @@ async def move_to_addressable_area( ) ], ), - state_update=StateUpdate().clear_all_pipette_locations(), + state_update=StateUpdate() + .clear_all_pipette_locations() + .set_addressable_area_used(addressable_area_name=addressable_area_name), ) else: deck_point = DeckPoint.construct(x=x, y=y, z=z) return SuccessData( public=DestinationPositionResult(position=deck_point), - state_update=StateUpdate().set_pipette_location( + state_update=StateUpdate() + .set_pipette_location( pipette_id=pipette_id, new_addressable_area_name=addressable_area_name, new_deck_point=deck_point, - ), + ) + .set_addressable_area_used(addressable_area_name=addressable_area_name), ) diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index 2738340afa8..8a223832602 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -16,8 +16,6 @@ Command, LoadModuleResult, MoveLabwareResult, - MoveToAddressableAreaResult, - MoveToAddressableAreaForDropTipResult, ) from ..errors import ( IncompatibleAddressableAreaError, @@ -230,13 +228,6 @@ def _handle_command(self, command: Command) -> None: elif isinstance(command.result, LoadModuleResult): self._add_addressable_area(command.params.location) - elif isinstance( - command.result, - (MoveToAddressableAreaResult, MoveToAddressableAreaForDropTipResult), - ): - addressable_area_name = command.params.addressableAreaName - self._add_addressable_area(addressable_area_name) - @staticmethod def _get_addressable_areas_from_deck_configuration( deck_config: DeckConfigurationType, deck_definition: DeckDefinitionV5 diff --git a/api/tests/opentrons/protocol_engine/commands/test_move_to_addressable_area.py b/api/tests/opentrons/protocol_engine/commands/test_move_to_addressable_area.py index 9f1470b95da..9142f792252 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_move_to_addressable_area.py +++ b/api/tests/opentrons/protocol_engine/commands/test_move_to_addressable_area.py @@ -93,7 +93,10 @@ async def test_move_to_addressable_area_implementation_non_gen1( pipette_id="abc", new_location=update_types.AddressableArea(addressable_area_name="123"), new_deck_point=DeckPoint(x=9, y=8, z=7), - ) + ), + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name="123" + ), ), ) @@ -154,7 +157,10 @@ async def test_move_to_addressable_area_implementation_with_gen1( pipette_id="abc", new_location=update_types.AddressableArea(addressable_area_name="123"), new_deck_point=DeckPoint(x=9, y=8, z=7), - ) + ), + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name="123" + ), ), ) @@ -206,5 +212,10 @@ async def test_move_to_addressable_area_implementation_handles_stalls( public=StallOrCollisionError.construct( id=test_id, createdAt=timestamp, wrappedErrors=[matchers.Anything()] ), - state_update=update_types.StateUpdate(pipette_location=update_types.CLEAR), + state_update=update_types.StateUpdate( + pipette_location=update_types.CLEAR, + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name="123" + ), + ), ) diff --git a/api/tests/opentrons/protocol_engine/commands/test_move_to_addressable_area_for_drop_tip.py b/api/tests/opentrons/protocol_engine/commands/test_move_to_addressable_area_for_drop_tip.py index 019ec6bec3f..b6ee2097458 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_move_to_addressable_area_for_drop_tip.py +++ b/api/tests/opentrons/protocol_engine/commands/test_move_to_addressable_area_for_drop_tip.py @@ -79,7 +79,10 @@ async def test_move_to_addressable_area_for_drop_tip_implementation( pipette_id="abc", new_location=update_types.AddressableArea(addressable_area_name="123"), new_deck_point=DeckPoint(x=9, y=8, z=7), - ) + ), + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name="123" + ), ), ) @@ -133,5 +136,10 @@ async def test_move_to_addressable_area_for_drop_tip_handles_stalls( public=StallOrCollisionError.construct( id=test_id, createdAt=timestamp, wrappedErrors=[matchers.Anything()] ), - state_update=update_types.StateUpdate(pipette_location=update_types.CLEAR), + state_update=update_types.StateUpdate( + pipette_location=update_types.CLEAR, + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name="123" + ), + ), ) diff --git a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py index 8c578da6c05..b17ee51d6e7 100644 --- a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py @@ -34,7 +34,6 @@ from .command_fixtures import ( create_load_module_command, create_move_labware_command, - create_move_to_addressable_area_command, ) @@ -198,12 +197,6 @@ def test_initial_state( ), "A4", ), - ( - create_move_to_addressable_area_command( - pipette_id="pipette-id", addressable_area_name="gripperWasteChute" - ), - "gripperWasteChute", - ), ), ) def test_addressable_area_referencing_commands_load_on_simulated_deck( From 3e0440f220823562efd8ff594bbbb4d1c869f131 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 3 Dec 2024 15:45:33 -0500 Subject: [PATCH 09/16] Remove deck_type arg from ensure_and_convert_module_fixture_location(). --- .../commands/absorbance_reader/close_lid.py | 1 - .../commands/absorbance_reader/open_lid.py | 1 - .../opentrons/protocol_engine/commands/load_module.py | 9 +++------ api/src/opentrons/protocol_engine/state/modules.py | 5 +++-- 4 files changed, 6 insertions(+), 10 deletions(-) diff --git a/api/src/opentrons/protocol_engine/commands/absorbance_reader/close_lid.py b/api/src/opentrons/protocol_engine/commands/absorbance_reader/close_lid.py index 069c2803b22..14c1f0f9ea3 100644 --- a/api/src/opentrons/protocol_engine/commands/absorbance_reader/close_lid.py +++ b/api/src/opentrons/protocol_engine/commands/absorbance_reader/close_lid.py @@ -95,7 +95,6 @@ async def execute(self, params: CloseLidParams) -> SuccessData[CloseLidResult]: deck_slot=self._state_view.modules.get_location( params.moduleId ).slotName, - deck_type=self._state_view.config.deck_type, model=absorbance_model, ) ) diff --git a/api/src/opentrons/protocol_engine/commands/absorbance_reader/open_lid.py b/api/src/opentrons/protocol_engine/commands/absorbance_reader/open_lid.py index 1ad56413f9a..96495a2bcde 100644 --- a/api/src/opentrons/protocol_engine/commands/absorbance_reader/open_lid.py +++ b/api/src/opentrons/protocol_engine/commands/absorbance_reader/open_lid.py @@ -91,7 +91,6 @@ async def execute(self, params: OpenLidParams) -> SuccessData[OpenLidResult]: deck_slot=self._state_view.modules.get_location( params.moduleId ).slotName, - deck_type=self._state_view.config.deck_type, model=absorbance_model, ) ) diff --git a/api/src/opentrons/protocol_engine/commands/load_module.py b/api/src/opentrons/protocol_engine/commands/load_module.py index 2eae90c134a..b8fafd93264 100644 --- a/api/src/opentrons/protocol_engine/commands/load_module.py +++ b/api/src/opentrons/protocol_engine/commands/load_module.py @@ -124,12 +124,9 @@ async def execute(self, params: LoadModuleParams) -> SuccessData[LoadModuleResul params.location.slotName.id ) else: - addressable_area = ( - self._state_view.modules.ensure_and_convert_module_fixture_location( - deck_slot=params.location.slotName, - deck_type=self._state_view.config.deck_type, - model=params.model, - ) + addressable_area = self._state_view.modules.ensure_and_convert_module_fixture_location( + deck_slot=params.location.slotName, + model=params.model, ) self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( addressable_area diff --git a/api/src/opentrons/protocol_engine/state/modules.py b/api/src/opentrons/protocol_engine/state/modules.py index 0292329b8ea..ebf503c51fb 100644 --- a/api/src/opentrons/protocol_engine/state/modules.py +++ b/api/src/opentrons/protocol_engine/state/modules.py @@ -908,7 +908,7 @@ def get_nominal_offset_to_child( "Module location invalid for nominal module offset calculation." ) module_addressable_area = self.ensure_and_convert_module_fixture_location( - location, self._state.deck_type, module.model + location, module.model ) module_addressable_area_position = ( addressable_areas.get_addressable_area_offsets_from_cutout( @@ -1281,13 +1281,14 @@ def convert_absorbance_reader_data_points( def ensure_and_convert_module_fixture_location( self, deck_slot: DeckSlotName, - deck_type: DeckType, model: ModuleModel, ) -> str: """Ensure module fixture load location is valid. Also, convert the deck slot to a valid module fixture addressable area. """ + deck_type = self._state.deck_type + if deck_type == DeckType.OT2_STANDARD or deck_type == DeckType.OT2_SHORT_TRASH: raise ValueError( f"Invalid Deck Type: {deck_type.name} - Does not support modules as fixtures." From a4ff423fffb4f67cf67c949e6800c8468050318e Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 2 Dec 2024 14:30:35 -0500 Subject: [PATCH 10/16] Port loadModule. --- .../protocol_engine/commands/load_module.py | 19 +++++++-- .../state/addressable_areas.py | 4 -- .../commands/test_load_module.py | 41 ++++++++++++++++++- .../state/test_addressable_area_store_old.py | 18 -------- 4 files changed, 56 insertions(+), 26 deletions(-) diff --git a/api/src/opentrons/protocol_engine/commands/load_module.py b/api/src/opentrons/protocol_engine/commands/load_module.py index b8fafd93264..a03e3745511 100644 --- a/api/src/opentrons/protocol_engine/commands/load_module.py +++ b/api/src/opentrons/protocol_engine/commands/load_module.py @@ -4,6 +4,8 @@ from typing_extensions import Literal from pydantic import BaseModel, Field +from opentrons.protocol_engine.state.update_types import StateUpdate + from .command import AbstractCommandImpl, BaseCommand, BaseCommandCreate, SuccessData from ..errors.error_occurrence import ErrorOccurrence from ..types import ( @@ -116,6 +118,8 @@ def __init__( async def execute(self, params: LoadModuleParams) -> SuccessData[LoadModuleResult]: """Check that the requested module is attached and assign its identifier.""" + state_update = StateUpdate() + module_type = params.model.as_type() self._ensure_module_location(params.location.slotName, module_type) @@ -123,14 +127,22 @@ async def execute(self, params: LoadModuleParams) -> SuccessData[LoadModuleResul self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( params.location.slotName.id ) + state_update.set_addressable_area_used( + addressable_area_name=params.location.slotName.id + ) else: - addressable_area = self._state_view.modules.ensure_and_convert_module_fixture_location( - deck_slot=params.location.slotName, - model=params.model, + addressable_area = ( + self._state_view.modules.ensure_and_convert_module_fixture_location( + deck_slot=params.location.slotName, + model=params.model, + ) ) self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( addressable_area ) + state_update.set_addressable_area_used( + addressable_area_name=addressable_area + ) verified_location = self._state_view.geometry.ensure_location_not_occupied( params.location @@ -156,6 +168,7 @@ async def execute(self, params: LoadModuleParams) -> SuccessData[LoadModuleResul model=loaded_module.definition.model, definition=loaded_module.definition, ), + state_update=state_update, ) def _ensure_module_location( diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index 8a223832602..f44aa6c8f6c 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -14,7 +14,6 @@ from ..commands import ( Command, - LoadModuleResult, MoveLabwareResult, ) from ..errors import ( @@ -225,9 +224,6 @@ def _handle_command(self, command: Command) -> None: if isinstance(location, (DeckSlotLocation, AddressableAreaLocation)): self._add_addressable_area(location) - elif isinstance(command.result, LoadModuleResult): - self._add_addressable_area(command.params.location) - @staticmethod def _get_addressable_areas_from_deck_configuration( deck_config: DeckConfigurationType, deck_definition: DeckDefinitionV5 diff --git a/api/tests/opentrons/protocol_engine/commands/test_load_module.py b/api/tests/opentrons/protocol_engine/commands/test_load_module.py index e5098b5dc49..7444eb160d4 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_load_module.py +++ b/api/tests/opentrons/protocol_engine/commands/test_load_module.py @@ -1,9 +1,12 @@ """Test load module command.""" -import pytest from typing import cast +from unittest.mock import sentinel + +import pytest from decoy import Decoy from opentrons.protocol_engine.errors import LocationIsOccupiedError +from opentrons.protocol_engine.state import update_types from opentrons.protocol_engine.state.state import StateView from opentrons_shared_data.robot.types import RobotType from opentrons.types import DeckSlotName @@ -70,6 +73,13 @@ async def test_load_module_implementation( ) ).then_return(DeckSlotLocation(slotName=DeckSlotName.SLOT_2)) + decoy.when( + state_view.modules.ensure_and_convert_module_fixture_location( + deck_slot=data.location.slotName, + model=data.model, + ) + ).then_return(sentinel.addressable_area_name) + decoy.when( await equipment.load_module( model=ModuleModel.TEMPERATURE_MODULE_V2, @@ -92,6 +102,11 @@ async def test_load_module_implementation( model=ModuleModel.TEMPERATURE_MODULE_V2, definition=tempdeck_v2_def, ), + state_update=update_types.StateUpdate( + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name=sentinel.addressable_area_name + ) + ), ) @@ -125,6 +140,13 @@ async def test_load_module_implementation_mag_block( ) ).then_return(DeckSlotLocation(slotName=DeckSlotName.SLOT_2)) + decoy.when( + state_view.modules.ensure_and_convert_module_fixture_location( + deck_slot=data.location.slotName, + model=data.model, + ) + ).then_return(sentinel.addressable_area_name) + decoy.when( await equipment.load_magnetic_block( model=ModuleModel.MAGNETIC_BLOCK_V1, @@ -147,6 +169,11 @@ async def test_load_module_implementation_mag_block( model=ModuleModel.MAGNETIC_BLOCK_V1, definition=mag_block_v1_def, ), + state_update=update_types.StateUpdate( + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name=sentinel.addressable_area_name + ) + ), ) @@ -180,6 +207,13 @@ async def test_load_module_implementation_abs_reader( ) ).then_return(DeckSlotLocation(slotName=DeckSlotName.SLOT_D3)) + decoy.when( + state_view.modules.ensure_and_convert_module_fixture_location( + deck_slot=data.location.slotName, + model=data.model, + ) + ).then_return(sentinel.addressable_area_name) + decoy.when( await equipment.load_module( model=ModuleModel.ABSORBANCE_READER_V1, @@ -202,6 +236,11 @@ async def test_load_module_implementation_abs_reader( model=ModuleModel.ABSORBANCE_READER_V1, definition=abs_reader_v1_def, ), + state_update=update_types.StateUpdate( + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name=sentinel.addressable_area_name + ) + ), ) diff --git a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py index b17ee51d6e7..835783e07c0 100644 --- a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py @@ -25,14 +25,12 @@ from opentrons.protocol_engine.types import ( DeckType, DeckConfigurationType, - ModuleModel, LabwareMovementStrategy, DeckSlotLocation, AddressableAreaLocation, ) from .command_fixtures import ( - create_load_module_command, create_move_labware_command, ) @@ -175,14 +173,6 @@ def test_initial_state( @pytest.mark.parametrize( ("command", "expected_area"), ( - ( - create_load_module_command( - location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), - module_id="test-module-id", - model=ModuleModel.TEMPERATURE_MODULE_V2, - ), - "A1", - ), ( create_move_labware_command( new_location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), @@ -240,14 +230,6 @@ def test_addressable_area_usage_in_simulation( @pytest.mark.parametrize( ("command", "expected_area"), ( - ( - create_load_module_command( - location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), - module_id="test-module-id", - model=ModuleModel.TEMPERATURE_MODULE_V2, - ), - "A1", - ), ( create_move_labware_command( new_location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), From 09851a484d27e4cd4fd03404a02b43d9a01c93d6 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 3 Dec 2024 10:59:32 -0500 Subject: [PATCH 11/16] Todo comments for OT-2/Flex split. --- api/src/opentrons/protocol_engine/commands/load_module.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/src/opentrons/protocol_engine/commands/load_module.py b/api/src/opentrons/protocol_engine/commands/load_module.py index a03e3745511..8e1a8e6489c 100644 --- a/api/src/opentrons/protocol_engine/commands/load_module.py +++ b/api/src/opentrons/protocol_engine/commands/load_module.py @@ -123,6 +123,9 @@ async def execute(self, params: LoadModuleParams) -> SuccessData[LoadModuleResul module_type = params.model.as_type() self._ensure_module_location(params.location.slotName, module_type) + # todo(mm, 2024-12-03): Theoretically, we should be able to deal with + # addressable areas and deck configurations the same way between OT-2 and Flex. + # Can this be simplified? if self._state_view.config.robot_type == "OT-2 Standard": self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( params.location.slotName.id @@ -174,6 +177,9 @@ async def execute(self, params: LoadModuleParams) -> SuccessData[LoadModuleResul def _ensure_module_location( self, slot: DeckSlotName, module_type: ModuleType ) -> None: + # todo(mm, 2024-12-03): Theoretically, we should be able to deal with + # addressable areas and deck configurations the same way between OT-2 and Flex. + # Can this be simplified? if self._state_view.config.robot_type == "OT-2 Standard": slot_def = self._state_view.addressable_areas.get_slot_definition(slot.id) compatible_modules = slot_def["compatibleModuleTypes"] From cdfab24d7261bacda830e15d9327d8fb36bbda74 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 3 Dec 2024 16:30:33 -0500 Subject: [PATCH 12/16] Port moveLabware. --- .../protocol_engine/commands/move_labware.py | 4 ++ .../state/addressable_areas.py | 16 +---- .../commands/test_move_labware.py | 58 ++++++++------- .../state/test_addressable_area_store_old.py | 71 ------------------- 4 files changed, 39 insertions(+), 110 deletions(-) diff --git a/api/src/opentrons/protocol_engine/commands/move_labware.py b/api/src/opentrons/protocol_engine/commands/move_labware.py index 09cdc08561c..b64491f5192 100644 --- a/api/src/opentrons/protocol_engine/commands/move_labware.py +++ b/api/src/opentrons/protocol_engine/commands/move_labware.py @@ -156,6 +156,7 @@ async def execute(self, params: MoveLabwareParams) -> _ExecuteReturn: # noqa: C self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( area_name ) + state_update.set_addressable_area_used(addressable_area_name=area_name) if fixture_validation.is_gripper_waste_chute(area_name): # When dropping off labware in the waste chute, some bigger pieces @@ -201,6 +202,9 @@ async def execute(self, params: MoveLabwareParams) -> _ExecuteReturn: # noqa: C self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( params.newLocation.slotName.id ) + state_update.set_addressable_area_used( + addressable_area_name=params.newLocation.slotName.id + ) available_new_location = self._state_view.geometry.ensure_location_not_occupied( location=params.newLocation diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index f44aa6c8f6c..14bb1ba8f2d 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -12,10 +12,6 @@ from opentrons.types import Point, DeckSlotName -from ..commands import ( - Command, - MoveLabwareResult, -) from ..errors import ( IncompatibleAddressableAreaError, AreaNotInDeckConfigurationError, @@ -35,7 +31,6 @@ from ..actions.get_state_update import get_state_updates from ..actions import ( Action, - SucceedCommandAction, SetDeckConfigurationAction, AddAddressableAreaAction, ) @@ -199,9 +194,7 @@ def handle_action(self, action: Action) -> None: state_update.addressable_area_used.addressable_area_name ) - if isinstance(action, SucceedCommandAction): - self._handle_command(action.command) - elif isinstance(action, AddAddressableAreaAction): + if isinstance(action, AddAddressableAreaAction): self._add_addressable_area(action.addressable_area_name) elif isinstance(action, SetDeckConfigurationAction): current_state = self._state @@ -217,13 +210,6 @@ def handle_action(self, action: Action) -> None: ) ) - def _handle_command(self, command: Command) -> None: - """Modify state in reaction to a command.""" - if isinstance(command.result, MoveLabwareResult): - location = command.params.newLocation - if isinstance(location, (DeckSlotLocation, AddressableAreaLocation)): - self._add_addressable_area(location) - @staticmethod def _get_addressable_areas_from_deck_configuration( deck_config: DeckConfigurationType, deck_definition: DeckDefinitionV5 diff --git a/api/tests/opentrons/protocol_engine/commands/test_move_labware.py b/api/tests/opentrons/protocol_engine/commands/test_move_labware.py index a946eccf05d..49e3c4f5471 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_move_labware.py +++ b/api/tests/opentrons/protocol_engine/commands/test_move_labware.py @@ -1,6 +1,8 @@ """Test the ``moveLabware`` command.""" from datetime import datetime import inspect +from unittest.mock import sentinel + import pytest from decoy import Decoy, matchers @@ -90,9 +92,10 @@ async def test_manual_move_labware_implementation( times_pause_called: int, ) -> None: """It should execute a pause and return the new offset.""" + new_location = DeckSlotLocation(slotName=DeckSlotName.SLOT_4) data = MoveLabwareParams( labwareId="my-cool-labware-id", - newLocation=DeckSlotLocation(slotName=DeckSlotName.SLOT_4), + newLocation=new_location, strategy=strategy, ) @@ -131,7 +134,10 @@ async def test_manual_move_labware_implementation( labware_id="my-cool-labware-id", offset_id="wowzers-a-new-offset-id", new_location=DeckSlotLocation(slotName=DeckSlotName.SLOT_5), - ) + ), + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name=new_location.slotName.id + ), ), ) @@ -211,20 +217,19 @@ async def test_gripper_move_labware_implementation( """It should delegate to the equipment handler and return the new offset.""" from_location = DeckSlotLocation(slotName=DeckSlotName.SLOT_1) new_location = DeckSlotLocation(slotName=DeckSlotName.SLOT_5) + pick_up_offset = LabwareOffsetVector(x=1, y=2, z=3) data = MoveLabwareParams( labwareId="my-cool-labware-id", - newLocation=DeckSlotLocation(slotName=DeckSlotName.SLOT_4), + newLocation=new_location, strategy=LabwareMovementStrategy.USING_GRIPPER, - pickUpOffset=LabwareOffsetVector(x=1, y=2, z=3), + pickUpOffset=pick_up_offset, dropOffset=None, ) decoy.when( state_view.labware.get_definition(labware_id="my-cool-labware-id") - ).then_return( - LabwareDefinition.construct(namespace="my-cool-namespace") # type: ignore[call-arg] - ) + ).then_return(sentinel.labware_definition) decoy.when(state_view.labware.get(labware_id="my-cool-labware-id")).then_return( LoadedLabware( id="my-cool-labware-id", @@ -235,29 +240,25 @@ async def test_gripper_move_labware_implementation( ) ) decoy.when( - state_view.geometry.ensure_location_not_occupied( - location=DeckSlotLocation(slotName=DeckSlotName.SLOT_4), - ) - ).then_return(DeckSlotLocation(slotName=DeckSlotName.SLOT_5)) + state_view.geometry.ensure_location_not_occupied(location=new_location) + ).then_return(sentinel.new_location_validated_unoccupied) decoy.when( equipment.find_applicable_labware_offset_id( labware_definition_uri="opentrons-test/load-name/1", - labware_location=new_location, + labware_location=sentinel.new_location_validated_unoccupied, ) ).then_return("wowzers-a-new-offset-id") - validated_from_location = DeckSlotLocation(slotName=DeckSlotName.SLOT_6) - validated_new_location = DeckSlotLocation(slotName=DeckSlotName.SLOT_7) decoy.when( state_view.geometry.ensure_valid_gripper_location(from_location) - ).then_return(validated_from_location) - decoy.when( - state_view.geometry.ensure_valid_gripper_location(new_location) - ).then_return(validated_new_location) + ).then_return(sentinel.from_location_validated_for_gripper) decoy.when( - labware_validation.validate_gripper_compatible( - LabwareDefinition.construct(namespace="my-cool-namespace") # type: ignore[call-arg] + state_view.geometry.ensure_valid_gripper_location( + sentinel.new_location_validated_unoccupied ) + ).then_return(sentinel.new_location_validated_for_gripper) + decoy.when( + labware_validation.validate_gripper_compatible(sentinel.labware_definition) ).then_return(True) result = await subject.execute(data) @@ -265,10 +266,10 @@ async def test_gripper_move_labware_implementation( state_view.labware.raise_if_labware_has_labware_on_top("my-cool-labware-id"), await labware_movement.move_labware_with_gripper( labware_id="my-cool-labware-id", - current_location=validated_from_location, - new_location=validated_new_location, + current_location=sentinel.from_location_validated_for_gripper, + new_location=sentinel.new_location_validated_for_gripper, user_offset_data=LabwareMovementOffsetData( - pickUpOffset=LabwareOffsetVector(x=1, y=2, z=3), + pickUpOffset=pick_up_offset, dropOffset=LabwareOffsetVector(x=0, y=0, z=0), ), post_drop_slide_offset=None, @@ -282,9 +283,12 @@ async def test_gripper_move_labware_implementation( pipette_location=update_types.CLEAR, labware_location=update_types.LabwareLocationUpdate( labware_id="my-cool-labware-id", - new_location=new_location, + new_location=sentinel.new_location_validated_unoccupied, offset_id="wowzers-a-new-offset-id", ), + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name=new_location.slotName.id + ), ), ) @@ -380,6 +384,9 @@ async def test_gripper_error( state_update=update_types.StateUpdate( labware_location=update_types.NO_CHANGE, pipette_location=update_types.CLEAR, + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name=new_location.slotName.id + ), ), ) @@ -520,6 +527,9 @@ async def test_gripper_move_to_waste_chute_implementation( new_location=new_location, offset_id="wowzers-a-new-offset-id", ), + addressable_area_used=update_types.AddressableAreaUsedUpdate( + addressable_area_name=new_location.addressableAreaName + ), ), ) diff --git a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py index 835783e07c0..e861a006e04 100644 --- a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py @@ -9,8 +9,6 @@ from opentrons_shared_data.deck.types import DeckDefinitionV5 -from opentrons.types import DeckSlotName - from opentrons.protocol_engine.commands import Command, Comment from opentrons.protocol_engine.actions import ( SucceedCommandAction, @@ -25,13 +23,6 @@ from opentrons.protocol_engine.types import ( DeckType, DeckConfigurationType, - LabwareMovementStrategy, - DeckSlotLocation, - AddressableAreaLocation, -) - -from .command_fixtures import ( - create_move_labware_command, ) @@ -168,37 +159,6 @@ def test_initial_state( assert len(subject.state.loaded_addressable_areas_by_name) == 16 -# todo(mm, 2024-12-02): Delete in favor of test_addressable_area_usage_in_simulation() -# when all of these commands have been ported to StateUpdate. -@pytest.mark.parametrize( - ("command", "expected_area"), - ( - ( - create_move_labware_command( - new_location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), - strategy=LabwareMovementStrategy.USING_GRIPPER, - ), - "A1", - ), - ( - create_move_labware_command( - new_location=AddressableAreaLocation(addressableAreaName="A4"), - strategy=LabwareMovementStrategy.USING_GRIPPER, - ), - "A4", - ), - ), -) -def test_addressable_area_referencing_commands_load_on_simulated_deck( - command: Command, - expected_area: str, - simulated_subject: AddressableAreaStore, -) -> None: - """It should check and store the addressable area when referenced in a command.""" - simulated_subject.handle_action(SucceedCommandAction(command=command)) - assert expected_area in simulated_subject.state.loaded_addressable_areas_by_name - - @pytest.mark.parametrize("addressable_area_name", ["A1", "A4", "gripperWasteChute"]) def test_addressable_area_usage_in_simulation( simulated_subject: AddressableAreaStore, @@ -225,37 +185,6 @@ def test_addressable_area_usage_in_simulation( ) -# todo(mm, 2024-12-02): Delete in favor of test_addressable_area_usage() -# when all of these commands have been ported to StateUpdate. -@pytest.mark.parametrize( - ("command", "expected_area"), - ( - ( - create_move_labware_command( - new_location=DeckSlotLocation(slotName=DeckSlotName.SLOT_A1), - strategy=LabwareMovementStrategy.USING_GRIPPER, - ), - "A1", - ), - ( - create_move_labware_command( - new_location=AddressableAreaLocation(addressableAreaName="C4"), - strategy=LabwareMovementStrategy.USING_GRIPPER, - ), - "C4", - ), - ), -) -def test_addressable_area_referencing_commands_load( - command: Command, - expected_area: str, - subject: AddressableAreaStore, -) -> None: - """It should check that the addressable area is in the deck config.""" - subject.handle_action(SucceedCommandAction(command=command)) - assert expected_area in subject.state.loaded_addressable_areas_by_name - - @pytest.mark.parametrize("addressable_area_name", ["A1", "C4"]) def test_addressable_area_usage( subject: AddressableAreaStore, From ed1bf0ef71f3dae3daead5a80ea1667ff3e90534 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 3 Dec 2024 16:03:15 -0500 Subject: [PATCH 13/16] Simplify _add_addressable_area(). --- .../protocol_engine/state/addressable_areas.py | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index 14bb1ba8f2d..d612456b7bd 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -1,7 +1,7 @@ """Basic addressable area data state and store.""" from dataclasses import dataclass from functools import cached_property -from typing import Dict, List, Optional, Set, Union +from typing import Dict, List, Optional, Set from opentrons_shared_data.robot.types import RobotType, RobotDefinition from opentrons_shared_data.deck.types import ( @@ -21,8 +21,6 @@ ) from ..resources import deck_configuration_provider from ..types import ( - DeckSlotLocation, - AddressableAreaLocation, AddressableArea, PotentialCutoutFixture, DeckConfigurationType, @@ -237,16 +235,7 @@ def _get_addressable_areas_from_deck_configuration( ) return {area.area_name: area for area in addressable_areas} - def _add_addressable_area( - self, location: Union[DeckSlotLocation, AddressableAreaLocation, str] - ) -> None: - if isinstance(location, DeckSlotLocation): - addressable_area_name = location.slotName.id - elif isinstance(location, AddressableAreaLocation): - addressable_area_name = location.addressableAreaName - else: - addressable_area_name = location - + def _add_addressable_area(self, addressable_area_name: str) -> None: if addressable_area_name not in self._state.loaded_addressable_areas_by_name: cutout_id = self._validate_addressable_area_for_simulation( addressable_area_name From 9d910a860a8bd629f22ad3e958e295d9a4182f49 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 4 Dec 2024 12:59:58 -0500 Subject: [PATCH 14/16] Fix loadModule using the module-provided addressable area instead of the module-occupied one. --- .../protocol_engine/commands/load_module.py | 13 +++++------- .../commands/test_load_module.py | 20 ++++++++++++++----- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/api/src/opentrons/protocol_engine/commands/load_module.py b/api/src/opentrons/protocol_engine/commands/load_module.py index 8e1a8e6489c..f8b88e08814 100644 --- a/api/src/opentrons/protocol_engine/commands/load_module.py +++ b/api/src/opentrons/protocol_engine/commands/load_module.py @@ -130,26 +130,23 @@ async def execute(self, params: LoadModuleParams) -> SuccessData[LoadModuleResul self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( params.location.slotName.id ) - state_update.set_addressable_area_used( - addressable_area_name=params.location.slotName.id - ) else: - addressable_area = ( + addressable_area_provided_by_module = ( self._state_view.modules.ensure_and_convert_module_fixture_location( deck_slot=params.location.slotName, model=params.model, ) ) self._state_view.addressable_areas.raise_if_area_not_in_deck_configuration( - addressable_area - ) - state_update.set_addressable_area_used( - addressable_area_name=addressable_area + addressable_area_provided_by_module ) verified_location = self._state_view.geometry.ensure_location_not_occupied( params.location ) + state_update.set_addressable_area_used( + addressable_area_name=params.location.slotName.id + ) if params.model == ModuleModel.MAGNETIC_BLOCK_V1: loaded_module = await self._equipment.load_magnetic_block( diff --git a/api/tests/opentrons/protocol_engine/commands/test_load_module.py b/api/tests/opentrons/protocol_engine/commands/test_load_module.py index 7444eb160d4..65ee30e7a88 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_load_module.py +++ b/api/tests/opentrons/protocol_engine/commands/test_load_module.py @@ -78,7 +78,7 @@ async def test_load_module_implementation( deck_slot=data.location.slotName, model=data.model, ) - ).then_return(sentinel.addressable_area_name) + ).then_return(sentinel.addressable_area_provided_by_module) decoy.when( await equipment.load_module( @@ -95,6 +95,11 @@ async def test_load_module_implementation( ) result = await subject.execute(data) + decoy.verify( + state_view.addressable_areas.raise_if_area_not_in_deck_configuration( + sentinel.addressable_area_provided_by_module + ) + ) assert result == SuccessData( public=LoadModuleResult( moduleId="module-id", @@ -104,7 +109,7 @@ async def test_load_module_implementation( ), state_update=update_types.StateUpdate( addressable_area_used=update_types.AddressableAreaUsedUpdate( - addressable_area_name=sentinel.addressable_area_name + addressable_area_name=data.location.slotName.id ) ), ) @@ -145,7 +150,7 @@ async def test_load_module_implementation_mag_block( deck_slot=data.location.slotName, model=data.model, ) - ).then_return(sentinel.addressable_area_name) + ).then_return(sentinel.addressable_area_provided_by_module) decoy.when( await equipment.load_magnetic_block( @@ -162,6 +167,11 @@ async def test_load_module_implementation_mag_block( ) result = await subject.execute(data) + decoy.verify( + state_view.addressable_areas.raise_if_area_not_in_deck_configuration( + sentinel.addressable_area_provided_by_module + ) + ) assert result == SuccessData( public=LoadModuleResult( moduleId="module-id", @@ -171,7 +181,7 @@ async def test_load_module_implementation_mag_block( ), state_update=update_types.StateUpdate( addressable_area_used=update_types.AddressableAreaUsedUpdate( - addressable_area_name=sentinel.addressable_area_name + addressable_area_name=data.location.slotName.id ) ), ) @@ -238,7 +248,7 @@ async def test_load_module_implementation_abs_reader( ), state_update=update_types.StateUpdate( addressable_area_used=update_types.AddressableAreaUsedUpdate( - addressable_area_name=sentinel.addressable_area_name + addressable_area_name=data.location.slotName.id ) ), ) From ca4f16fc2bd89b9046f12fd94b606fef42d6d04b Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 4 Dec 2024 13:49:02 -0500 Subject: [PATCH 15/16] The todo is todone. --- api/src/opentrons/protocol_engine/state/addressable_areas.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/addressable_areas.py b/api/src/opentrons/protocol_engine/state/addressable_areas.py index d612456b7bd..16898ccb4ed 100644 --- a/api/src/opentrons/protocol_engine/state/addressable_areas.py +++ b/api/src/opentrons/protocol_engine/state/addressable_areas.py @@ -182,8 +182,6 @@ def __init__( robot_definition=robot_definition, ) - # TODO: Port loadLabware, moveLabware, loadModule, moveToAddressableArea, and moveToAddressableAreaForDropTip - # and their tests def handle_action(self, action: Action) -> None: """Modify state in reaction to an action.""" for state_update in get_state_updates(action): From ee4915184fa7cef42856caece6a256552949653f Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 9 Dec 2024 12:23:27 -0500 Subject: [PATCH 16/16] Delete obsolete test. --- .../state/test_addressable_area_store_old.py | 31 ------------------- 1 file changed, 31 deletions(-) diff --git a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py index e861a006e04..1bbccf96d42 100644 --- a/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_addressable_area_store_old.py @@ -185,37 +185,6 @@ def test_addressable_area_usage_in_simulation( ) -@pytest.mark.parametrize("addressable_area_name", ["A1", "C4"]) -def test_addressable_area_usage( - subject: AddressableAreaStore, - addressable_area_name: str, -) -> None: - """Non-simulating stores should correctly handle `StateUpdate`s with addressable areas. - - todo(mm, 2024-12-02): This is ported from an older test that said the - subject "should check that the addressable area is in the deck config." But - AddressableAreaStore does not do that--that is the job of AddressableAreaView--and - the original test did not cover that. Do we still need to test anything here, or - can this be deleted? - """ - # The addressable area should have been added by the deck configuration. - # (Tested more explicitly elsewhere.) - assert addressable_area_name in subject.state.loaded_addressable_areas_by_name - - subject.handle_action( - SucceedCommandAction( - command=_dummy_command(), - state_update=update_types.StateUpdate( - addressable_area_used=update_types.AddressableAreaUsedUpdate( - addressable_area_name - ) - ), - ) - ) - # The addressable area should still be there after handling the action. - assert addressable_area_name in subject.state.loaded_addressable_areas_by_name - - def test_add_addressable_area_action( simulated_subject: AddressableAreaStore, ) -> None: