From 65b0d7f3fcf8cd870b2dd22d27e26176e73f9163 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Thu, 19 Sep 2024 12:47:19 -0400 Subject: [PATCH] fix(api): Fix liquidProbe error message after blowout (#16294) ## Overview This is the error message fix part of RQA-3171. ## Changelog When we're not ready to do a `liquidProbe`, be a little bit finer-grained about why we're not ready. Give the "missing tip" case and "plunger not ready" case their own error messages. --- .../protocol_engine/commands/liquid_probe.py | 34 ++++- .../protocol_engine/execution/pipetting.py | 11 -- .../protocol_engine/state/pipettes.py | 2 +- .../commands/test_liquid_probe.py | 128 +++++++----------- 4 files changed, 80 insertions(+), 95 deletions(-) diff --git a/api/src/opentrons/protocol_engine/commands/liquid_probe.py b/api/src/opentrons/protocol_engine/commands/liquid_probe.py index ecf932a3470..d352a08015f 100644 --- a/api/src/opentrons/protocol_engine/commands/liquid_probe.py +++ b/api/src/opentrons/protocol_engine/commands/liquid_probe.py @@ -2,7 +2,11 @@ from __future__ import annotations from typing import TYPE_CHECKING, Optional, Type, Union -from opentrons.protocol_engine.errors.exceptions import MustHomeError, TipNotEmptyError +from opentrons.protocol_engine.errors.exceptions import ( + MustHomeError, + PipetteNotReadyToAspirateError, + TipNotEmptyError, +) from opentrons.types import MountType from opentrons_shared_data.errors.exceptions import ( PipetteLiquidNotFoundError, @@ -32,6 +36,7 @@ if TYPE_CHECKING: from ..execution import MovementHandler, PipettingHandler from ..resources import ModelUtils + from ..state import StateView LiquidProbeCommandType = Literal["liquidProbe"] @@ -92,11 +97,13 @@ class LiquidProbeImplementation( def __init__( self, + state_view: StateView, movement: MovementHandler, pipetting: PipettingHandler, model_utils: ModelUtils, **kwargs: object, ) -> None: + self._state_view = state_view self._movement = movement self._pipetting = pipetting self._model_utils = model_utils @@ -112,6 +119,8 @@ async def execute(self, params: _CommonParams) -> _LiquidProbeExecuteReturn: the pipette. TipNotEmptyError: as an undefined error, if the tip starts with liquid in it. + PipetteNotReadyToAspirateError: as an undefined error, if the plunger is not + in a safe position to do the liquid probe. MustHomeError: as an undefined error, if the plunger is not in a valid position. """ @@ -119,13 +128,21 @@ async def execute(self, params: _CommonParams) -> _LiquidProbeExecuteReturn: labware_id = params.labwareId well_name = params.wellName - # _validate_tip_attached in pipetting.py is a private method so we're using - # get_is_ready_to_aspirate as an indirect way to throw a TipNotAttachedError if appropriate - self._pipetting.get_is_ready_to_aspirate(pipette_id=pipette_id) - - if self._pipetting.get_is_empty(pipette_id=pipette_id) is False: + # May raise TipNotAttachedError. + aspirated_volume = self._state_view.pipettes.get_aspirated_volume(pipette_id) + + if aspirated_volume is None: + # Theoretically, we could avoid raising an error by automatically preparing + # to aspirate above the well like AspirateImplementation does. However, the + # only way for this to happen is if someone tries to do a liquid probe with + # a tip that's previously held liquid, which they should avoid anyway. + raise PipetteNotReadyToAspirateError( + "The pipette cannot probe liquid because of a previous blow out." + " The plunger must be reset while the tip is somewhere away from liquid." + ) + elif aspirated_volume != 0: raise TipNotEmptyError( - message="This operation requires a tip with no liquid in it." + message="The pipette cannot probe for liquid when the tip has liquid in it." ) if await self._movement.check_for_valid_position(mount=MountType.LEFT) is False: @@ -182,11 +199,13 @@ class TryLiquidProbeImplementation( def __init__( self, + state_view: StateView, movement: MovementHandler, pipetting: PipettingHandler, model_utils: ModelUtils, **kwargs: object, ) -> None: + self._state_view = state_view self._movement = movement self._pipetting = pipetting self._model_utils = model_utils @@ -203,6 +222,7 @@ async def execute(self, params: _CommonParams) -> _TryLiquidProbeExecuteReturn: # Otherwise, we return the result or propagate the exception unchanged. original_impl = LiquidProbeImplementation( + state_view=self._state_view, movement=self._movement, pipetting=self._pipetting, model_utils=self._model_utils, diff --git a/api/src/opentrons/protocol_engine/execution/pipetting.py b/api/src/opentrons/protocol_engine/execution/pipetting.py index c3e606849ff..7812378f3de 100644 --- a/api/src/opentrons/protocol_engine/execution/pipetting.py +++ b/api/src/opentrons/protocol_engine/execution/pipetting.py @@ -29,9 +29,6 @@ class PipettingHandler(TypingProtocol): """Liquid handling commands.""" - def get_is_empty(self, pipette_id: str) -> bool: - """Get whether a pipette has an aspirated volume equal to 0.""" - def get_is_ready_to_aspirate(self, pipette_id: str) -> bool: """Get whether a pipette is ready to aspirate.""" @@ -81,10 +78,6 @@ def __init__(self, state_view: StateView, hardware_api: HardwareControlAPI) -> N self._state_view = state_view self._hardware_api = hardware_api - def get_is_empty(self, pipette_id: str) -> bool: - """Get whether a pipette has an aspirated volume equal to 0.""" - return self._state_view.pipettes.get_aspirated_volume(pipette_id) == 0 - def get_is_ready_to_aspirate(self, pipette_id: str) -> bool: """Get whether a pipette is ready to aspirate.""" hw_pipette = self._state_view.pipettes.get_hardware_pipette( @@ -236,10 +229,6 @@ def __init__( """Initialize a PipettingHandler instance.""" self._state_view = state_view - def get_is_empty(self, pipette_id: str) -> bool: - """Get whether a pipette has an aspirated volume equal to 0.""" - return self._state_view.pipettes.get_aspirated_volume(pipette_id) == 0 - def get_is_ready_to_aspirate(self, pipette_id: str) -> bool: """Get whether a pipette is ready to aspirate.""" return self._state_view.pipettes.get_aspirated_volume(pipette_id) is not None diff --git a/api/src/opentrons/protocol_engine/state/pipettes.py b/api/src/opentrons/protocol_engine/state/pipettes.py index 7552581a69c..203f5ddddab 100644 --- a/api/src/opentrons/protocol_engine/state/pipettes.py +++ b/api/src/opentrons/protocol_engine/state/pipettes.py @@ -611,7 +611,7 @@ def get_aspirated_volume(self, pipette_id: str) -> Optional[float]: Returns: The volume the pipette has aspirated. - None, after blow-out and the plunger is in an unsafe position or drop-tip and there is no tip attached. + None, after blow-out and the plunger is in an unsafe position. Raises: PipetteNotLoadedError: pipette ID does not exist. diff --git a/api/tests/opentrons/protocol_engine/commands/test_liquid_probe.py b/api/tests/opentrons/protocol_engine/commands/test_liquid_probe.py index 61f4339360d..b935f6bf2c1 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_liquid_probe.py +++ b/api/tests/opentrons/protocol_engine/commands/test_liquid_probe.py @@ -4,6 +4,7 @@ from opentrons.protocol_engine.errors.exceptions import ( MustHomeError, + PipetteNotReadyToAspirateError, TipNotAttachedError, TipNotEmptyError, ) @@ -37,7 +38,6 @@ PipettingHandler, ) from opentrons.protocol_engine.resources.model_utils import ModelUtils -from opentrons.protocol_engine.types import LoadedPipette EitherImplementationType = Union[ @@ -84,12 +84,14 @@ def result_type(types: tuple[object, object, EitherResultType]) -> EitherResultT @pytest.fixture def subject( implementation_type: EitherImplementationType, + state_view: StateView, movement: MovementHandler, pipetting: PipettingHandler, model_utils: ModelUtils, ) -> Union[LiquidProbeImplementation, TryLiquidProbeImplementation]: """Get the implementation subject.""" return implementation_type( + state_view=state_view, pipetting=pipetting, movement=movement, model_utils=model_utils, @@ -99,6 +101,7 @@ def subject( async def test_liquid_probe_implementation_no_prep( decoy: Decoy, movement: MovementHandler, + state_view: StateView, pipetting: PipettingHandler, subject: EitherImplementation, params_type: EitherParamsType, @@ -114,7 +117,9 @@ async def test_liquid_probe_implementation_no_prep( wellLocation=location, ) - decoy.when(pipetting.get_is_ready_to_aspirate(pipette_id="abc")).then_return(True) + decoy.when(state_view.pipettes.get_aspirated_volume(pipette_id="abc")).then_return( + 0 + ) decoy.when( await movement.move_to_well( @@ -143,69 +148,9 @@ async def test_liquid_probe_implementation_no_prep( ) -async def test_liquid_probe_implementation_with_prep( - decoy: Decoy, - state_view: StateView, - movement: MovementHandler, - pipetting: PipettingHandler, - subject: EitherImplementation, - params_type: EitherParamsType, - result_type: EitherResultType, -) -> None: - """A Liquid Probe should have an execution implementation with preparing to aspirate.""" - location = WellLocation(origin=WellOrigin.TOP, offset=WellOffset(x=0, y=0, z=2)) - - data = params_type( - pipetteId="abc", - labwareId="123", - wellName="A3", - wellLocation=location, - ) - - decoy.when(pipetting.get_is_ready_to_aspirate(pipette_id="abc")).then_return(False) - - decoy.when(state_view.pipettes.get(pipette_id="abc")).then_return( - LoadedPipette.construct( # type:ignore[call-arg] - mount=MountType.LEFT - ) - ) - decoy.when( - await movement.move_to_well( - pipette_id="abc", labware_id="123", well_name="A3", well_location=location - ), - ).then_return(Point(x=1, y=2, z=3)) - - decoy.when( - await pipetting.liquid_probe_in_place( - pipette_id="abc", - labware_id="123", - well_name="A3", - well_location=location, - ), - ).then_return(15.0) - - result = await subject.execute(data) - - assert type(result.public) is result_type # Pydantic v1 only compares the fields. - assert result == SuccessData( - public=result_type(z_position=15.0, position=DeckPoint(x=1, y=2, z=3)), - private=None, - ) - - decoy.verify( - await movement.move_to_well( - pipette_id="abc", - labware_id="123", - well_name="A3", - well_location=WellLocation( - origin=WellOrigin.TOP, offset=WellOffset(x=0, y=0, z=2) - ), - ), - ) - - async def test_liquid_not_found_error( decoy: Decoy, + state_view: StateView, movement: MovementHandler, pipetting: PipettingHandler, subject: EitherImplementation, @@ -232,9 +177,7 @@ async def test_liquid_not_found_error( wellLocation=well_location, ) - decoy.when(pipetting.get_is_ready_to_aspirate(pipette_id=pipette_id)).then_return( - True - ) + decoy.when(state_view.pipettes.get_aspirated_volume(pipette_id)).then_return(0) decoy.when( await movement.move_to_well( @@ -282,11 +225,11 @@ async def test_liquid_not_found_error( async def test_liquid_probe_tip_checking( decoy: Decoy, - pipetting: PipettingHandler, + state_view: StateView, subject: EitherImplementation, params_type: EitherParamsType, ) -> None: - """It should return a TipNotAttached error if the hardware API indicates that.""" + """It should raise a TipNotAttached error if the state view indicates that.""" pipette_id = "pipette-id" labware_id = "labware-id" well_name = "well-name" @@ -301,18 +244,42 @@ async def test_liquid_probe_tip_checking( wellLocation=well_location, ) - decoy.when( - pipetting.get_is_ready_to_aspirate( - pipette_id=pipette_id, - ), - ).then_raise(TipNotAttachedError()) + decoy.when(state_view.pipettes.get_aspirated_volume(pipette_id)).then_raise( + TipNotAttachedError() + ) with pytest.raises(TipNotAttachedError): await subject.execute(data) +async def test_liquid_probe_plunger_preparedness_checking( + decoy: Decoy, + state_view: StateView, + subject: EitherImplementation, + params_type: EitherParamsType, +) -> None: + """It should raise a PipetteNotReadyToAspirate error if the state view indicates that.""" + pipette_id = "pipette-id" + labware_id = "labware-id" + well_name = "well-name" + well_location = WellLocation( + origin=WellOrigin.BOTTOM, offset=WellOffset(x=0, y=0, z=1) + ) + + data = params_type( + pipetteId=pipette_id, + labwareId=labware_id, + wellName=well_name, + wellLocation=well_location, + ) + + decoy.when(state_view.pipettes.get_aspirated_volume(pipette_id)).then_return(None) + with pytest.raises(PipetteNotReadyToAspirateError): + await subject.execute(data) + + async def test_liquid_probe_volume_checking( decoy: Decoy, - pipetting: PipettingHandler, + state_view: StateView, subject: EitherImplementation, params_type: EitherParamsType, ) -> None: @@ -330,15 +297,23 @@ async def test_liquid_probe_volume_checking( wellName=well_name, wellLocation=well_location, ) + decoy.when( - pipetting.get_is_empty(pipette_id=pipette_id), - ).then_return(False) + state_view.pipettes.get_aspirated_volume(pipette_id=pipette_id), + ).then_return(123) with pytest.raises(TipNotEmptyError): await subject.execute(data) + decoy.when( + state_view.pipettes.get_aspirated_volume(pipette_id=pipette_id), + ).then_return(None) + with pytest.raises(PipetteNotReadyToAspirateError): + await subject.execute(data) + async def test_liquid_probe_location_checking( decoy: Decoy, + state_view: StateView, movement: MovementHandler, subject: EitherImplementation, params_type: EitherParamsType, @@ -357,6 +332,7 @@ async def test_liquid_probe_location_checking( wellName=well_name, wellLocation=well_location, ) + decoy.when(state_view.pipettes.get_aspirated_volume(pipette_id)).then_return(0) decoy.when( await movement.check_for_valid_position( mount=MountType.LEFT,