Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): return machine appropriate deck slot names in PAPIv2 #12595

Merged
merged 16 commits into from
May 5, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
6 changes: 5 additions & 1 deletion .github/workflows/robot-server-lint-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,12 @@ jobs:
run: make -C robot-server setup-ot2
- name: Lint
run: make -C robot-server lint
- name: Test
- if: ${{ matrix.with-ot-hardware == 'false' }}
name: Test without opentrons_hardware
run: make -C robot-server test-cov test_opts="-m 'not ot3_only'"
- if: ${{ matrix.with-ot-hardware == 'true' }}
name: Test with opentrons_hardware
run: make -C robot-server test-cov test_opts="-m 'not ot2_only'"
SyntaxColoring marked this conversation as resolved.
Show resolved Hide resolved
- name: Ensure assets build
run: make -C robot-server sdist wheel
- name: Upload coverage report
Expand Down
7 changes: 7 additions & 0 deletions api/src/opentrons/protocol_api/core/engine/module_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

from opentrons.protocols.api_support.types import APIVersion

from ... import validation
from ..module import (
AbstractModuleCore,
AbstractTemperatureModuleCore,
Expand Down Expand Up @@ -78,6 +79,12 @@ def get_deck_slot(self) -> DeckSlotName:
"""Get the module's deck slot."""
return self._engine_client.state.modules.get_location(self.module_id).slotName

def get_deck_slot_id(self) -> str:
slot_name = self.get_deck_slot()
return validation.ensure_deck_slot_string(
slot_name, robot_type=self._engine_client.state.config.robot_type
)

def get_display_name(self) -> str:
"""Get the module's display name."""
return self._engine_client.state.modules.get_definition(
Expand Down
12 changes: 10 additions & 2 deletions api/src/opentrons/protocol_api/core/engine/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
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
from opentrons_shared_data.robot.dev_types import RobotType

from opentrons.types import DeckSlotName, Location, Mount, MountType, Point
from opentrons.hardware_control import SyncHardwareAPI, SynchronousAdapter
Expand All @@ -27,6 +28,7 @@
from opentrons.protocol_engine.clients import SyncClient as ProtocolEngineClient
from opentrons.protocol_engine.errors import LabwareNotLoadedOnModuleError

from ... import validation
from ..._liquid import Liquid
from ..protocol import AbstractProtocol
from ..labware import LabwareLoadParams
Expand Down Expand Up @@ -73,6 +75,10 @@ def api_version(self) -> APIVersion:
"""Get the api version protocol target."""
return self._api_version

@property
def robot_type(self) -> RobotType:
return self._engine_client.state.config.robot_type

@property
def fixed_trash(self) -> LabwareCore:
"""Get the fixed trash labware."""
Expand Down Expand Up @@ -439,13 +445,15 @@ def define_liquid(

def get_labware_location(
self, labware_core: LabwareCore
) -> Union[DeckSlotName, ModuleCore, None]:
) -> Union[str, ModuleCore, None]:
"""Get labware parent location."""
labware_location = self._engine_client.state.labware.get_location(
labware_core.labware_id
)
if isinstance(labware_location, DeckSlotLocation):
return labware_location.slotName
return validation.ensure_deck_slot_string(
labware_location.slotName, self._engine_client.state.config.robot_type
)
elif isinstance(labware_location, ModuleLocation):
return self._module_cores_by_id.get(labware_location.moduleId)
return None
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ def get_deck_slot(self) -> DeckSlotName:
"""Get the module's deck slot."""
return DeckSlotName.from_primitive(self._geometry.parent) # type: ignore[arg-type]

def get_deck_slot_id(self) -> str:
return str(self.get_deck_slot())

jbleon95 marked this conversation as resolved.
Show resolved Hide resolved
def get_display_name(self) -> str:
"""Get the module's display name."""
return self._geometry.display_name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from opentrons_shared_data.deck.dev_types import DeckDefinitionV3
from opentrons_shared_data.labware.dev_types import LabwareDefinition
from opentrons_shared_data.pipette.dev_types import PipetteNameType
from opentrons_shared_data.robot.dev_types import RobotType

from opentrons.types import DeckSlotName, Location, Mount, Point
from opentrons.equipment_broker import EquipmentBroker
Expand Down Expand Up @@ -90,6 +91,10 @@ def api_version(self) -> APIVersion:
"""Get the API version the protocol is adhering to."""
return self._api_version

@property
def robot_type(self) -> RobotType:
return "OT-2 Standard"

jbleon95 marked this conversation as resolved.
Show resolved Hide resolved
@property
def equipment_broker(self) -> EquipmentBroker[LoadInfo]:
"""A message broker to to publish equipment load events.
Expand Down Expand Up @@ -437,6 +442,6 @@ def define_liquid(

def get_labware_location(
self, labware_core: LegacyLabwareCore
) -> Union[DeckSlotName, legacy_module_core.LegacyModuleCore, None]:
) -> Union[str, legacy_module_core.LegacyModuleCore, None]:
"""Get labware parent location."""
assert False, "get_labware_location only supported on engine core"
4 changes: 4 additions & 0 deletions api/src/opentrons/protocol_api/core/module.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ def get_serial_number(self) -> str:
def get_deck_slot(self) -> DeckSlotName:
"""Get the module's deck slot."""

@abstractmethod
def get_deck_slot_id(self) -> str:
"""Get the module's deck slot in a robot accurate format."""

def get_display_name(self) -> str:
"""Get the module's display name."""

Expand Down
8 changes: 7 additions & 1 deletion api/src/opentrons/protocol_api/core/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from opentrons_shared_data.deck.dev_types import DeckDefinitionV3
from opentrons_shared_data.pipette.dev_types import PipetteNameType
from opentrons_shared_data.labware.dev_types import LabwareDefinition
from opentrons_shared_data.robot.dev_types import RobotType

from opentrons.types import DeckSlotName, Location, Mount, Point
from opentrons.hardware_control import SyncHardwareAPI
Expand All @@ -29,6 +30,11 @@ def fixed_trash(self) -> LabwareCoreType:
"""Get the fixed trash labware core."""
...

@property
@abstractmethod
def robot_type(self) -> RobotType:
...

@abstractmethod
def get_max_speeds(self) -> AxisMaxSpeeds:
...
Expand Down Expand Up @@ -174,5 +180,5 @@ def define_liquid(
@abstractmethod
def get_labware_location(
self, labware_core: LabwareCoreType
) -> Union[DeckSlotName, ModuleCoreType, None]:
) -> Union[str, ModuleCoreType, None]:
"""Get labware parent location."""
2 changes: 1 addition & 1 deletion api/src/opentrons/protocol_api/deck.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def position_for(self, slot: DeckLocation) -> Location:
def get_slot_definition(self, slot: DeckLocation) -> SlotDefV3:
"""Get the geometric definition data of a slot."""
slot_name = _get_slot_name(slot)
return self._slot_definitions_by_name[slot_name.value]
return self._slot_definitions_by_name[slot_name.id]

def get_slot_center(self, slot: DeckLocation) -> Point:
"""Get the absolute coordinates of a slot's center."""
Expand Down
2 changes: 1 addition & 1 deletion api/src/opentrons/protocol_api/labware.py
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ def parent(self) -> Union[str, ModuleTypes, None]:
if isinstance(labware_location, AbstractModuleCore):
return self._core_map.get(labware_location)

return None if labware_location is None else labware_location.id
return labware_location

@property # type: ignore[misc]
@requires_version(2, 0)
Expand Down
2 changes: 1 addition & 1 deletion api/src/opentrons/protocol_api/module_contexts.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ def labware(self) -> Optional[Labware]:
@requires_version(2, 14)
def parent(self) -> str:
"""The name of the slot the module is on."""
return self._core.get_deck_slot().value
return self._core.get_deck_slot_id()

@property # type: ignore[misc]
@requires_version(2, 0)
Expand Down
8 changes: 8 additions & 0 deletions api/src/opentrons/protocol_api/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from typing_extensions import TypeGuard

from opentrons_shared_data.pipette.dev_types import PipetteNameType
from opentrons_shared_data.robot.dev_types import RobotType

from opentrons.types import Mount, DeckSlotName, Location
from opentrons.hardware_control.modules.types import (
Expand Down Expand Up @@ -76,6 +77,13 @@ def ensure_deck_slot(deck_slot: Union[int, str]) -> DeckSlotName:
raise ValueError(f"'{deck_slot}' is not a valid deck slot") from e


def ensure_deck_slot_string(slot_name: DeckSlotName, robot_type: RobotType) -> str:
if robot_type == "OT-2 Standard":
return str(slot_name)
else:
return slot_name.as_coordinate()


def ensure_lowercase_name(name: str) -> str:
"""Ensure that a given name string is all lowercase."""
if not isinstance(name, str):
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
"""Tests for opentrons.protocol_api.core.engine.ModuleCore."""
import pytest
import inspect
from decoy import Decoy

from opentrons.hardware_control import SynchronousAdapter
from opentrons.hardware_control.modules import AbstractModule
from opentrons.protocol_engine import DeckSlotLocation
from opentrons.protocol_engine.clients import SyncClient as EngineClient
from opentrons.protocol_engine.types import ModuleModel, ModuleDefinition
from opentrons.protocol_api import MAX_SUPPORTED_VERSION
from opentrons.protocol_api import MAX_SUPPORTED_VERSION, validation as mock_validation
from opentrons.protocols.api_support.types import APIVersion
from opentrons.protocol_api.core.engine.module_core import ModuleCore
from opentrons.types import DeckSlotName
Expand All @@ -31,6 +32,13 @@ def mock_sync_module_hardware(decoy: Decoy) -> SynchronousAdapter[AbstractModule
return decoy.mock(name="SynchronousAdapter[AbstractModule]") # type: ignore[no-any-return]


@pytest.fixture(autouse=True)
def _mock_validation_module(decoy: Decoy, monkeypatch: pytest.MonkeyPatch) -> None:
"""Mock out opentrons.legacy.validation functions."""
for name, func in inspect.getmembers(mock_validation, inspect.isfunction):
monkeypatch.setattr(mock_validation, name, decoy.mock(func=func))


@pytest.fixture
def subject(
mock_engine_client: EngineClient,
Expand Down Expand Up @@ -63,6 +71,23 @@ def test_get_deck_slot(
assert subject.get_deck_slot() == DeckSlotName.SLOT_1


def test_get_deck_slot_id(
decoy: Decoy, subject: ModuleCore, mock_engine_client: EngineClient
) -> None:
"""It should return the correct deck slot string associated with the module and robot type."""
decoy.when(mock_engine_client.state.modules.get_location("1234")).then_return(
DeckSlotLocation(slotName=DeckSlotName.SLOT_1)
)

decoy.when(mock_engine_client.state.config.robot_type).then_return("OT-3 Standard")

decoy.when(
mock_validation.ensure_deck_slot_string(DeckSlotName.SLOT_1, "OT-3 Standard")
).then_return("foo")

assert subject.get_deck_slot_id() == "foo"


def test_get_model(
decoy: Decoy, subject: ModuleCore, mock_engine_client: EngineClient
) -> None:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
ThermocyclerModuleCore,
HeaterShakerModuleCore,
)
from opentrons.protocol_api import MAX_SUPPORTED_VERSION
from opentrons.protocol_api import validation, MAX_SUPPORTED_VERSION

from opentrons.protocols.api_support.types import APIVersion

Expand All @@ -67,11 +67,18 @@
def patch_mock_load_labware_params(
decoy: Decoy, monkeypatch: pytest.MonkeyPatch
) -> None:
"""Mock out point_calculations.py functions."""
"""Mock out load_labware_params.py functions."""
for name, func in inspect.getmembers(load_labware_params, inspect.isfunction):
monkeypatch.setattr(load_labware_params, name, decoy.mock(func=func))


@pytest.fixture(autouse=True)
def patch_mock_validation(decoy: Decoy, monkeypatch: pytest.MonkeyPatch) -> None:
"""Mock out validation.py functions."""
for name, func in inspect.getmembers(validation, inspect.isfunction):
monkeypatch.setattr(validation, name, decoy.mock(func=func))


@pytest.fixture(autouse=True)
def patch_mock_deck_conflict_check(
decoy: Decoy, monkeypatch: pytest.MonkeyPatch
Expand Down Expand Up @@ -765,3 +772,80 @@ def test_add_liquid(
)

assert result == expected_result


def test_get_labware_location_deck_slot(
decoy: Decoy,
mock_engine_client: EngineClient,
subject: ProtocolCore,
) -> None:
"""It should return the labware location as a deck slot string."""
decoy.when(mock_engine_client.state.labware.get_definition("abc")).then_return(
LabwareDefinition.construct(ordering=[]) # type: ignore[call-arg]
)
decoy.when(mock_engine_client.state.labware.get_location("abc")).then_return(
DeckSlotLocation(slotName=DeckSlotName.SLOT_1)
)
decoy.when(mock_engine_client.state.config.robot_type).then_return("OT-2 Standard")
decoy.when(
validation.ensure_deck_slot_string(DeckSlotName.SLOT_1, "OT-2 Standard")
).then_return("777")

labware = LabwareCore(
labware_id="abc",
engine_client=mock_engine_client,
)

assert subject.get_labware_location(labware) == "777"


def test_get_labware_location_module(
decoy: Decoy,
mock_engine_client: EngineClient,
api_version: APIVersion,
mock_sync_module_hardware: SynchronousAdapter[AbstractModule],
subject: ProtocolCore,
) -> None:
"""It should return the labware location as a module."""
decoy.when(mock_engine_client.state.labware.get_definition("abc")).then_return(
LabwareDefinition.construct(ordering=[]) # type: ignore[call-arg]
)
decoy.when(mock_engine_client.state.labware.get_location("abc")).then_return(
ModuleLocation(moduleId="123")
)

module = ModuleCore(
module_id="module-id",
engine_client=mock_engine_client,
api_version=api_version,
sync_module_hardware=mock_sync_module_hardware,
)
subject._module_cores_by_id["123"] = module

labware = LabwareCore(
labware_id="abc",
engine_client=mock_engine_client,
)

assert subject.get_labware_location(labware) == module


def test_get_labware_location_off_deck(
decoy: Decoy,
mock_engine_client: EngineClient,
subject: ProtocolCore,
) -> None:
"""It should return the labware location as None to represent an off deck labware."""
decoy.when(mock_engine_client.state.labware.get_definition("abc")).then_return(
LabwareDefinition.construct(ordering=[]) # type: ignore[call-arg]
)
decoy.when(mock_engine_client.state.labware.get_location("abc")).then_return(
"offDeck"
)

labware = LabwareCore(
labware_id="abc",
engine_client=mock_engine_client,
)

assert subject.get_labware_location(labware) is None
Loading