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): OT2 fixed trash load fix and API 2,15 support for new trash container structure #14145

Merged
merged 12 commits into from
Dec 8, 2023
Merged
3 changes: 2 additions & 1 deletion api/src/opentrons/protocol_api/core/engine/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,14 @@ def _load_fixed_trash(self) -> None:
)

def append_disposal_location(
self, disposal_location: Union[TrashBin, WasteChute]
self, disposal_location: Union[Labware, TrashBin, WasteChute]
) -> None:
"""Append a disposal location object to the core"""
self._disposal_locations.append(disposal_location)

def get_disposal_locations(self) -> List[Union[Labware, TrashBin, WasteChute]]:
"""Get disposal locations."""

return self._disposal_locations

def get_max_speeds(self) -> AxisMaxSpeeds:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ def __init__(
self._loaded_modules: Set["AbstractModule"] = set()
self._module_cores: List[legacy_module_core.LegacyModuleCore] = []
self._labware_cores: List[LegacyLabwareCore] = [self.fixed_trash]
self._disposal_locations: List[Union[Labware, TrashBin, WasteChute]] = []

@property
def api_version(self) -> APIVersion:
Expand Down Expand Up @@ -133,11 +134,13 @@ def is_simulating(self) -> bool:
return self._sync_hardware.is_simulator # type: ignore[no-any-return]

def append_disposal_location(
self, disposal_location: Union[TrashBin, WasteChute]
self, disposal_location: Union[Labware, TrashBin, WasteChute]
) -> None:
raise APIVersionError(
"Disposal locations are not supported in this API Version."
)
if isinstance(disposal_location, (TrashBin, WasteChute)):
raise APIVersionError(
"Trash Bin and Waste Chute Disposal locations are not supported in this API Version."
)
self._disposal_locations.append(disposal_location)

def add_labware_definition(
self,
Expand Down Expand Up @@ -383,10 +386,7 @@ def get_loaded_instruments(

def get_disposal_locations(self) -> List[Union[Labware, TrashBin, WasteChute]]:
"""Get valid disposal locations."""
trash = self._deck_layout["12"]
if isinstance(trash, Labware):
return [trash]
raise APIVersionError("No dynamically loadable disposal locations.")
return self._disposal_locations

def pause(self, msg: Optional[str]) -> None:
"""Pause the protocol."""
Expand Down
2 changes: 1 addition & 1 deletion api/src/opentrons/protocol_api/core/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def add_labware_definition(

@abstractmethod
def append_disposal_location(
self, disposal_location: Union[TrashBin, WasteChute]
self, disposal_location: Union[Labware, TrashBin, WasteChute]
) -> None:
"""Append a disposal location object to the core"""
...
Expand Down
48 changes: 43 additions & 5 deletions api/src/opentrons/protocol_api/protocol_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@
from opentrons.commands import protocol_commands as cmds, types as cmd_types
from opentrons.commands.publisher import CommandPublisher, publish
from opentrons.protocols.api_support import instrument as instrument_support
from opentrons.protocols.api_support.deck_type import NoTrashDefinedError
from opentrons.protocols.api_support.deck_type import (
NoTrashDefinedError,
should_load_fixed_trash_for_python_protocol,
)
from opentrons.protocols.api_support.types import APIVersion
from opentrons.protocols.api_support.util import (
AxisMaxSpeeds,
Expand Down Expand Up @@ -138,7 +141,26 @@ def __init__(
mount: None for mount in Mount
}
self._bundled_data: Dict[str, bytes] = bundled_data or {}

# With the addition of Moveable Trashes and Waste Chute support, it is not necessary
# to ensure that the list of "disposal locations", essentially the list of trashes,
# is initialized correctly on protocols utilizing former API versions prior to 2.16
# and also to ensure that any protocols after 2.16 intialize a Fixed Trash for OT-2
# protocols so that no load trash bin behavior is required within the protocol itself.
# Protocols prior to 2.16 expect the Fixed Trash to exist as a Labware object, while
# protocols after 2.16 expect trash to exist as either a TrashBin or WasteChute object.
Comment on lines +145 to +151
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this comment be broken up, or shortened, for readability? I'm having trouble parsing it.


self._load_fixed_trash()
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like some trash initialization code already existed in self._load_fixed_trash(). Organizationally, it seems like we should be adding our new code there.

if should_load_fixed_trash_for_python_protocol(self._api_version):
self._core.append_disposal_location(self.fixed_trash)
elif (
self._api_version >= APIVersion(2, 16)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, this self._api_version >= APIVersion(2, 16) check is not doing anything. We can only reach here if self._api_version >= APIVersion(2, 16), because there's a similar version check in the call to should_load_fixed_trash_for_python_protocol(), above.

I see where this is coming from, but I think we should leave this _api_version check out. I'm worried about accidentally leaving a gap between should_load_fixed_trash_for_python_protocol()'s maximum APIVersion, which defines the latest version where the fixed trash is a Labware, and this APIVersion, which defines the earliest version where the fixed trash is a TrashBin. If there's a gap, we'd be left with versions where OT-2 protocols accidentally have no fixed trash.

I would just write this as elif self._core.robot_type == "OT-2 Standard".

and self._core.robot_type == "OT-2 Standard"
):
_fixed_trash_trashbin = TrashBin(
location=DeckSlotName.FIXED_TRASH, addressable_area_name="fixedTrash"
)
self._core.append_disposal_location(_fixed_trash_trashbin)
Comment on lines +154 to +163
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 showing us that we need to rename or rework should_load_fixed_trash_for_python_protocol().

I'm not super familiar with that code, but I'm inferring from this usage that it currently works like "should we load a fixed trash labware," not "should we load any form of fixed trash at all."

So:

  • We call should_load_fixed_trash_for_python_protocol().
    • If it returns yes: Load a fixed trash as a Labware.
    • If it returns no:
      • Do our own check for whether we should load a fixed trash.
        • If yes: Load a fixed trash as a TrashBin.
          • This is what's confusing to me. It reads like we just asked should_load_fixed_trash_for_python_protocol() a question, then ignored its answer and did things our own way anyway.

@jbleon95 Any opinions?


self._commands: List[str] = []
self._unsubscribe_commands: Optional[Callable[[], None]] = None
Expand Down Expand Up @@ -861,10 +883,10 @@ def load_instrument(
log=logger,
)

trash: Optional[Labware]
trash: Optional[Union[Labware, TrashBin]]
try:
trash = self.fixed_trash
except NoTrashDefinedError:
except (NoTrashDefinedError, APIVersionError):
trash = None

instrument = InstrumentContext(
Expand Down Expand Up @@ -1024,17 +1046,33 @@ def deck(self) -> Deck:

@property # type: ignore
@requires_version(2, 0)
def fixed_trash(self) -> Labware:
def fixed_trash(self) -> Union[Labware, TrashBin]:
"""The trash fixed to slot 12 of the robot deck.

It has one well and should be accessed like labware in your protocol.
In API Versions prior to 2.16 it has one well and should be accessed like labware in your protocol.
e.g. ``protocol.fixed_trash['A1']``

In API Version 2.16 and above it returns a Trash fixture for OT-2 Protocols.
"""
if self._api_version >= APIVersion(2, 16):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This newly added section to should provide handling for the issue Max identified and has tests implemented for. The following behavior is to be expected as a result:
v2.16 OT2 Protocol will identify protocol.fixed_trash as a TrashBin object
v2.16 Flex Protocol will raise an error on protocol.fixed_trash, notifying that for the Flex fixed trash does not exist on API version 2.16 and above.

if self._core.robot_type == "OT-3 Standard":
raise APIVersionError(
"Fixed Trash is not supported on Flex protocols in API Version 2.16 and above."
)
Comment on lines +1059 to +1061
Copy link
Contributor

@SyntaxColoring SyntaxColoring Dec 8, 2023

Choose a reason for hiding this comment

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

Let's raise NoTrashDefinedError instead of APIVersionError. Whether protocol.fixed_trash is available is a question of more than just the apiLevel; it has to do with the robot type, too.

Suggested change
raise APIVersionError(
"Fixed Trash is not supported on Flex protocols in API Version 2.16 and above."
)
raise NoTrashDefinedError(
"Fixed Trash is not supported on Flex protocols in API Version 2.16 and above."
)

And we'll have to update the except that's catching it, too.

disposal_locations = self._core.get_disposal_locations()
if len(disposal_locations) == 0:
raise NoTrashDefinedError(
"No trash container has been defined in this protocol."
)
if isinstance(disposal_locations[0], TrashBin):
return disposal_locations[0]

fixed_trash = self._core_map.get(self._core.fixed_trash)
if fixed_trash is None:
raise NoTrashDefinedError(
"No trash container has been defined in this protocol."
)

Comment on lines +1066 to +1075
Copy link
Contributor

@SyntaxColoring SyntaxColoring Dec 8, 2023

Choose a reason for hiding this comment

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

This is a bit confusing to me because it looks like we now have two competing ways of keeping track of the fixed trash: AbstractProtocol.fixed_trash, and AbstractProtocol.get_disposal_locations().

AbstractProtocol.append_disposal_location()/.get_disposal_locations() were really only added specifically to keep track of the user's explicit calls to ProtocolContext.load_waste_chute() and ProtocolContext.load_trash_bin(). They could also keep track of the fixed trash, like you have here, but if we do that, we need to do it consistently.

In other words, I think we need to choose between either of these two strategies:

  1. AbstractProtocol.get_disposal_locations() returns the fixed trash or the user's explicitly-loaded trashes.
    • Then, we would delete self._core.fixed_trash from our codebase.
  2. AbstractProtocol.get_disposal_locations() returns only the user's explicitly-loaded trashes. AbstractProtocol.fixed_trash returns only the Opentrons standard fixed trash, if there is one.
    • Then, when a user accesses ProtocolContext.fixed_trash, we would not do self._core.get_disposal_locations()[0]. Instead, we would do self._core.fixed_trash.

return fixed_trash

def _load_fixed_trash(self) -> None:
Expand Down
1 change: 0 additions & 1 deletion api/src/opentrons/protocol_engine/state/labware.py
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,6 @@ def get_fixed_trash_id(self) -> Optional[str]:
DeckSlotName.SLOT_A3,
}:
return labware.id

return None

def is_fixed_trash(self, labware_id: str) -> bool:
Expand Down
27 changes: 26 additions & 1 deletion api/tests/opentrons/protocol_api/test_protocol_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@
MagneticModuleCore,
MagneticBlockCore,
)
from opentrons.protocols.api_support.deck_type import (
NoTrashDefinedError,
)


@pytest.fixture(autouse=True)
Expand Down Expand Up @@ -78,6 +81,12 @@ def mock_deck(decoy: Decoy) -> Deck:
return decoy.mock(cls=Deck)


@pytest.fixture
def mock_fixed_trash(decoy: Decoy) -> Labware:
"""Get a mock Fixed Trash."""
return decoy.mock(cls=Labware)


@pytest.fixture
def api_version() -> APIVersion:
"""The API version under test."""
Expand All @@ -90,8 +99,11 @@ def subject(
mock_core_map: LoadedCoreMap,
mock_deck: Deck,
api_version: APIVersion,
mock_fixed_trash: Labware,
decoy: Decoy,
) -> ProtocolContext:
"""Get a ProtocolContext test subject with its dependencies mocked out."""
decoy.when(mock_core_map.get(mock_core.fixed_trash)).then_return(mock_fixed_trash)
return ProtocolContext(
api_version=api_version,
core=mock_core,
Expand All @@ -115,7 +127,7 @@ def test_fixed_trash(
trash = trash_captor.value

decoy.when(mock_core_map.get(mock_core.fixed_trash)).then_return(trash)

decoy.when(mock_core.get_disposal_locations()).then_return([trash])
result = subject.fixed_trash

assert result is trash
Expand Down Expand Up @@ -152,6 +164,9 @@ def test_load_instrument(
).then_return(mock_instrument_core)

decoy.when(mock_instrument_core.get_pipette_name()).then_return("Gandalf the Grey")
decoy.when(mock_core.get_disposal_locations()).then_raise(
NoTrashDefinedError("No trash!")
)

result = subject.load_instrument(
instrument_name="Gandalf", mount="shadowfax", tip_racks=mock_tip_racks
Expand Down Expand Up @@ -196,6 +211,9 @@ def test_load_instrument_replace(
)
).then_return(mock_instrument_core)
decoy.when(mock_instrument_core.get_pipette_name()).then_return("Ada Lovelace")
decoy.when(mock_core.get_disposal_locations()).then_raise(
NoTrashDefinedError("No trash!")
)

pipette_1 = subject.load_instrument(instrument_name="ada", mount=Mount.RIGHT)
assert subject.loaded_instruments["right"] is pipette_1
Expand Down Expand Up @@ -229,6 +247,9 @@ def test_96_channel_pipette_always_loads_on_the_left_mount(
mount=Mount.LEFT,
)
).then_return(mock_instrument_core)
decoy.when(mock_core.get_disposal_locations()).then_raise(
NoTrashDefinedError("No trash!")
)

result = subject.load_instrument(
instrument_name="A 96 Channel Name", mount="shadowfax"
Expand Down Expand Up @@ -261,6 +282,10 @@ def test_96_channel_pipette_raises_if_another_pipette_attached(

decoy.when(mock_instrument_core.get_pipette_name()).then_return("ada")

decoy.when(mock_core.get_disposal_locations()).then_raise(
NoTrashDefinedError("No trash!")
)

pipette_1 = subject.load_instrument(instrument_name="ada", mount=Mount.RIGHT)
assert subject.loaded_instruments["right"] is pipette_1

Expand Down
21 changes: 12 additions & 9 deletions api/tests/opentrons/protocol_api_integration/test_trashes.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,6 @@
"2.16",
"OT-2",
protocol_api.TrashBin,
marks=[
pytest.mark.ot3_only, # Simulating a Flex protocol requires a Flex hardware API.
pytest.mark.xfail(
strict=True, reason="https://opentrons.atlassian.net/browse/RSS-417"
),
],
),
pytest.param(
"2.16",
Expand Down Expand Up @@ -58,7 +52,10 @@ def test_fixed_trash_presence(
)

if expected_trash_class is None:
with pytest.raises(Exception, match="No trash container has been defined"):
with pytest.raises(
Exception,
match="Fixed Trash is not supported on Flex protocols in API Version 2.16 and above.",
):
protocol.fixed_trash
with pytest.raises(Exception, match="No trash container has been defined"):
instrument.trash_container
Expand All @@ -75,7 +72,10 @@ def test_trash_search() -> None:
instrument = protocol.load_instrument("flex_1channel_50", mount="left")

# By default, there should be no trash.
with pytest.raises(Exception, match="No trash container has been defined"):
with pytest.raises(
Exception,
match="Fixed Trash is not supported on Flex protocols in API Version 2.16 and above.",
):
protocol.fixed_trash
with pytest.raises(Exception, match="No trash container has been defined"):
instrument.trash_container
Expand All @@ -84,7 +84,10 @@ def test_trash_search() -> None:
loaded_second = protocol.load_trash_bin("B1")

# After loading some trashes, there should still be no protocol.fixed_trash...
with pytest.raises(Exception, match="No trash container has been defined"):
with pytest.raises(
Exception,
match="Fixed Trash is not supported on Flex protocols in API Version 2.16 and above.",
):
protocol.fixed_trash
# ...but instrument.trash_container should automatically update to point to
# the first trash that we loaded.
Expand Down