fix(api): Fix floating point errors and backwards incompatibility in volume validation (#14253)

SyntaxColoring · web-flow · commit 876bd1e40c25 · 2024-01-12T10:47:46.000-05:00
diff --git a/api/docs/v2/versioning.rst b/api/docs/v2/versioning.rst
@@ -124,6 +124,11 @@ This table lists the correspondence between Protocol API versions and robot soft
 Changes in API Versions
 =======================
 
+Version 2.17
+------------
+
+- :py:meth:`.dispense` will now raise an error if you try to dispense more than is available.
+
 Version 2.16
 ------------
 
diff --git a/api/release-notes.md b/api/release-notes.md
@@ -6,6 +6,14 @@ log][]. For a list of currently known issues, please see the [Opentrons issue tr
 
 ---
 
+## Opentrons Robot Software Changes in [!!EDIT ME WITH THE ACTUAL NUMBER OF THE NEXT RELEASE!!]
+
+### HTTP API
+
+- In the `/runs/commands`, `/maintenance_runs/commands`, and `/protocols` endpoints, the `dispense` command will now return an error if you try to dispense more than you've aspirated, instead of silently clamping.
+
+---
+
 ## Opentrons Robot Software Changes in 7.1.1
 
 Welcome to the v7.1.1 release of the Opentrons robot software!
diff --git a/api/src/opentrons/hardware_control/instruments/ot2/pipette_handler.py b/api/src/opentrons/hardware_control/instruments/ot2/pipette_handler.py
@@ -619,7 +619,12 @@ def plan_check_dispense(  # type: ignore[no-untyped-def]
         else:
             disp_vol = volume
 
-        # Ensure we don't dispense more than the current volume
+        # Ensure we don't dispense more than the current volume.
+        #
+        # This clamping is inconsistent with plan_check_aspirate(), which asserts
+        # that its input is in bounds instead of clamping it. This is left to avoid
+        # disturbing Python protocols with apiLevel <= 2.13. In newer Python protocols,
+        # the Protocol Engine layer applies its own bounds checking.
         disp_vol = min(instrument.current_volume, disp_vol)
 
         if disp_vol == 0:
diff --git a/api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py b/api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py
@@ -597,7 +597,12 @@ def plan_check_dispense(
         else:
             disp_vol = volume
 
-        # Ensure we don't dispense more than the current volume
+        # Ensure we don't dispense more than the current volume.
+        #
+        # This clamping is inconsistent with plan_check_aspirate(), which asserts
+        # that its input is in bounds instead of clamping it. This is to match a quirk
+        # of the OT-2 version of this class. Protocol Engine does its own clamping,
+        # so we don't expect this to trigger in practice.
         disp_vol = min(instrument.current_volume, disp_vol)
         is_full_dispense = numpy.isclose(instrument.current_volume - disp_vol, 0)
 
diff --git a/api/src/opentrons/hardware_control/ot3api.py b/api/src/opentrons/hardware_control/ot3api.py
@@ -1908,7 +1908,6 @@ async def dispense(
         mount: Union[top_types.Mount, OT3Mount],
         volume: Optional[float] = None,
         rate: float = 1.0,
-        # TODO (tz, 8-24-24): add implementation https://opentrons.atlassian.net/browse/RET-1373
         push_out: Optional[float] = None,
     ) -> None:
         """
diff --git a/api/src/opentrons/protocol_api/core/engine/instrument.py b/api/src/opentrons/protocol_api/core/engine/instrument.py
@@ -2,6 +2,7 @@
 from __future__ import annotations
 
 from typing import Optional, TYPE_CHECKING, cast, Union
+from opentrons.protocols.api_support.types import APIVersion
 
 from opentrons.types import Location, Mount
 from opentrons.hardware_control import SyncHardwareAPI
@@ -44,6 +45,9 @@
     from .protocol import ProtocolCore
 
 
+_DISPENSE_VOLUME_VALIDATION_ADDED_IN = APIVersion(2, 17)
+
+
 class InstrumentCore(AbstractInstrument[WellCore]):
     """Instrument API core using a ProtocolEngine.
 
@@ -180,6 +184,15 @@ def dispense(
             in_place: whether this is a in-place command.
             push_out: The amount to push the plunger below bottom position.
         """
+        if self._protocol_core.api_version < _DISPENSE_VOLUME_VALIDATION_ADDED_IN:
+            # In older API versions, when you try to dispense more than you can,
+            # it gets clamped.
+            volume = min(volume, self.get_current_volume())
+        else:
+            # Newer API versions raise an error if you try to dispense more than
+            # you can. Let the error come from Protocol Engine's validation.
+            pass
+
         if well_core is None:
             if not in_place:
                 if isinstance(location, (TrashBin, WasteChute)):
@@ -733,7 +746,6 @@ def configure_nozzle_layout(
         primary_nozzle: Optional[str],
         front_right_nozzle: Optional[str],
     ) -> None:
-
         if style == NozzleLayout.COLUMN:
             configuration_model: NozzleLayoutConfigurationType = (
                 ColumnNozzleLayoutConfiguration(
diff --git a/api/src/opentrons/protocol_api/instrument_context.py b/api/src/opentrons/protocol_api/instrument_context.py
@@ -346,6 +346,9 @@ def dispense(  # noqa: C901
         .. versionchanged:: 2.15
             Added the ``push_out`` parameter.
 
+        .. versionchanged:: 2.17
+            Now raises an exception if you try to dispense more than is available.
+            Previously, it would silently clamp.
         """
         if self.api_version < APIVersion(2, 15) and push_out:
             raise APIVersionError(
diff --git a/api/src/opentrons/protocol_engine/commands/aspirate.py b/api/src/opentrons/protocol_engine/commands/aspirate.py
@@ -5,7 +5,7 @@
 
 from .pipetting_common import (
     PipetteIdMixin,
-    VolumeMixin,
+    AspirateVolumeMixin,
     FlowRateMixin,
     WellLocationMixin,
     BaseLiquidHandlingResult,
@@ -25,7 +25,9 @@
 AspirateCommandType = Literal["aspirate"]
 
 
-class AspirateParams(PipetteIdMixin, VolumeMixin, FlowRateMixin, WellLocationMixin):
+class AspirateParams(
+    PipetteIdMixin, AspirateVolumeMixin, FlowRateMixin, WellLocationMixin
+):
     """Parameters required to aspirate from a specific well."""
 
     pass
diff --git a/api/src/opentrons/protocol_engine/commands/aspirate_in_place.py b/api/src/opentrons/protocol_engine/commands/aspirate_in_place.py
@@ -8,7 +8,7 @@
 
 from .pipetting_common import (
     PipetteIdMixin,
-    VolumeMixin,
+    AspirateVolumeMixin,
     FlowRateMixin,
     BaseLiquidHandlingResult,
 )
@@ -23,7 +23,7 @@
 AspirateInPlaceCommandType = Literal["aspirateInPlace"]
 
 
-class AspirateInPlaceParams(PipetteIdMixin, VolumeMixin, FlowRateMixin):
+class AspirateInPlaceParams(PipetteIdMixin, AspirateVolumeMixin, FlowRateMixin):
     """Payload required to aspirate in place."""
 
     pass
diff --git a/api/src/opentrons/protocol_engine/commands/configure_for_volume.py b/api/src/opentrons/protocol_engine/commands/configure_for_volume.py
@@ -1,13 +1,10 @@
 """Configure for volume command request, result, and implementation models."""
 from __future__ import annotations
-from pydantic import BaseModel
+from pydantic import BaseModel, Field
 from typing import TYPE_CHECKING, Optional, Type, Tuple
 from typing_extensions import Literal
 
-from .pipetting_common import (
-    PipetteIdMixin,
-    VolumeMixin,
-)
+from .pipetting_common import PipetteIdMixin
 from .command import (
     AbstractCommandWithPrivateResultImpl,
     BaseCommand,
@@ -22,10 +19,15 @@
 ConfigureForVolumeCommandType = Literal["configureForVolume"]
 
 
-class ConfigureForVolumeParams(PipetteIdMixin, VolumeMixin):
+class ConfigureForVolumeParams(PipetteIdMixin):
     """Parameters required to configure volume for a specific pipette."""
 
-    pass
+    volume: float = Field(
+        ...,
+        description="Amount of liquid in uL. Must be at least 0 and no greater "
+        "than a pipette-specific maximum volume.",
+        ge=0,
+    )
 
 
 class ConfigureForVolumePrivateResult(PipetteConfigUpdateResultMixin):
diff --git a/api/src/opentrons/protocol_engine/commands/dispense.py b/api/src/opentrons/protocol_engine/commands/dispense.py
@@ -8,7 +8,7 @@
 from ..types import DeckPoint
 from .pipetting_common import (
     PipetteIdMixin,
-    VolumeMixin,
+    DispenseVolumeMixin,
     FlowRateMixin,
     WellLocationMixin,
     BaseLiquidHandlingResult,
@@ -23,7 +23,9 @@
 DispenseCommandType = Literal["dispense"]
 
 
-class DispenseParams(PipetteIdMixin, VolumeMixin, FlowRateMixin, WellLocationMixin):
+class DispenseParams(
+    PipetteIdMixin, DispenseVolumeMixin, FlowRateMixin, WellLocationMixin
+):
     """Payload required to dispense to a specific well."""
 
     pushOut: Optional[float] = Field(
diff --git a/api/src/opentrons/protocol_engine/commands/dispense_in_place.py b/api/src/opentrons/protocol_engine/commands/dispense_in_place.py
@@ -7,7 +7,7 @@
 
 from .pipetting_common import (
     PipetteIdMixin,
-    VolumeMixin,
+    DispenseVolumeMixin,
     FlowRateMixin,
     BaseLiquidHandlingResult,
 )
@@ -20,7 +20,7 @@
 DispenseInPlaceCommandType = Literal["dispenseInPlace"]
 
 
-class DispenseInPlaceParams(PipetteIdMixin, VolumeMixin, FlowRateMixin):
+class DispenseInPlaceParams(PipetteIdMixin, DispenseVolumeMixin, FlowRateMixin):
     """Payload required to dispense in place."""
 
     pushOut: Optional[float] = Field(
diff --git a/api/src/opentrons/protocol_engine/commands/pipetting_common.py b/api/src/opentrons/protocol_engine/commands/pipetting_common.py
@@ -14,13 +14,28 @@ class PipetteIdMixin(BaseModel):
     )
 
 
-class VolumeMixin(BaseModel):
-    """Mixin for command requests that take a volume of liquid."""
+class AspirateVolumeMixin(BaseModel):
+    """Mixin for the `volume` field of aspirate commands."""
 
     volume: float = Field(
         ...,
-        description="Amount of liquid in uL. Must be at least 0 and no greater "
-        "than a pipette-specific maximum volume.",
+        description="The amount of liquid to aspirate, in µL."
+        " Must not be greater than the remaining available amount, which depends on"
+        " the pipette (see `loadPipette`), its configuration (see `configureForVolume`),"
+        " the tip (see `pickUpTip`), and the amount you've aspirated so far."
+        " There is some tolerance for floating point rounding errors.",
+        ge=0,
+    )
+
+
+class DispenseVolumeMixin(BaseModel):
+    """Mixin for the `volume` field of dispense commands."""
+
+    volume: float = Field(
+        ...,
+        description="The amount of liquid to dispense, in µL."
+        " Must not be greater than the currently aspirated volume."
+        " There is some tolerance for floating point rounding errors.",
         ge=0,
     )
 
diff --git a/api/src/opentrons/protocol_engine/errors/exceptions.py b/api/src/opentrons/protocol_engine/errors/exceptions.py
@@ -886,42 +886,54 @@ def __init__(
         super().__init__(ErrorCodes.GENERAL_ERROR, message, details, wrapping)
 
 
-class InvalidPipettingVolumeError(ProtocolEngineError):
+class InvalidAspirateVolumeError(ProtocolEngineError):
     """Raised when pipetting a volume larger than the pipette volume."""
 
     def __init__(
         self,
-        message: Optional[str] = None,
-        details: Optional[Dict[str, Any]] = None,
+        attempted_aspirate_volume: float,
+        available_volume: float,
+        max_pipette_volume: float,
+        max_tip_volume: Optional[float],  # None if there's no tip.
         wrapping: Optional[Sequence[EnumeratedError]] = None,
     ) -> None:
         """Build a InvalidPipettingVolumeError."""
+        message = (
+            f"Cannot aspirate {attempted_aspirate_volume} µL when only"
+            f" {available_volume} is available."
+        )
+        details = {
+            "attempted_aspirate_volume": attempted_aspirate_volume,
+            "available_volume": available_volume,
+            "max_pipette_volume": max_pipette_volume,
+            "max_tip_volume": max_tip_volume,
+        }
         super().__init__(ErrorCodes.GENERAL_ERROR, message, details, wrapping)
 
 
-class InvalidPushOutVolumeError(ProtocolEngineError):
-    """Raised when attempting to use an invalid volume for dispense push_out."""
+class InvalidDispenseVolumeError(ProtocolEngineError):
+    """Raised when attempting to dispense a volume that was not aspirated."""
 
     def __init__(
         self,
         message: Optional[str] = None,
         details: Optional[Dict[str, Any]] = None,
         wrapping: Optional[Sequence[EnumeratedError]] = None,
     ) -> None:
-        """Build a InvalidPushOutVolumeError."""
+        """Build a InvalidDispenseVolumeError."""
         super().__init__(ErrorCodes.GENERAL_ERROR, message, details, wrapping)
 
 
-class InvalidDispenseVolumeError(ProtocolEngineError):
-    """Raised when attempting to dispense a volume that was not aspirated."""
+class InvalidPushOutVolumeError(ProtocolEngineError):
+    """Raised when attempting to use an invalid volume for dispense push_out."""
 
     def __init__(
         self,
         message: Optional[str] = None,
         details: Optional[Dict[str, Any]] = None,
         wrapping: Optional[Sequence[EnumeratedError]] = None,
     ) -> None:
-        """Build a InvalidDispenseVolumeError."""
+        """Build a InvalidPushOutVolumeError."""
         super().__init__(ErrorCodes.GENERAL_ERROR, message, details, wrapping)
 
 
diff --git a/api/src/opentrons/protocol_engine/execution/pipetting.py b/api/src/opentrons/protocol_engine/execution/pipetting.py
diff --git a/api/src/opentrons/protocol_engine/state/pipettes.py b/api/src/opentrons/protocol_engine/state/pipettes.py
diff --git a/api/src/opentrons/protocols/api_support/definitions.py b/api/src/opentrons/protocols/api_support/definitions.py
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
diff --git a/api/tests/opentrons/protocol_engine/execution/test_pipetting_handler.py b/api/tests/opentrons/protocol_engine/execution/test_pipetting_handler.py
diff --git a/api/tests/opentrons/protocol_engine/state/test_pipette_store.py b/api/tests/opentrons/protocol_engine/state/test_pipette_store.py
diff --git a/shared-data/command/schemas/8.json b/shared-data/command/schemas/8.json
