feat(api): add execution of dispense steps for liquid class based transfer #17138

sanni-t · 2024-12-18T16:08:26Z

Overview

Part 2 of the three-part series of implementing transfer function.

This PR adds InstrumentCore.dispense_liquid_class() which utilizes the TransferComponentsExecutor to execute the dispense steps in specific order. dispense_liquid_class() will then be utilized by the InstrumentCore.transfer_liquid() method to perform dispense during each transfer step. This method can also be accessed in the protocol by using private API accessors for testing purposes.

Changes to TransferComponentsExecutor:

adds retract_after_dispensing() to execute post-dispense retraction steps
adds a tip state tracker to the class in order to keep track of whether the pipette is 'ready to aspirate' and also track the liquid and air gap in the tip that either the single aspirate or dispense steps will be handling.

Test Plan and Hands on Testing

Integrated and on-robot testing will have to wait until this implementation is wired up to the public API
Added unit tests for all new functions and classes

Review requests

Some suggestions from the aspirate PR will still hold to this PR- eg, naming of TransferComponentsExecutor. Those will be addressed in the final PR. But let me know if there are any critical changes that should be made here.
Usual code review stuff

Risk assessment

Low. Makes no changes to the existing code.

sanni-t · 2024-12-18T19:09:40Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

+    def update(
+        self,
+        liquid: Optional[float] = None,
+        air_gap: Optional[float] = None,
+        ready_to_aspirate: Optional[bool] = None,
+    ) -> None:
+        """Update the tip state contents with given values."""
+        if liquid is not None:
+            self.last_liquid_and_air_gap_in_tip.liquid = liquid
+        if air_gap is not None:
+            self.last_liquid_and_air_gap_in_tip.air_gap = air_gap
+        if ready_to_aspirate is not None:
+            self.ready_to_aspirate = ready_to_aspirate


sanni-t · 2024-12-18T19:26:48Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

