Merge branch 'edge' into pd-submerge-aspirate

Author: shiyaochen

api/release-notes-internal.md

@@ -11,6 +11,10 @@ This internal release, pulled from the `edge` branch, contains features being de
 - Python API version bumped to 2.23
 - Added liquid classes and new transfer functions
 
+### Bug Fixes In This Release (list in progress):
+- Fixed `InstrumentContext.name` so that it returns the correct API-specific names of Flex pipettes.
+
+
 ## Internal Release 2.3.0-alpha.2
 
 This internal release, pulled from the `edge` branch, contains features being developed for 8.3.0. It's for internal testing only.

api/src/opentrons/protocol_api/_liquid.py

@@ -1,7 +1,7 @@
 from __future__ import annotations
 
 from dataclasses import dataclass
-from typing import Optional, Dict
+from typing import Optional, Dict, Union, TYPE_CHECKING
 
 from opentrons_shared_data.liquid_classes.liquid_class_definition import (
     LiquidClassSchemaV1,
@@ -12,6 +12,9 @@
     build_transfer_properties,
 )
 
+if TYPE_CHECKING:
+    from . import InstrumentContext, Labware
+
 
 @dataclass(frozen=True)
 class Liquid:
@@ -64,18 +67,42 @@ def name(self) -> str:
     def display_name(self) -> str:
         return self._display_name
 
-    def get_for(self, pipette: str, tiprack: str) -> TransferProperties:
+    def get_for(
+        self, pipette: Union[str, InstrumentContext], tip_rack: Union[str, Labware]
+    ) -> TransferProperties:
         """Get liquid class transfer properties for the specified pipette and tip."""
+        from . import InstrumentContext, Labware
+
+        if isinstance(pipette, InstrumentContext):
+            pipette_name = pipette.name
+        elif isinstance(pipette, str):
+            pipette_name = pipette
+        else:
+            raise ValueError(
+                f"{pipette} should either be an InstrumentContext object"
+                f" or a pipette name string."
+            )
+
+        if isinstance(tip_rack, Labware):
+            tiprack_uri = tip_rack.uri
+        elif isinstance(tip_rack, str):
+            tiprack_uri = tip_rack
+        else:
+            raise ValueError(
+                f"{tip_rack} should either be a tiprack Labware object"
+                f" or a tiprack URI string."
+            )
+
         try:
-            settings_for_pipette = self._by_pipette_setting[pipette]
+            settings_for_pipette = self._by_pipette_setting[pipette_name]
         except KeyError:
             raise ValueError(
-                f"No properties found for {pipette} in {self._name} liquid class"
+                f"No properties found for {pipette_name} in {self._name} liquid class"
             )
         try:
-            transfer_properties = settings_for_pipette[tiprack]
+            transfer_properties = settings_for_pipette[tiprack_uri]
         except KeyError:
             raise ValueError(
-                f"No properties found for {tiprack} in {self._name} liquid class"
+                f"No properties found for {tiprack_uri} in {self._name} liquid class"
             )
         return transfer_properties

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

@@ -42,7 +42,7 @@
 from opentrons.protocol_engine.errors.exceptions import TipNotAttachedError
 from opentrons.protocol_engine.clients import SyncClient as EngineClient
 from opentrons.protocols.api_support.definitions import MAX_SUPPORTED_VERSION
-from opentrons_shared_data.pipette.types import PipetteNameType, PIPETTE_API_NAMES_MAP
+from opentrons_shared_data.pipette.types import PIPETTE_API_NAMES_MAP
 from opentrons_shared_data.errors.exceptions import (
     UnsupportedHardwareCommand,
 )
@@ -62,6 +62,9 @@
 
 _DISPENSE_VOLUME_VALIDATION_ADDED_IN = APIVersion(2, 17)
 
