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

refactor(api): better names for liquid probe commands #15578

Merged
merged 5 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 24 additions & 23 deletions api/src/opentrons/protocol_api/core/engine/instrument.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -846,34 +844,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 liquid_probe_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 liquid_probe_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
# should never get here
raise PipetteLiquidNotFoundError(
"Error while trying to find liquid level.",
)
7 changes: 6 additions & 1 deletion api/src/opentrons/protocol_api/core/instrument.py
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,12 @@ def retract(self) -> None:
...

@abstractmethod
def find_liquid_level(self, well_core: WellCoreType, error_recovery: bool) -> float:
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 liquid_probe_without_recovery(self, well_core: WellCoreType) -> float:
"""Do a liquid probe to find the level of the liquid in the well."""
...

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +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_level(self, well_core: WellCore, error_recovery: bool) -> float:
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_level only supported in API 2.20 & later"
assert False, "liquid_probe_with_recovery only supported in API 2.20 & later"

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, "liquid_probe_without_recovery only supported in API 2.20 & later"
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +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_level(self, well_core: WellCore, error_recovery: bool) -> float:
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_level only supported in API 2.20 & later"
assert False, "liquid_probe_with_recovery only supported in API 2.20 & later"

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, "liquid_probe_without_recovery only supported in API 2.20 & later"
30 changes: 15 additions & 15 deletions api/src/opentrons/protocol_api/instrument_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +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
Expand Down Expand Up @@ -2057,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:
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
except PipetteLiquidNotFoundError:
return False
except Exception as e:
self._core.liquid_probe_without_recovery(well._core)
except ProtocolCommandFailedError as e:
if isinstance(e.original_error, LiquidNotFoundError):
return False
raise e
else:
return True

@requires_version(2, 20)
def require_liquid_presence(self, well: labware.Well) -> None:
Expand All @@ -2077,18 +2075,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.liquid_probe_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
)
return float(height)
height = self._core.liquid_probe_without_recovery(well._core)
return height
21 changes: 0 additions & 21 deletions api/src/opentrons/protocol_engine/clients/sync_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -96,26 +95,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."""
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -1292,23 +1293,41 @@ 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_liquid_probe_without_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."""
"""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
)
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.find_liquid_level(well_core=well_core, error_recovery=True)
subject.liquid_probe_without_recovery(well_core=well_core)
except PipetteLiquidNotFoundError:
assert True
decoy.verify(
mock_engine_client.execute_command_with_result(
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,
wellLocation=WellLocation(
Expand All @@ -1318,13 +1337,25 @@ def test_find_liquid_level(
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)))
def test_liquid_probe_with_recovery(
decoy: Decoy,
mock_engine_client: EngineClient,
mock_protocol_core: ProtocolCore,
subject: InstrumentCore,
version: APIVersion,
) -> None:
"""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.find_liquid_level(well_core=well_core, error_recovery=False)
except PipetteLiquidNotFoundError:
assert True
subject.liquid_probe_with_recovery(well_core=well_core)
decoy.verify(
mock_engine_client.execute_command_without_recovery(
mock_engine_client.execute_command(
cmd.LiquidProbeParams(
pipetteId=subject.pipette_id,
wellLocation=WellLocation(
Expand Down
42 changes: 32 additions & 10 deletions api/tests/opentrons/protocol_api/test_instrument_context.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -38,7 +43,6 @@

from opentrons_shared_data.errors.exceptions import (
CommandPreconditionViolated,
PipetteLiquidNotFoundError,
)


Expand Down Expand Up @@ -1279,11 +1283,17 @@ 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(mock_well._core, False)
).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)
assert not result


@pytest.mark.parametrize("api_version", [APIVersion(2, 20)])
Expand All @@ -1295,13 +1305,19 @@ 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))
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_level(mock_well._core, True)
).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 is errorToRaise


@pytest.mark.parametrize("api_version", [APIVersion(2, 20)])
Expand All @@ -1313,8 +1329,14 @@ 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(mock_well._core, False)
).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 is errorToRaise
Loading