From 8e6bb69611cc7f26d92b04118e4c77ca6ce58e0a Mon Sep 17 00:00:00 2001 From: aaron-kulkarni Date: Wed, 3 Jul 2024 15:18:58 -0400 Subject: [PATCH 1/5] refactor(api): edit 3 instr context methods for liquid probe Edit detect_liquid_presence, require_liquid_presence, and measure_liquid_height as per the standup today. --- .../protocol_api/core/engine/instrument.py | 41 +++++++++++-------- .../opentrons/protocol_api/core/instrument.py | 7 +++- .../core/legacy/legacy_instrument_core.py | 12 +++++- .../legacy_instrument_core.py | 12 +++++- .../protocol_api/instrument_context.py | 23 ++++++----- .../protocol_engine/clients/sync_client.py | 20 --------- .../core/engine/test_instrument_core.py | 24 ++++++++--- .../protocol_api/test_instrument_context.py | 8 ++-- 8 files changed, 86 insertions(+), 61 deletions(-) diff --git a/api/src/opentrons/protocol_api/core/engine/instrument.py b/api/src/opentrons/protocol_api/core/engine/instrument.py index f39f315366c..af28682ac19 100644 --- a/api/src/opentrons/protocol_api/core/engine/instrument.py +++ b/api/src/opentrons/protocol_api/core/engine/instrument.py @@ -846,30 +846,37 @@ def retract(self) -> None: z_axis = self._engine_client.state.pipettes.get_z_axis(self._pipette_id) self._engine_client.execute_command(cmd.HomeParams(axes=[z_axis])) - def find_liquid_level(self, well_core: WellCore, error_recovery: bool) -> float: + def find_liquid_presence_with_recovery(self, well_core: WellCore) -> None: labware_id = well_core.labware_id well_name = well_core.get_name() well_location = WellLocation( origin=WellOrigin.TOP, offset=WellOffset(x=0, y=0, z=0) ) - if error_recovery: - result = self._engine_client.execute_command_with_result( - cmd.LiquidProbeParams( - labwareId=labware_id, - wellName=well_name, - wellLocation=well_location, - pipetteId=self.pipette_id, - ) + + self._engine_client.execute_command( + cmd.LiquidProbeParams( + labwareId=labware_id, + wellName=well_name, + wellLocation=well_location, + pipetteId=self.pipette_id, ) - else: - result = self._engine_client.execute_command_without_recovery( - cmd.LiquidProbeParams( - labwareId=labware_id, - wellName=well_name, - wellLocation=well_location, - pipetteId=self.pipette_id, - ) + ) + + def find_liquid_level_without_recovery(self, well_core: WellCore) -> float: + labware_id = well_core.labware_id + well_name = well_core.get_name() + well_location = WellLocation( + origin=WellOrigin.TOP, offset=WellOffset(x=0, y=0, z=0) + ) + + result = self._engine_client.execute_command_without_recovery( + cmd.LiquidProbeParams( + labwareId=labware_id, + wellName=well_name, + wellLocation=well_location, + pipetteId=self.pipette_id, ) + ) if result is not None and isinstance(result, LiquidProbeResult): return result.z_position diff --git a/api/src/opentrons/protocol_api/core/instrument.py b/api/src/opentrons/protocol_api/core/instrument.py index 77967023714..4894f0edb8b 100644 --- a/api/src/opentrons/protocol_api/core/instrument.py +++ b/api/src/opentrons/protocol_api/core/instrument.py @@ -304,7 +304,12 @@ def retract(self) -> None: ... @abstractmethod - def find_liquid_level(self, well_core: WellCoreType, error_recovery: bool) -> float: + def find_liquid_presence_with_recovery(self, well_core: WellCoreType) -> None: + """Do a liquid probe to detect the presence of liquid in the well.""" + ... + + @abstractmethod + def find_liquid_level_without_recovery(self, well_core: WellCoreType) -> float: """Do a liquid probe to find the level of the liquid in the well.""" ... diff --git a/api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py b/api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py index 2e2aeba3ed9..bc3b1c0770a 100644 --- a/api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py +++ b/api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py @@ -571,6 +571,14 @@ def retract(self) -> None: """Retract this instrument to the top of the gantry.""" self._protocol_interface.get_hardware.retract(self._mount) # type: ignore [attr-defined] - def find_liquid_level(self, well_core: WellCore, error_recovery: bool) -> float: + def find_liquid_presence_with_recovery(self, well_core: WellCore) -> None: """This will never be called because it was added in API 2.20.""" - assert False, "find_liquid_level only supported in API 2.20 & later" + assert ( + False + ), "find_liquid_presence_with_recovery only supported in API 2.20 & later" + + def find_liquid_level_without_recovery(self, well_core: WellCore) -> float: + """This will never be called because it was added in API 2.20.""" + assert ( + False + ), "find_liquid_level_without_recovery only supported in API 2.20 & later" diff --git a/api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py b/api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py index 27b964b4d61..71482fac30e 100644 --- a/api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py +++ b/api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py @@ -489,6 +489,14 @@ def retract(self) -> None: """Retract this instrument to the top of the gantry.""" self._protocol_interface.get_hardware.retract(self._mount) # type: ignore [attr-defined] - def find_liquid_level(self, well_core: WellCore, error_recovery: bool) -> float: + def find_liquid_presence_with_recovery(self, well_core: WellCore) -> None: """This will never be called because it was added in API 2.20.""" - assert False, "find_liquid_level only supported in API 2.20 & later" + assert ( + False + ), "find_liquid_presence_with_recovery only supported in API 2.20 & later" + + def find_liquid_level_without_recovery(self, well_core: WellCore) -> float: + """This will never be called because it was added in API 2.20.""" + assert ( + False + ), "find_liquid_level_without_recovery only supported in API 2.20 & later" diff --git a/api/src/opentrons/protocol_api/instrument_context.py b/api/src/opentrons/protocol_api/instrument_context.py index 4b77340bc7e..43810a9755c 100644 --- a/api/src/opentrons/protocol_api/instrument_context.py +++ b/api/src/opentrons/protocol_api/instrument_context.py @@ -2,6 +2,7 @@ import logging from contextlib import ExitStack from typing import Any, List, Optional, Sequence, Union, cast, Dict +from opentrons.protocol_engine.errors.error_occurrence import ProtocolCommandFailedError from opentrons_shared_data.errors.exceptions import ( CommandPreconditionViolated, CommandParameterLimitViolated, @@ -2057,14 +2058,14 @@ def detect_liquid_presence(self, well: labware.Well) -> bool: if not isinstance(well, labware.Well): raise WellDoesNotExistError("You must provide a valid well to check.") try: - height = self._core.find_liquid_level( - well_core=well._core, error_recovery=False - ) - if height > 0: - return True - return False # it should never get here + self._core.find_liquid_level_without_recovery(well._core) + return True except PipetteLiquidNotFoundError: return False + except ProtocolCommandFailedError as e: + if isinstance(e.original_error, PipetteLiquidNotFoundError): + return False + raise e except Exception as e: raise e @@ -2077,18 +2078,20 @@ def require_liquid_presence(self, well: labware.Well) -> None: if not isinstance(well, labware.Well): raise WellDoesNotExistError("You must provide a valid well to check.") - self._core.find_liquid_level(well_core=well._core, error_recovery=True) + self._core.find_liquid_presence_with_recovery(well._core) @requires_version(2, 20) def measure_liquid_height(self, well: labware.Well) -> float: """Check the height of the liquid within a well. :returns: The height, in mm, of the liquid from the deck. + + :meta private: + + This is intended for Opentrons internal use only and is not a guaranteed API. """ if not isinstance(well, labware.Well): raise WellDoesNotExistError("You must provide a valid well to check.") - height = self._core.find_liquid_level( - well_core=well._core, error_recovery=False - ) + height = self._core.find_liquid_level_without_recovery(well._core) return float(height) diff --git a/api/src/opentrons/protocol_engine/clients/sync_client.py b/api/src/opentrons/protocol_engine/clients/sync_client.py index 3eed8e2d263..40cac55b555 100644 --- a/api/src/opentrons/protocol_engine/clients/sync_client.py +++ b/api/src/opentrons/protocol_engine/clients/sync_client.py @@ -96,26 +96,6 @@ def execute_command_without_recovery( create_request = CreateType(params=cast(Any, params)) return self._transport.execute_command(create_request) - def execute_command_with_result( - self, params: commands.CommandParams - ) -> Optional[commands.CommandResult]: - """Execute a ProtocolEngine command, including error recovery, and return a result. - - See `ChildThreadTransport.execute_command_wait_for_recovery()` for exact - behavior. - """ - CreateType = CREATE_TYPES_BY_PARAMS_TYPE[type(params)] - create_request = CreateType(params=cast(Any, params)) - result = self._transport.execute_command_wait_for_recovery(create_request) - if result.error is None: - return result.result - if isinstance(result.error, BaseException): # necessary to pass lint - raise result.error - raise ProtocolCommandFailedError( - original_error=result.error, - message=f"{result.error.errorType}: {result.error.detail}", - ) - @property def state(self) -> StateView: """Get a view of the engine's state.""" 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 a38ffc0a234..d9ceabb0260 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 @@ -1292,7 +1292,7 @@ def test_configure_for_volume_post_219( @pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 20))) -def test_find_liquid_level( +def test_find_liquid_level_without_recovery( decoy: Decoy, mock_engine_client: EngineClient, mock_protocol_core: ProtocolCore, @@ -1304,11 +1304,11 @@ def test_find_liquid_level( name="my cool well", labware_id="123abc", engine_client=mock_engine_client ) try: - subject.find_liquid_level(well_core=well_core, error_recovery=True) + subject.find_liquid_level_without_recovery(well_core=well_core) except PipetteLiquidNotFoundError: assert True decoy.verify( - mock_engine_client.execute_command_with_result( + mock_engine_client.execute_command_without_recovery( cmd.LiquidProbeParams( pipetteId=subject.pipette_id, wellLocation=WellLocation( @@ -1319,12 +1319,26 @@ def test_find_liquid_level( ) ) ) + + +@pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 20))) +def test_find_liquid_presence_with_recovery( + decoy: Decoy, + mock_engine_client: EngineClient, + mock_protocol_core: ProtocolCore, + subject: InstrumentCore, + version: APIVersion, +) -> None: + """It should raise an exception on an empty well.""" + well_core = WellCore( + name="my cool well", labware_id="123abc", engine_client=mock_engine_client + ) try: - subject.find_liquid_level(well_core=well_core, error_recovery=False) + subject.find_liquid_presence_with_recovery(well_core=well_core) except PipetteLiquidNotFoundError: assert True decoy.verify( - mock_engine_client.execute_command_without_recovery( + mock_engine_client.execute_command( cmd.LiquidProbeParams( pipetteId=subject.pipette_id, wellLocation=WellLocation( diff --git a/api/tests/opentrons/protocol_api/test_instrument_context.py b/api/tests/opentrons/protocol_api/test_instrument_context.py index ab5d099460e..433f04523e5 100644 --- a/api/tests/opentrons/protocol_api/test_instrument_context.py +++ b/api/tests/opentrons/protocol_api/test_instrument_context.py @@ -1280,7 +1280,7 @@ def test_detect_liquid_presence( """It should only return booleans. Not raise an exception.""" mock_well = decoy.mock(cls=Well) decoy.when( - mock_instrument_core.find_liquid_level(mock_well._core, False) + mock_instrument_core.find_liquid_level_without_recovery(mock_well._core) ).then_raise(PipetteLiquidNotFoundError()) result = subject.detect_liquid_presence(mock_well) assert isinstance(result, bool) @@ -1295,10 +1295,10 @@ def test_require_liquid_presence( ) -> None: """It should raise an exception when called.""" mock_well = decoy.mock(cls=Well) - decoy.when(mock_instrument_core.find_liquid_level(mock_well._core, True)) + decoy.when(mock_instrument_core.find_liquid_presence_with_recovery(mock_well._core)) subject.require_liquid_presence(mock_well) decoy.when( - mock_instrument_core.find_liquid_level(mock_well._core, True) + mock_instrument_core.find_liquid_presence_with_recovery(mock_well._core) ).then_raise(PipetteLiquidNotFoundError()) with pytest.raises(PipetteLiquidNotFoundError): subject.require_liquid_presence(mock_well) @@ -1314,7 +1314,7 @@ def test_measure_liquid_height( """It should raise an exception when called.""" mock_well = decoy.mock(cls=Well) decoy.when( - mock_instrument_core.find_liquid_level(mock_well._core, False) + mock_instrument_core.find_liquid_level_without_recovery(mock_well._core) ).then_raise(PipetteLiquidNotFoundError()) with pytest.raises(PipetteLiquidNotFoundError): subject.measure_liquid_height(mock_well) From 9a55c4f91e29f1c5db53660b8ea96a28e8464555 Mon Sep 17 00:00:00 2001 From: aaron-kulkarni Date: Wed, 3 Jul 2024 15:24:53 -0400 Subject: [PATCH 2/5] lint --- api/src/opentrons/protocol_engine/clients/sync_client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api/src/opentrons/protocol_engine/clients/sync_client.py b/api/src/opentrons/protocol_engine/clients/sync_client.py index 40cac55b555..f772d81cea8 100644 --- a/api/src/opentrons/protocol_engine/clients/sync_client.py +++ b/api/src/opentrons/protocol_engine/clients/sync_client.py @@ -2,7 +2,6 @@ from typing import cast, Any, Optional, overload -from opentrons.protocol_engine.errors.error_occurrence import ProtocolCommandFailedError from opentrons_shared_data.labware.dev_types import LabwareUri from opentrons_shared_data.labware.labware_definition import LabwareDefinition From 7abff6d071be45353168cdea6a0cd812131c9d16 Mon Sep 17 00:00:00 2001 From: aaron-kulkarni Date: Fri, 5 Jul 2024 09:53:30 -0400 Subject: [PATCH 3/5] requested edits --- .../protocol_api/core/engine/instrument.py | 4 +- .../opentrons/protocol_api/core/instrument.py | 4 +- .../core/legacy/legacy_instrument_core.py | 12 ++---- .../legacy_instrument_core.py | 12 ++---- .../protocol_api/instrument_context.py | 19 ++++---- .../core/engine/test_instrument_core.py | 8 ++-- .../protocol_api/test_instrument_context.py | 43 ++++++++++++++----- 7 files changed, 57 insertions(+), 45 deletions(-) diff --git a/api/src/opentrons/protocol_api/core/engine/instrument.py b/api/src/opentrons/protocol_api/core/engine/instrument.py index af28682ac19..c09a6b19136 100644 --- a/api/src/opentrons/protocol_api/core/engine/instrument.py +++ b/api/src/opentrons/protocol_api/core/engine/instrument.py @@ -846,7 +846,7 @@ def retract(self) -> None: z_axis = self._engine_client.state.pipettes.get_z_axis(self._pipette_id) self._engine_client.execute_command(cmd.HomeParams(axes=[z_axis])) - def find_liquid_presence_with_recovery(self, well_core: WellCore) -> None: + def liquid_probe_with_recovery(self, well_core: WellCore) -> None: labware_id = well_core.labware_id well_name = well_core.get_name() well_location = WellLocation( @@ -862,7 +862,7 @@ def find_liquid_presence_with_recovery(self, well_core: WellCore) -> None: ) ) - def find_liquid_level_without_recovery(self, well_core: WellCore) -> float: + def liquid_probe_without_recovery(self, well_core: WellCore) -> float: labware_id = well_core.labware_id well_name = well_core.get_name() well_location = WellLocation( diff --git a/api/src/opentrons/protocol_api/core/instrument.py b/api/src/opentrons/protocol_api/core/instrument.py index 4894f0edb8b..2ad70c7274b 100644 --- a/api/src/opentrons/protocol_api/core/instrument.py +++ b/api/src/opentrons/protocol_api/core/instrument.py @@ -304,12 +304,12 @@ def retract(self) -> None: ... @abstractmethod - def find_liquid_presence_with_recovery(self, well_core: WellCoreType) -> None: + def liquid_probe_with_recovery(self, well_core: WellCoreType) -> None: """Do a liquid probe to detect the presence of liquid in the well.""" ... @abstractmethod - def find_liquid_level_without_recovery(self, well_core: WellCoreType) -> float: + def liquid_probe_without_recovery(self, well_core: WellCoreType) -> float: """Do a liquid probe to find the level of the liquid in the well.""" ... diff --git a/api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py b/api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py index bc3b1c0770a..02f0bef1b0a 100644 --- a/api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py +++ b/api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py @@ -571,14 +571,10 @@ def retract(self) -> None: """Retract this instrument to the top of the gantry.""" self._protocol_interface.get_hardware.retract(self._mount) # type: ignore [attr-defined] - def find_liquid_presence_with_recovery(self, well_core: WellCore) -> None: + def liquid_probe_with_recovery(self, well_core: WellCore) -> None: """This will never be called because it was added in API 2.20.""" - assert ( - False - ), "find_liquid_presence_with_recovery only supported in API 2.20 & later" + assert False, "liquid_probe_with_recovery only supported in API 2.20 & later" - def find_liquid_level_without_recovery(self, well_core: WellCore) -> float: + def liquid_probe_without_recovery(self, well_core: WellCore) -> float: """This will never be called because it was added in API 2.20.""" - assert ( - False - ), "find_liquid_level_without_recovery only supported in API 2.20 & later" + assert False, "liquid_probe_without_recovery only supported in API 2.20 & later" diff --git a/api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py b/api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py index 71482fac30e..f53279334a4 100644 --- a/api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py +++ b/api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py @@ -489,14 +489,10 @@ def retract(self) -> None: """Retract this instrument to the top of the gantry.""" self._protocol_interface.get_hardware.retract(self._mount) # type: ignore [attr-defined] - def find_liquid_presence_with_recovery(self, well_core: WellCore) -> None: + def liquid_probe_with_recovery(self, well_core: WellCore) -> None: """This will never be called because it was added in API 2.20.""" - assert ( - False - ), "find_liquid_presence_with_recovery only supported in API 2.20 & later" + assert False, "liquid_probe_with_recovery only supported in API 2.20 & later" - def find_liquid_level_without_recovery(self, well_core: WellCore) -> float: + def liquid_probe_without_recovery(self, well_core: WellCore) -> float: """This will never be called because it was added in API 2.20.""" - assert ( - False - ), "find_liquid_level_without_recovery only supported in API 2.20 & later" + assert False, "liquid_probe_without_recovery only supported in API 2.20 & later" diff --git a/api/src/opentrons/protocol_api/instrument_context.py b/api/src/opentrons/protocol_api/instrument_context.py index 43810a9755c..2c5b1c58160 100644 --- a/api/src/opentrons/protocol_api/instrument_context.py +++ b/api/src/opentrons/protocol_api/instrument_context.py @@ -2,11 +2,11 @@ import logging from contextlib import ExitStack from typing import Any, List, Optional, Sequence, Union, cast, Dict +from opentrons.protocol_engine.commands.pipetting_common import LiquidNotFoundError from opentrons.protocol_engine.errors.error_occurrence import ProtocolCommandFailedError from opentrons_shared_data.errors.exceptions import ( CommandPreconditionViolated, CommandParameterLimitViolated, - PipetteLiquidNotFoundError, UnexpectedTipRemovalError, ) from opentrons.protocol_engine.errors.exceptions import WellDoesNotExistError @@ -2058,16 +2058,13 @@ def detect_liquid_presence(self, well: labware.Well) -> bool: if not isinstance(well, labware.Well): raise WellDoesNotExistError("You must provide a valid well to check.") try: - self._core.find_liquid_level_without_recovery(well._core) - return True - except PipetteLiquidNotFoundError: - return False + self._core.liquid_probe_without_recovery(well._core) except ProtocolCommandFailedError as e: - if isinstance(e.original_error, PipetteLiquidNotFoundError): + if isinstance(e.original_error, LiquidNotFoundError): return False raise e - except Exception as e: - raise e + else: + return True @requires_version(2, 20) def require_liquid_presence(self, well: labware.Well) -> None: @@ -2078,7 +2075,7 @@ def require_liquid_presence(self, well: labware.Well) -> None: if not isinstance(well, labware.Well): raise WellDoesNotExistError("You must provide a valid well to check.") - self._core.find_liquid_presence_with_recovery(well._core) + self._core.liquid_probe_with_recovery(well._core) @requires_version(2, 20) def measure_liquid_height(self, well: labware.Well) -> float: @@ -2093,5 +2090,5 @@ def measure_liquid_height(self, well: labware.Well) -> float: if not isinstance(well, labware.Well): raise WellDoesNotExistError("You must provide a valid well to check.") - height = self._core.find_liquid_level_without_recovery(well._core) - return float(height) + height = self._core.liquid_probe_without_recovery(well._core) + return height 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 d9ceabb0260..a127fb854bf 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 @@ -1292,7 +1292,7 @@ def test_configure_for_volume_post_219( @pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 20))) -def test_find_liquid_level_without_recovery( +def test_liquid_probe_without_recovery( decoy: Decoy, mock_engine_client: EngineClient, mock_protocol_core: ProtocolCore, @@ -1304,7 +1304,7 @@ def test_find_liquid_level_without_recovery( name="my cool well", labware_id="123abc", engine_client=mock_engine_client ) try: - subject.find_liquid_level_without_recovery(well_core=well_core) + subject.liquid_probe_without_recovery(well_core=well_core) except PipetteLiquidNotFoundError: assert True decoy.verify( @@ -1322,7 +1322,7 @@ def test_find_liquid_level_without_recovery( @pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 20))) -def test_find_liquid_presence_with_recovery( +def test_liquid_probe_with_recovery( decoy: Decoy, mock_engine_client: EngineClient, mock_protocol_core: ProtocolCore, @@ -1334,7 +1334,7 @@ def test_find_liquid_presence_with_recovery( name="my cool well", labware_id="123abc", engine_client=mock_engine_client ) try: - subject.find_liquid_presence_with_recovery(well_core=well_core) + subject.liquid_probe_with_recovery(well_core=well_core) except PipetteLiquidNotFoundError: assert True decoy.verify( diff --git a/api/tests/opentrons/protocol_api/test_instrument_context.py b/api/tests/opentrons/protocol_api/test_instrument_context.py index 433f04523e5..9dd029be775 100644 --- a/api/tests/opentrons/protocol_api/test_instrument_context.py +++ b/api/tests/opentrons/protocol_api/test_instrument_context.py @@ -1,6 +1,11 @@ """Tests for the InstrumentContext public interface.""" from collections import OrderedDict +from datetime import datetime import inspect +from opentrons.protocol_engine.commands.pipetting_common import LiquidNotFoundError +from opentrons.protocol_engine.errors.error_occurrence import ( + ProtocolCommandFailedError, +) import pytest from pytest_lazyfixture import lazy_fixture # type: ignore[import-untyped] from decoy import Decoy @@ -38,7 +43,6 @@ from opentrons_shared_data.errors.exceptions import ( CommandPreconditionViolated, - PipetteLiquidNotFoundError, ) @@ -1279,9 +1283,14 @@ def test_detect_liquid_presence( ) -> None: """It should only return booleans. Not raise an exception.""" mock_well = decoy.mock(cls=Well) + lnfe = LiquidNotFoundError(id="1234", createdAt=datetime.now()) + errorToRaise = ProtocolCommandFailedError( + original_error=lnfe, + message=f"{lnfe.errorType}: {lnfe.detail}", + ) decoy.when( - mock_instrument_core.find_liquid_level_without_recovery(mock_well._core) - ).then_raise(PipetteLiquidNotFoundError()) + mock_instrument_core.liquid_probe_without_recovery(mock_well._core) + ).then_raise(errorToRaise) result = subject.detect_liquid_presence(mock_well) assert isinstance(result, bool) @@ -1295,13 +1304,20 @@ def test_require_liquid_presence( ) -> None: """It should raise an exception when called.""" mock_well = decoy.mock(cls=Well) - decoy.when(mock_instrument_core.find_liquid_presence_with_recovery(mock_well._core)) + lnfe = LiquidNotFoundError(id="1234", createdAt=datetime.now()) + errorToRaise = ProtocolCommandFailedError( + original_error=lnfe, + message=f"{lnfe.errorType}: {lnfe.detail}", + ) + decoy.when(mock_instrument_core.liquid_probe_with_recovery(mock_well._core)) subject.require_liquid_presence(mock_well) decoy.when( - mock_instrument_core.find_liquid_presence_with_recovery(mock_well._core) - ).then_raise(PipetteLiquidNotFoundError()) - with pytest.raises(PipetteLiquidNotFoundError): + mock_instrument_core.liquid_probe_with_recovery(mock_well._core) + ).then_raise(errorToRaise) + with pytest.raises(ProtocolCommandFailedError) as pcfe: subject.require_liquid_presence(mock_well) + assert pcfe.value.original_error is not None + assert pcfe.value.original_error.errorType == "liquidNotFound" @pytest.mark.parametrize("api_version", [APIVersion(2, 20)]) @@ -1313,8 +1329,15 @@ def test_measure_liquid_height( ) -> None: """It should raise an exception when called.""" mock_well = decoy.mock(cls=Well) + lnfe = LiquidNotFoundError(id="1234", createdAt=datetime.now()) + errorToRaise = ProtocolCommandFailedError( + original_error=lnfe, + message=f"{lnfe.errorType}: {lnfe.detail}", + ) decoy.when( - mock_instrument_core.find_liquid_level_without_recovery(mock_well._core) - ).then_raise(PipetteLiquidNotFoundError()) - with pytest.raises(PipetteLiquidNotFoundError): + mock_instrument_core.liquid_probe_without_recovery(mock_well._core) + ).then_raise(errorToRaise) + with pytest.raises(ProtocolCommandFailedError) as pcfe: subject.measure_liquid_height(mock_well) + assert pcfe.value.original_error is not None + assert pcfe.value.original_error.errorType == "liquidNotFound" From 9fa2651d887b717af714f2dfe96e858b39cba16b Mon Sep 17 00:00:00 2001 From: aaron-kulkarni Date: Fri, 5 Jul 2024 13:25:04 -0400 Subject: [PATCH 4/5] instrument core test updates --- .../opentrons/protocol_api/core/engine/instrument.py | 1 + .../protocol_api/core/engine/test_instrument_core.py | 11 +++-------- .../opentrons/protocol_api/test_instrument_context.py | 7 +++---- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/api/src/opentrons/protocol_api/core/engine/instrument.py b/api/src/opentrons/protocol_api/core/engine/instrument.py index c09a6b19136..ac906856266 100644 --- a/api/src/opentrons/protocol_api/core/engine/instrument.py +++ b/api/src/opentrons/protocol_api/core/engine/instrument.py @@ -881,6 +881,7 @@ def liquid_probe_without_recovery(self, well_core: WellCore) -> float: if result is not None and isinstance(result, LiquidProbeResult): return result.z_position # should never get here + print("I got here") raise PipetteLiquidNotFoundError( "Error while trying to find liquid level.", ) 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 a127fb854bf..0c600b7fde8 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 @@ -1303,10 +1303,8 @@ def test_liquid_probe_without_recovery( well_core = WellCore( name="my cool well", labware_id="123abc", engine_client=mock_engine_client ) - try: + with pytest.raises(PipetteLiquidNotFoundError): subject.liquid_probe_without_recovery(well_core=well_core) - except PipetteLiquidNotFoundError: - assert True decoy.verify( mock_engine_client.execute_command_without_recovery( cmd.LiquidProbeParams( @@ -1329,14 +1327,11 @@ def test_liquid_probe_with_recovery( subject: InstrumentCore, version: APIVersion, ) -> None: - """It should raise an exception on an empty well.""" + """It should not raise an exception on an empty well.""" well_core = WellCore( name="my cool well", labware_id="123abc", engine_client=mock_engine_client ) - try: - subject.liquid_probe_with_recovery(well_core=well_core) - except PipetteLiquidNotFoundError: - assert True + subject.liquid_probe_with_recovery(well_core=well_core) decoy.verify( mock_engine_client.execute_command( cmd.LiquidProbeParams( diff --git a/api/tests/opentrons/protocol_api/test_instrument_context.py b/api/tests/opentrons/protocol_api/test_instrument_context.py index 9dd029be775..0d69d462098 100644 --- a/api/tests/opentrons/protocol_api/test_instrument_context.py +++ b/api/tests/opentrons/protocol_api/test_instrument_context.py @@ -1293,6 +1293,7 @@ def test_detect_liquid_presence( ).then_raise(errorToRaise) result = subject.detect_liquid_presence(mock_well) assert isinstance(result, bool) + assert not result @pytest.mark.parametrize("api_version", [APIVersion(2, 20)]) @@ -1316,8 +1317,7 @@ def test_require_liquid_presence( ).then_raise(errorToRaise) with pytest.raises(ProtocolCommandFailedError) as pcfe: subject.require_liquid_presence(mock_well) - assert pcfe.value.original_error is not None - assert pcfe.value.original_error.errorType == "liquidNotFound" + assert pcfe.value is errorToRaise @pytest.mark.parametrize("api_version", [APIVersion(2, 20)]) @@ -1339,5 +1339,4 @@ def test_measure_liquid_height( ).then_raise(errorToRaise) with pytest.raises(ProtocolCommandFailedError) as pcfe: subject.measure_liquid_height(mock_well) - assert pcfe.value.original_error is not None - assert pcfe.value.original_error.errorType == "liquidNotFound" + assert pcfe.value is errorToRaise From 5e15bd70fabeb211ed188d04210d46419ff33b8e Mon Sep 17 00:00:00 2001 From: aaron-kulkarni Date: Mon, 8 Jul 2024 14:25:40 -0400 Subject: [PATCH 5/5] final minor test changes --- .../protocol_api/core/engine/instrument.py | 7 ----- .../core/engine/test_instrument_core.py | 30 ++++++++++++++++--- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/api/src/opentrons/protocol_api/core/engine/instrument.py b/api/src/opentrons/protocol_api/core/engine/instrument.py index ac906856266..7b6400bc561 100644 --- a/api/src/opentrons/protocol_api/core/engine/instrument.py +++ b/api/src/opentrons/protocol_api/core/engine/instrument.py @@ -31,8 +31,6 @@ 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.errors.exceptions import PipetteLiquidNotFoundError from opentrons_shared_data.pipette.dev_types import PipetteNameType from opentrons.protocol_api._nozzle_layout import NozzleLayout from opentrons.hardware_control.nozzle_manager import NozzleConfigurationType @@ -880,8 +878,3 @@ def liquid_probe_without_recovery(self, well_core: WellCore) -> float: if result is not None and isinstance(result, LiquidProbeResult): return result.z_position - # should never get here - print("I got here") - raise PipetteLiquidNotFoundError( - "Error while trying to find liquid level.", - ) 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 0c600b7fde8..3ca12bc004f 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 @@ -1,6 +1,7 @@ """Test for the ProtocolEngine-based instrument API core.""" from typing import cast, Optional, Union +from opentrons.protocol_engine.commands.liquid_probe import LiquidProbeResult from opentrons_shared_data.errors.exceptions import PipetteLiquidNotFoundError import pytest from decoy import Decoy @@ -1299,13 +1300,33 @@ def test_liquid_probe_without_recovery( subject: InstrumentCore, version: APIVersion, ) -> None: - """It should raise an exception on an empty well.""" + """It should raise an exception on an empty well and return a float on a valid well.""" well_core = WellCore( name="my cool well", labware_id="123abc", engine_client=mock_engine_client ) - with pytest.raises(PipetteLiquidNotFoundError): + decoy.when( + mock_engine_client.execute_command_without_recovery( + cmd.LiquidProbeParams( + pipetteId=subject.pipette_id, + wellLocation=WellLocation( + origin=WellOrigin.TOP, offset=WellOffset(x=0, y=0, z=0) + ), + wellName=well_core.get_name(), + labwareId=well_core.labware_id, + ) + ) + ).then_raise(PipetteLiquidNotFoundError()) + try: subject.liquid_probe_without_recovery(well_core=well_core) - decoy.verify( + except PipetteLiquidNotFoundError: + assert True + else: + assert False + + decoy.reset() + + lpr = LiquidProbeResult(z_position=5.0) + decoy.when( mock_engine_client.execute_command_without_recovery( cmd.LiquidProbeParams( pipetteId=subject.pipette_id, @@ -1316,7 +1337,8 @@ def test_liquid_probe_without_recovery( labwareId=well_core.labware_id, ) ) - ) + ).then_return(lpr) + assert subject.liquid_probe_without_recovery(well_core=well_core) == 5.0 @pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 20)))