From 3c2379302a3d4c4f24c2e342a22029866a2e68db Mon Sep 17 00:00:00 2001 From: Sanniti Pimpley Date: Thu, 30 Jan 2025 17:18:39 -0500 Subject: [PATCH] feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() (#17289) Closes AUTH-1295, AUTH-1299 # Overview - fixes `InstrumentContext.name` to fetch the python API load name of flex pipettes - updates `LiquidClass.get_for()` to accept the a loaded instrument object and loaded tiprack object for `pipette` and `tip_rack` args respectively. - changes the tiprack argument name of `get_for()` from `tiprack` to `tip_rack` to be consistent with API naming conventions. ## Risk assessment None --------- Co-authored-by: Max Marrone --- api/release-notes-internal.md | 4 ++ api/src/opentrons/protocol_api/_liquid.py | 39 +++++++++++--- .../protocol_api/core/engine/instrument.py | 52 +++++++++--------- .../protocol_api/instrument_context.py | 5 ++ .../protocols/api_support/instrument.py | 15 +++++- .../core/engine/test_instrument_core.py | 54 +++++++++++++++---- .../test_transfer_components_executor.py | 2 +- .../protocol_api/test_liquid_class.py | 12 +++++ .../protocol_api/test_protocol_context.py | 1 + .../test_liquid_classes.py | 7 +-- 10 files changed, 140 insertions(+), 51 deletions(-) diff --git a/api/release-notes-internal.md b/api/release-notes-internal.md index 7fb93059e15..1186b510eb6 100644 --- a/api/release-notes-internal.md +++ b/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. diff --git a/api/src/opentrons/protocol_api/_liquid.py b/api/src/opentrons/protocol_api/_liquid.py index 12c9a140ce3..fa979428eb2 100644 --- a/api/src/opentrons/protocol_api/_liquid.py +++ b/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 diff --git a/api/src/opentrons/protocol_api/core/engine/instrument.py b/api/src/opentrons/protocol_api/core/engine/instrument.py index d9f0cbdd1d4..d1898e5ccaa 100644 --- a/api/src/opentrons/protocol_api/core/engine/instrument.py +++ b/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( diff --git a/api/src/opentrons/protocol_api/instrument_context.py b/api/src/opentrons/protocol_api/instrument_context.py index 6bd93f27970..992aa68c785 100644 --- a/api/src/opentrons/protocol_api/instrument_context.py +++ b/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() diff --git a/api/src/opentrons/protocols/api_support/instrument.py b/api/src/opentrons/protocols/api_support/instrument.py index 3299b8512f9..35625100645 100644 --- a/api/src/opentrons/protocols/api_support/instrument.py +++ b/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}" diff --git a/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py b/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py index 12e658ab7ae..c7e5fa904e0 100644 --- a/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py +++ b/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) diff --git a/api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py b/api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py index 4dadf5b503b..b58b873523c 100644 --- a/api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py +++ b/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" ) diff --git a/api/tests/opentrons/protocol_api/test_liquid_class.py b/api/tests/opentrons/protocol_api/test_liquid_class.py index 7118080eda0..47404c2b480 100644 --- a/api/tests/opentrons/protocol_api/test_liquid_class.py +++ b/api/tests/opentrons/protocol_api/test_liquid_class.py @@ -1,10 +1,12 @@ """Tests for LiquidClass methods.""" import pytest +from decoy import Decoy from opentrons_shared_data.liquid_classes.liquid_class_definition import ( LiquidClassSchemaV1, ) from opentrons.protocol_api import LiquidClass +from opentrons.protocol_api import InstrumentContext, Labware def test_create_liquid_class( @@ -17,6 +19,7 @@ def test_create_liquid_class( def test_get_for_pipette_and_tip( + decoy: Decoy, minimal_liquid_class_def2: LiquidClassSchemaV1, ) -> None: """It should get the properties for the specified pipette and tip.""" @@ -26,6 +29,15 @@ def test_get_for_pipette_and_tip( 10.0: 40.0, 20.0: 30.0, } + mock_instrument = decoy.mock(cls=InstrumentContext) + mock_tiprack = decoy.mock(cls=Labware) + decoy.when(mock_instrument.name).then_return("flex_1channel_50") + decoy.when(mock_tiprack.uri).then_return("opentrons_flex_96_tiprack_50ul") + result_2 = liq_class.get_for(mock_instrument, mock_tiprack) + assert result_2.aspirate.flow_rate_by_volume.as_dict() == { + 10.0: 40.0, + 20.0: 30.0, + } def test_get_for_raises_for_incorrect_pipette_or_tip( diff --git a/api/tests/opentrons/protocol_api/test_protocol_context.py b/api/tests/opentrons/protocol_api/test_protocol_context.py index ebe6734a539..4536003094a 100644 --- a/api/tests/opentrons/protocol_api/test_protocol_context.py +++ b/api/tests/opentrons/protocol_api/test_protocol_context.py @@ -283,6 +283,7 @@ 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_instrument_core.get_model()).then_return("wizard") decoy.when(mock_core.get_disposal_locations()).then_raise( NoTrashDefinedError("No trash!") ) diff --git a/api/tests/opentrons/protocol_api_integration/test_liquid_classes.py b/api/tests/opentrons/protocol_api_integration/test_liquid_classes.py index 83b53f01e1a..cf7791a271a 100644 --- a/api/tests/opentrons/protocol_api_integration/test_liquid_classes.py +++ b/api/tests/opentrons/protocol_api_integration/test_liquid_classes.py @@ -13,7 +13,7 @@ def test_liquid_class_creation_and_property_fetching( ) -> None: """It should create the liquid class and provide access to its properties.""" pipette_load_name = "flex_8channel_50" - simulated_protocol_context.load_instrument(pipette_load_name, mount="left") + p50 = simulated_protocol_context.load_instrument(pipette_load_name, mount="left") tiprack = simulated_protocol_context.load_labware( "opentrons_flex_96_tiprack_50ul", "D1" ) @@ -24,10 +24,7 @@ def test_liquid_class_creation_and_property_fetching( # TODO (spp, 2024-10-17): update this to fetch pipette load name from instrument context assert ( - water.get_for( - pipette_load_name, tiprack.uri - ).dispense.flow_rate_by_volume.get_for_volume(1) - == 50 + water.get_for(p50, tiprack).dispense.flow_rate_by_volume.get_for_volume(1) == 50 ) assert water.get_for(pipette_load_name, tiprack.uri).aspirate.submerge.speed == 100