+_FLEX_PIPETTE_NAMES_FIXED_IN = APIVersion(2, 23)
+"""The version after which InstrumentContext.name returns the correct API-specific names of Flex pipettes."""
+
 
 class InstrumentCore(AbstractInstrument[WellCore, LabwareCore]):
     """Instrument API core using a ProtocolEngine.
@@ -721,33 +724,29 @@ def get_pipette_name(self) -> str:
 
         Will match the load name of the actually loaded pipette,
         which may differ from the requested load name.
-        """
-        # TODO (tz, 11-23-22): revert this change when merging
-        # https://opentrons.atlassian.net/browse/RLIQ-251
-        pipette = self._engine_client.state.pipettes.get(self._pipette_id)
-        return (
-            pipette.pipetteName.value
-            if isinstance(pipette.pipetteName, PipetteNameType)
-            else pipette.pipetteName
-        )
 
-    def get_load_name(self) -> str:
-        """Get the pipette's requested API load name.
+        From API v2.15 to v2.22, this property returned an internal, engine-specific,
+         name for Flex pipettes (eg, "p50_multi_flex" instead of "flex_8channel_50").
 
-        This is the load name that is specified in the `ProtocolContext.load_instrument()`
-        method. This name might differ from the engine-specific pipette name.
+        From API v2.23 onwards, this behavior is fixed so that this property returns
+        the API-specific names of Flex pipettes.
         """
+        # TODO (tz, 11-23-22): revert this change when merging
+        # https://opentrons.atlassian.net/browse/RLIQ-251
         pipette = self._engine_client.state.pipettes.get(self._pipette_id)
-        load_name = next(
-            (
-                pip_api_name
-                for pip_api_name, pip_name in PIPETTE_API_NAMES_MAP.items()
-                if pip_name == pipette.pipetteName
-            ),
-            None,
-        )
-        assert load_name, "Load name not found."
-        return load_name
+        if self._protocol_core.api_version < _FLEX_PIPETTE_NAMES_FIXED_IN:
+            return pipette.pipetteName.value
+        else:
+            name = next(
+                (
+                    pip_api_name
+                    for pip_api_name, pip_name in PIPETTE_API_NAMES_MAP.items()
+                    if pip_name == pipette.pipetteName
+                ),
+                None,
+            )
+            assert name, "Pipette name not found."
+            return name
 
     def get_model(self) -> str:
         return self._engine_client.state.pipettes.get_model_name(self._pipette_id)
