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

<!--
Thanks for taking the time to open a pull request! Please make sure
you've read the "Opening Pull Requests" section of our Contributing
Guide:


https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests

To ensure your code is reviewed quickly and thoroughly, please fill out
the sections below to the best of your ability!
-->

# Overview

Edit detect_liquid_presence, require_liquid_presence, and
measure_liquid_height as per the standup today.

Change instrument_context tests to check for the right
error(LiquidNotFoundError)

Split find_liquid_level into two functions: liquid_probe_with_recovery
and liquid_probe_without_recovery

<!--
Use this section to describe your pull-request at a high level. If the
PR addresses any open issues, please tag the issues here.
-->

# Test Plan

<!--
Use this section to describe the steps that you took to test your Pull
Request.
If you did not perform any testing provide justification why.

OT-3 Developers: You should default to testing on actual physical
hardware.
Once again, if you did not perform testing against hardware, justify
why.

Note: It can be helpful to write a test plan before doing development

Example Test Plan (HTTP API Change)

- Verified that new optional argument `dance-party` causes the robot to
flash its lights, move the pipettes,
then home.
- Verified that when you omit the `dance-party` option the robot homes
normally
- Added protocol that uses `dance-party` argument to G-Code Testing
Suite
- Ran protocol that did not use `dance-party` argument and everything
was successful
- Added unit tests to validate that changes to pydantic model are
correct

-->

Added unit tests for liquid_probe_with_recovery and liquid_probe_without
recovery to test_instrument_core.py
Added unit tests for detect_liquid_presence, require_liquid_presence,
and measure_liquid_height to test_instrument_context.py

After the PR is merged, these API calls will be tested on the robot.

# Changelog

<!--
List out the changes to the code in this PR. Please try your best to
categorize your changes and describe what has changed and why.

Example changelog:
- Fixed app crash when trying to calibrate an illegal pipette
- Added state to API to track pipette usage
- Updated API docs to mention only two pipettes are supported

IMPORTANT: MAKE SURE ANY BREAKING CHANGES ARE PROPERLY COMMUNICATED
-->

# Review requests

<!--
Describe any requests for your reviewers here.
-->

Accidentally merged #15555 too soon, this is a continuation of that PR.

# Risk assessment

<!--
Carefully go over your pull request and look at the other parts of the
codebase it may affect. Look for the possibility, even if you think it's
small, that your change may affect some other part of the system - for
instance, changing return tip behavior in protocol may also change the
behavior of labware calibration.

Identify the other parts of the system your codebase may affect, so that
in addition to your own review and testing, other people who may not
have the system internalized as much as you can focus their attention
and testing there.
-->

Low: Since all the work was done in newly created functions, the risk
that it breaks existing code is minimal.
However, the new API calls still do need to be tested on a robot before
they can be used in other parts of the system.

Author: aaron-kulkarni

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
@@ -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.",
-        )

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 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."""
         ...
 

api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py

@@ -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"

api/src/opentrons/protocol_api/core/legacy_simulator/legacy_instrument_core.py

@@ -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"

api/src/opentrons/protocol_api/instrument_context.py

@@ -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
@@ -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:
@@ -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

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
 
@@ -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."""

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
@@ -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(
@@ -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(

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,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)])
@@ -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)])
@@ -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