+            `TransferComponentsExecutor`s should be ready_to_aspirate == True.
+    """
+
+    ready_to_aspirate: bool = True


Consider making this an enum that keeps track of where the plunger is

jbleon95

Overall good, just some minor code organization, clarifications and reminders that may be addressed in other PRs in this stack

jbleon95 · 2025-01-06T16:20:07Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

+
+    ready_to_aspirate: bool = True
+    # TODO: maybe use the tip contents from engine state instead.
+    last_liquid_and_air_gap_in_tip: LiquidAndAirGapPair = LiquidAndAirGapPair(


Reminder this needs to be fixed before the final merge into edge

jbleon95 · 2025-01-06T16:21:11Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

+        # when there is an air gap present.
+        assert (
+            self.last_liquid_and_air_gap_in_tip.air_gap == 0
+        ), "Air gap present in the tip."


Are these intended to be internal asserts for our logic, cause if not we should raise a more well formed error (otherwise this is a-okay)

They're internal asserts for internal logic

jbleon95 · 2025-01-06T16:26:04Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

            self.aspirate_and_wait(volume=mix_properties.volume)
-            # TODO: Update to doing a push out at the end of mix for a post-dispense mix
-            self.dispense_and_wait(volume=mix_properties.volume, push_out=0)
+            if n == 1:


I still think I'd prefer this to be a straightforward loop to n-1 repetitions, then follow that with the push out dispense_and_wait, to avoid the if check and simplify this a little

Adding onto this comment: Can you just shove the logic into the dispense_and_wait() expression? I.e.,

self.dispense_and_wait( ..., push_out_override=push_out_vol if last_dispense_push_out is True and n == 1 else 0 )

jbleon95 · 2025-01-06T16:37:24Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

+        )
+        retract_delay = retract_props.delay
+        if retract_delay.enabled:
+            assert retract_delay.duration is not None


stray thought, but maybe we can add a @property to these liquid class properties to encapsulate the enabled is True and .foo is not None checks

That's a good thought and might help remove all of these redundant checks

jbleon95 · 2025-01-06T16:38:42Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

+            self._instrument.delay(retract_delay.duration)
+
+        blowout_props = retract_props.blowout
+        if (


this block of logic starting here is a little confusing with how it's set up. I'm not sure if it can be simplified, but if not having some comments and organizing the blocks a little differently might make this easier to follow

True. I'll add comments to explain.

jbleon95 · 2025-01-06T16:39:05Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

+                and touch_tip_props.z_offset is not None
+                and touch_tip_props.mm_to_edge is not None
+            )
+            # TODO: update this once touch tip has mmToEdge


we do have mmToEdge now!

Will address it in a follow-up PR.

jbleon95 · 2025-01-06T16:40:55Z

api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py

@@ -130,6 +136,8 @@ def test_aspirate_and_wait(
        transfer_properties=sample_transfer_props,
        target_location=Location(Point(1, 2, 3), labware=None),
        target_well=source_well,
+        tip_state=TipState(),


based on our conversation last week and the fix for the mutable object in TipState, a reminder that some of these tests might be able to be cleaned up

ddcc4 · 2025-01-06T19:28:03Z

api/src/opentrons/protocol_api/core/engine/instrument.py

        # TODO: when aspirating for consolidation, do not perform mix
-        components_executer.mix(mix_properties=aspirate_props.mix)
+        components_executor.mix(


Wait, when aspirating for liquid classes, you ALWAYS mix? Like, the user doesn't have an option to not mix?

No, you don't always mix. The TransferComponentsExecutor.mix() checks whether mix is enabled or not and then performs the mix accordingly.

ddcc4 · 2025-01-06T19:30:20Z

api/src/opentrons/protocol_api/core/engine/instrument.py

-        components_executer.retract_after_aspiration(volume=volume)
+        components_executor.aspirate_and_wait(volume=volume)
+        components_executor.retract_after_aspiration(volume=volume)
+        return components_executor.tip_state.last_liquid_and_air_gap_in_tip


To make sure I understand: You pass in a last_liquid_and_airgap_in_tip to the constructor when you make the components_executor. Then after you call submerge(), mix(), aspirate_and_wait(), etc., the components_executor will have a different last_liquid_and_air_gap_in_tip that you're reading back out here?

ddcc4 · 2025-01-06T19:37:48Z

api/src/opentrons/protocol_api/core/engine/instrument.py

+        )
+        dispense_location = Location(dispense_point, labware=dest_loc.labware)
+        if len(tip_contents) > 0:
+            last_liquid_and_airgap_in_tip = tip_contents[-1]


I think I'm still a little confused about the division of labor between the TransferComponentsExecutor and the functions that use it. Like, the reason you have to keep passing last_liquid_and_airgap_in_tip around is because you create and destroy the TransferComponentsExecutor for each of the substeps?

ddcc4 · 2025-01-06T19:48:55Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

+
+@dataclass
+class LiquidAndAirGapPair:
+    """Pairing of a liquid and air gap in a tip, in that order."""


What order?? :)
Can you spell out whether the air is above or below the liquid physically?

Air is below the liquid. Will update the comment

ddcc4 · 2025-01-06T19:51:52Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

-            volume=air_gap_volume,
-        )
+        if self._transfer_type == TransferType.ONE_TO_ONE:
+            self._remove_air_gap(location=submerge_start_location)


Hm, I wasn't sure about this from reading your comment above: When you're puffing out the air here, is the tip inside the liquid or not?

(But also it might be nice to mention this _remove_air_gap() substep in the comment above.)

ddcc4 · 2025-01-06T19:58:41Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

            rate=1,
            flow_rate=flow_rate,
            in_place=True,
            is_meniscus=None,
            push_out=0,
        )
+        self._tip_state.remove_air_gap(last_air_gap)


Hm, do me a favor: It's getting a bit hard to keep track of which of the similarly-named functions do something physical vs. merely altering variables. Could you rename the functions in TipState to make them more distinguishable, and make it obvious that they're not doing anything physical? E.g., maybe TransferComponentsExecutor._remove_air_gap() vs TipState.delete_air_gap().

ddcc4 · 2025-01-06T20:05:55Z

api/src/opentrons/protocol_api/core/engine/transfer_components_executor.py

            self.aspirate_and_wait(volume=mix_properties.volume)
-            # TODO: Update to doing a push out at the end of mix for a post-dispense mix
-            self.dispense_and_wait(volume=mix_properties.volume, push_out=0)
+            if n == 1:


Adding onto this comment: Can you just shove the logic into the dispense_and_wait() expression? I.e.,

self.dispense_and_wait( ..., push_out_override=push_out_vol if last_dispense_push_out is True and n == 1 else 0 )

ddcc4 · 2025-01-06T21:54:37Z

api/src/opentrons/protocol_api/core/engine/instrument.py

+            - If dispense location is above the meniscus, DO NOT remove air gap
+              (it will be dispensed along with rest of the liquid later).
+              All other scenarios, remove the air gap by doing a dispense
+            - Flow rate = min(dispenseFlowRate, (airGapByVolume)/sec)


Hm, I don't understand this expression. What is sec here?

ddcc4 · 2025-01-06T21:55:45Z

api/src/opentrons/protocol_api/core/engine/instrument.py

+            - Flow rate = min(dispenseFlowRate, (airGapByVolume)/sec)
+            - Use the post-dispense delay
+        4. Move to the dispense position at the specified ‘submerge’ speed
+           (even if we might not be moving into the liquid)


Can you explain how this differs from Step 1 where you move to the submerge position?

ddcc4 · 2025-01-06T22:18:02Z

api/src/opentrons/protocol_api/core/engine/instrument.py

+        7. Delay
+        8. Mix using the same flow rate and delays as specified for asp+disp,
+           with the volume and the number of repetitions specified. Use the delays in asp & disp.
+            - If the dispense position is outside the liquid, then raise error if mix is enabled.


sanni-t commented Dec 18, 2024

View reviewed changes

sanni-t changed the base branch from AUTH-866-add-transfer-flow-builder to edge December 19, 2024 18:44

sanni-t added 2 commits December 20, 2024 01:19

added dispense steps and refactored some components executors

c7ffe85

linter errors

9b2de59

sanni-t force-pushed the AUTH-866-add-transfer-flow-builder-2 branch from c3cc384 to 9b2de59 Compare December 20, 2024 06:21

sanni-t changed the base branch from edge to AUTH-866-add-transfer-flow-builder December 20, 2024 06:21

sanni-t added 2 commits January 2, 2025 01:02

added tests

279a913

fixed validation test

bdfe17b

sanni-t marked this pull request as ready for review January 2, 2025 15:53

sanni-t requested a review from a team as a code owner January 2, 2025 15:53

sanni-t requested review from jbleon95 and ddcc4 January 2, 2025 15:53

jbleon95 approved these changes Jan 6, 2025

View reviewed changes

ddcc4 reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add execution of dispense steps for liquid class based transfer #17138

feat(api): add execution of dispense steps for liquid class based transfer #17138

sanni-t commented Dec 18, 2024 •

edited

Loading

sanni-t Dec 18, 2024

sanni-t Dec 18, 2024

jbleon95 left a comment

jbleon95 Jan 6, 2025

jbleon95 Jan 6, 2025

sanni-t Jan 6, 2025

jbleon95 Jan 6, 2025

ddcc4 Jan 6, 2025

jbleon95 Jan 6, 2025

sanni-t Jan 6, 2025

jbleon95 Jan 6, 2025

sanni-t Jan 6, 2025

jbleon95 Jan 6, 2025

sanni-t Jan 6, 2025

jbleon95 Jan 6, 2025

ddcc4 Jan 6, 2025

sanni-t Jan 6, 2025

ddcc4 Jan 6, 2025

sanni-t Jan 6, 2025

ddcc4 Jan 6, 2025

ddcc4 Jan 6, 2025

sanni-t Jan 6, 2025

ddcc4 Jan 6, 2025

ddcc4 Jan 6, 2025

ddcc4 Jan 6, 2025

ddcc4 Jan 6, 2025

ddcc4 Jan 6, 2025

ddcc4 Jan 6, 2025

feat(api): add execution of dispense steps for liquid class based transfer #17138

Are you sure you want to change the base?

feat(api): add execution of dispense steps for liquid class based transfer #17138

Conversation

sanni-t commented Dec 18, 2024 • edited Loading

Overview

Test Plan and Hands on Testing

Review requests

Risk assessment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbleon95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t commented Dec 18, 2024 •

edited

Loading