@@ -932,7 +931,7 @@ def load_liquid_class(
         """
         liquid_class_record = LiquidClassRecord(
             liquidClassName=name,
-            pipetteModel=self.get_load_name(),
+            pipetteModel=self.get_pipette_name(),
             tiprack=tiprack_uri,
             aspirate=transfer_properties.aspirate.as_shared_data_model(),
             singleDispense=transfer_properties.dispense.as_shared_data_model(),
@@ -994,8 +993,7 @@ def transfer_liquid(  # noqa: C901
             )
         tiprack_uri_for_transfer_props = tip_racks[0][1].get_uri()
         transfer_props = liquid_class.get_for(
-            pipette=self.get_load_name(),
-            tiprack=tiprack_uri_for_transfer_props,
+            pipette=self.get_pipette_name(), tip_rack=tiprack_uri_for_transfer_props
         )
         # TODO: use the ID returned by load_liquid_class in command annotations
         self.load_liquid_class(
@@ -1008,7 +1006,12 @@ def transfer_liquid(  # noqa: C901
         source_dest_per_volume_step = tx_commons.expand_for_volume_constraints(
             volumes=[volume for _ in range(len(source))],
             targets=zip(source, dest),
-            max_volume=self.get_max_volume(),
+            max_volume=min(
+                self.get_max_volume(),
+                tip_racks[0][1]
+                .get_well_core("A1")
+                .get_max_volume(),  # Assuming all tips in tiprack are of same volume
+            ),
         )
 
         def _drop_tip() -> None:
@@ -1175,10 +1178,11 @@ def aspirate_liquid_class(
         Return: List of liquid and air gap pairs in tip.
         """
         aspirate_props = transfer_properties.aspirate
+        # TODO (spp, 2025-01-30): check if check_valid_volume_parameters is necessary and is enough.
         tx_commons.check_valid_volume_parameters(
             disposal_volume=0,  # No disposal volume for 1-to-1 transfer
             air_gap=aspirate_props.retract.air_gap_by_volume.get_for_volume(volume),
-            max_volume=self.get_max_volume(),
+            max_volume=self.get_working_volume(),
         )
         source_loc, source_well = source
         aspirate_point = (

api/src/opentrons/protocol_api/instrument_context.py

@@ -64,6 +64,7 @@
 _AIR_GAP_TRACKING_ADDED_IN = APIVersion(2, 22)
 """The version after which air gaps should be implemented with a separate call instead of an aspirate for better liquid volume tracking."""
 
+
 AdvancedLiquidHandling = v1_transfer.AdvancedLiquidHandling
 
 
@@ -2004,6 +2005,10 @@ def trash_container(
     def name(self) -> str:
         """
         The name string for the pipette (e.g., ``"p300_single"``).
+
+        From API v2.15 to v2.22, this property returned an internal name for Flex pipettes.
+        From API v2.23 onwards, this behavior is fixed so that this property returns
+        the Python Protocol API load names of Flex pipettes.
         """
         return self._core.get_pipette_name()
 

api/src/opentrons/protocols/api_support/instrument.py

@@ -99,7 +99,20 @@ def validate_tiprack(
         gen_lookup = (
             "FLEX" if ("flex" in instr_metadata or "96" in instr_metadata) else "OT2"
         )
-        valid_vols = VALID_PIP_TIPRACK_VOL[gen_lookup][instrument_name.split("_")[0]]
+
+        # TODO (spp, 2025-01-30): do what AA's note above says or at least,
+        #  fetch the 'pip_type' below from the 'model' field in pipette definitions
+        #  so that we don't have to figure it out from pipette names
+        if instrument_name.split("_")[0] == "flex":
+            # Flex's API load names have the format 'flex_1channel_1000'
+            # From API v2.23 on, this is the name returned by InstrumentContext.name
+            pip_type = "p" + instrument_name.split("_")[2]
+        else:
+            # Until API v2.23, InstrumentContext.name returned the engine-specific names
+            # of Flex pipettes. These names, as well as OT2 pipette names,
+            # have the format- 'p1000_single_gen2' or 'p1000_single_flex'
+            pip_type = instrument_name.split("_")[0]
+        valid_vols = VALID_PIP_TIPRACK_VOL[gen_lookup][pip_type]
         if tiprack_vol not in valid_vols:
             log.warning(
                 f"The pipette {instrument_name} and its tip rack {tip_rack.load_name}"

api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py

@@ -187,32 +187,60 @@ def test_pipette_id(subject: InstrumentCore) -> None:
     assert subject.pipette_id == "abc123"
 
 
-def test_get_pipette_name(
-    decoy: Decoy, mock_engine_client: EngineClient, subject: InstrumentCore
+@pytest.mark.parametrize(
+    "version",
+    [
+        APIVersion(2, 15),
+        APIVersion(2, 17),
+        APIVersion(2, 20),
+        APIVersion(2, 22),
+    ],
+)
+def test_get_pipette_name_old(
+    decoy: Decoy,
+    mock_engine_client: EngineClient,
+    mock_protocol_core: ProtocolCore,
+    subject: InstrumentCore,
+    version: APIVersion,
 ) -> None:
     """It should get the pipette's load name."""
+    decoy.when(mock_protocol_core.api_version).then_return(version)
     decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return(
         LoadedPipette.model_construct(pipetteName=PipetteNameType.P300_SINGLE)  # type: ignore[call-arg]
     )
-
-    result = subject.get_pipette_name()
-
-    assert result == "p300_single"
+    assert subject.get_pipette_name() == "p300_single"
+    decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return(
+        LoadedPipette.model_construct(pipetteName=PipetteNameType.P1000_96)  # type: ignore[call-arg]
+    )
+    assert subject.get_pipette_name() == "p1000_96"
+    decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return(
+        LoadedPipette.model_construct(pipetteName=PipetteNameType.P50_SINGLE_FLEX)  # type: ignore[call-arg]
+    )
+    assert subject.get_pipette_name() == "p50_single_flex"
 
 
-def test_get_pipette_load_name(
-    decoy: Decoy, mock_engine_client: EngineClient, subject: InstrumentCore
+@pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 23)))
+def test_get_pipette_name_new(
+    decoy: Decoy,
+    mock_engine_client: EngineClient,
+    mock_protocol_core: ProtocolCore,
+    subject: InstrumentCore,
+    version: APIVersion,
 ) -> None:
     """It should get the pipette's API-specific load name."""
+    decoy.when(mock_protocol_core.api_version).then_return(version)
     decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return(
         LoadedPipette.model_construct(pipetteName=PipetteNameType.P300_SINGLE)  # type: ignore[call-arg]
     )
-    assert subject.get_load_name() == "p300_single"
-
+    assert subject.get_pipette_name() == "p300_single"
     decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return(
         LoadedPipette.model_construct(pipetteName=PipetteNameType.P1000_96)  # type: ignore[call-arg]
     )
-    assert subject.get_load_name() == "flex_96channel_1000"
+    assert subject.get_pipette_name() == "flex_96channel_1000"
+    decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return(
+        LoadedPipette.model_construct(pipetteName=PipetteNameType.P50_SINGLE_FLEX)  # type: ignore[call-arg]
+    )
+    assert subject.get_pipette_name() == "flex_1channel_50"
 
 
 def test_get_mount(
@@ -1671,11 +1699,14 @@ def test_liquid_probe_with_recovery(
     )
 
 
+@pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 23)))
 def test_load_liquid_class(
     decoy: Decoy,
     mock_engine_client: EngineClient,
+    mock_protocol_core: ProtocolCore,
     subject: InstrumentCore,
     minimal_liquid_class_def2: LiquidClassSchemaV1,
+    version: APIVersion,
 ) -> None:
     """It should send the load liquid class command to the engine."""
     sample_aspirate_data = minimal_liquid_class_def2.byPipette[0].byTipType[0].aspirate
@@ -1686,6 +1717,7 @@ def test_load_liquid_class(
         minimal_liquid_class_def2.byPipette[0].byTipType[0].multiDispense
     )
 
+    decoy.when(mock_protocol_core.api_version).then_return(version)
     test_liq_class = decoy.mock(cls=LiquidClass)
     test_transfer_props = decoy.mock(cls=TransferProperties)
 
@@ -1786,6 +1818,42 @@ def test_aspirate_liquid_class(
     assert result == [LiquidAndAirGapPair(air_gap=222, liquid=111)]
 
 
+def test_aspirate_liquid_class_raises_for_more_than_max_volume(
+    decoy: Decoy,
+    mock_engine_client: EngineClient,
+    subject: InstrumentCore,
+    minimal_liquid_class_def2: LiquidClassSchemaV1,
+    mock_transfer_components_executor: TransferComponentsExecutor,
+) -> None:
+    """It should call aspirate sub-steps execution based on liquid class."""
+    source_well = decoy.mock(cls=WellCore)
+    source_location = Location(Point(1, 2, 3), labware=None)
+    test_liquid_class = LiquidClass.create(minimal_liquid_class_def2)
+    test_transfer_properties = test_liquid_class.get_for(
+        "flex_1channel_50", "opentrons_flex_96_tiprack_50ul"
+    )
+    decoy.when(
+        mock_engine_client.state.pipettes.get_working_volume("abc123")
+    ).then_return(100)
+    decoy.when(
+        tx_commons.check_valid_volume_parameters(
+            disposal_volume=0,
+            air_gap=test_transfer_properties.aspirate.retract.air_gap_by_volume.get_for_volume(
+                123
+            ),
+            max_volume=100,
+        )
+    ).then_raise(ValueError("Oh oh!"))
+    with pytest.raises(ValueError, match="Oh oh!"):
+        subject.aspirate_liquid_class(
+            volume=123,
+            source=(source_location, source_well),
+            transfer_properties=test_transfer_properties,
+            transfer_type=TransferType.ONE_TO_ONE,
+            tip_contents=[],
+        )
+
+
 def test_dispense_liquid_class(
     decoy: Decoy,
     mock_engine_client: EngineClient,

api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py

@@ -36,7 +36,7 @@ def sample_transfer_props(
 ) -> TransferProperties:
     """Return a mocked out liquid class fixture."""
     return LiquidClass.create(maximal_liquid_class_def).get_for(
-        pipette="flex_1channel_50", tiprack="opentrons_flex_96_tiprack_50ul"
+        pipette="flex_1channel_50", tip_rack="opentrons_flex_96_tiprack_50ul"
     )