feat(api): implement the liquid transfer function #17179

sanni-t · 2025-01-02T16:07:00Z

Overview

Final PR of the 3-PR series.
Implements InstrumentCore.transfer_liquid().

The transfer_liquid() method does the following:

Loads the relevant liquid class record into protocol engine, gets the liquid class ID back. This will be used in command annotations in the future
Breaks down the total transfer volume into piecewise transfers, if necessary.
Does auto-tip handling, unless the user has specified to 'never' use a new tip.
Calls aspirate_liquid_class() and dispense_liquid_class() for every source-> destination transfer.

This PR also updates the tiprack 'names' to URIs in the liquid class definition in shared data for 'water' only. We will have a separate PR for updating rest of the definitions.

Test Plan and Hands on Testing

Added some unit tests for things that are unit-testable
Added integration tests for verifying that transfer_liquid() execution succeeds for various supported configurations
TODO: on-robot testing

Additionally, we will be adding analysis snapshot tests with protocols that use various configurations for transferring with liquid classes.

Review requests

The implementation of transfer_liquid() turned out to be not easily unit-testable, mainly owing to the location of implementation (inside InstrumentCore). I plan on moving this function out of the InstrumentCore so that it can be unit-tested without the constraints of rest of the Instrument core. But this still serves as a robust implementation for doing end-to-end, integration testing so that's what I am focusing on in this PR.
This PR also makes liquid-classes-based transfer available for hardware testing and verification.

Risk assessment

Low. Doesn't change existing API

ddcc4 · 2025-01-07T18:19:58Z

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

+            pip_api_name
+            for pip_api_name, pip_name in PIPETTE_API_NAMES_MAP.items()
+            if pip_name == pipette.pipetteName
+        ][0]


Small nitpick: This can be more efficiently implemented as:

load_name = next( (pip_api_name for pip_api_name, pip_name in PIPETTE_API_NAMES_MAP.items() if pip_name == pipette.pipetteName), None ) assert load_name return load_name

ddcc4 · 2025-01-07T18:20:29Z

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

@@ -883,7 +904,7 @@ def load_liquid_class(

        liquid_class_record = LiquidClassRecord(
            liquidClassName=liquid_class.name,
-            pipetteModel=self.get_model(),  # TODO: verify this is the correct 'model' to use
+            pipetteModel=pipette_load_name,  # TODO: verify this is the correct 'model' to use


Does comment still apply?

Nope. I'll remove it.

ddcc4 · 2025-01-07T18:24:54Z

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

-            pipette_load_name=self.get_pipette_name(),  # TODO: update this to use load name instead
-            tiprack_uri=tiprack_uri,
+            pipette_load_name=self.get_pipette_load_name(),
+            tiprack_uri=tiprack_uri_for_transfer_props,
        )
        transfer_props = liquid_class.get_for(


self.load_liquid_class() above also calls liquid_class.get_for(), right?

Can you refactor self.load_liquid_class() to just take in the transfer_props directly as an argument?

ddcc4 · 2025-01-07T18:36:34Z

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

@@ -1034,7 +1153,9 @@ def aspirate_liquid_class(
        )
        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
+        last_contents = components_executor.tip_state.last_liquid_and_air_gap_in_tip
+        tip_contents[-1] = last_contents


Hm, I'm a little confused what this is trying to do: it sometimes modifies the list that's passed in, and it sometimes modifies a list that was created local to this function?

Talked about it in person. The list that's passed in is not re-used by the caller function so it doesn't matter if the list gets modified here or not, since the list in the caller function gets replaced by ones returned by the aspirate_liquid_class and dispense_liquid_class. But I can see how it can cause confusion so I'll update this to use a copy of the list for any modifications.

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

        mock_instrument_core.air_gap_in_place(
            volume=air_gap_volume, flow_rate=air_gap_volume
-        ),
-        mock_instrument_core.delay(0.2),
+        )
+        if add_final_air_gap is True
+        else None,
+        mock_instrument_core.delay(0.2) if add_final_air_gap is True else None,


jbleon95

It all looks good to me, and tested this on the robot so we're ready to hand this off for hardware testing

ddcc4 · 2025-01-14T16:48:13Z

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

+        ]
+        for step_volume, src_dest_combo in source_dest_per_volume_step:
+            step_source, step_destination = src_dest_combo
+            is_last_step = len([source_dest_per_volume_step]) > 0


How does this work? Why does len([source_dest_per_volume_step]) > 0 mean it's the last step? And wouldn't the length always be > 0?

Oh shoot! I meant to fix this! Thanks for catching it!

ddcc4 · 2025-01-14T16:50:05Z

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

        # Neither aspirate nor a dispense process should be adding liquid
        # 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."
        self.last_liquid_and_air_gap_in_tip.liquid += volume

-    def remove_liquid(self, volume: float) -> None:
+    def delete_liquid(self, volume: float) -> None:
        # Neither aspirate nor a dispense process should be removing liquid
        # 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."
        self.last_liquid_and_air_gap_in_tip.liquid -= volume


Is it allowed to make last_liquid_and_air_gap_in_tip.liquid go below zero here?

Nope. I can add an assertion there to make sure we never remove more liquid than is present

ddcc4 · 2025-01-14T16:57:32Z

shared-data/python/opentrons_shared_data/pipette/types.py

+# Mapping of public Python Protocol API pipette load names
+# to names used by the internal Opentrons system
+# TODO (spp, 2025-01-02): make the API load name a part of the pipette's definition
+PIPETTE_API_NAMES_MAP = {


Is this same dict in api/src/opentrons/protocol_api/validation.py as well? (I also needed it for Python generation from PD -- see here.)

Yep. I've moved it to shared data for precisely this reason that PD will need to use it too

ddcc4 · 2025-01-14T22:33:54Z

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

+                    source_dest_per_volume_step
+                )
+            except StopIteration:
+                is_last_step = True


Is source_dest_per_volume_step an iterator or a list?
Can you turn it into a list?
If so, could you just enumerate over the list and check if you're at the index of the last item?

Like,

for idx, (next_step_volume, next_src_dest_combo) in enumerate(source_dest_per_volume_step): is_last_step = (idx == len(source_dest_per_volume_step) - 1) do stuff ..

It's an iterator.
Yes I can turn it into a list but that defeats the purpose of having expand_for_volume_constraints as a generator function that uses memory more efficiently. The iterator can have quite a number of elements on some occasions. So unless there's a compelling reason to not do it the way I have over here, I think we should avoid converting it to list.

Ah I see. In that case, I wouldn't do this inline in the code here -- it's too much logic to pack in here. Instead, I would define a helper function that implements another generator to wrap expand_for_volume_constraints() that includes whether we're at the last step.

But you also control expand_for_volume_constraints(), don't you? Could you just make it include is_last_step in the values that it yields?

…17244) - Functions should not modify any argument that's passed in (unless that's the purpose of the function). - Objects should not modify any arguments given to their constructor (unless the object is supposed to take ownership of the argument).

…tep if not using new tip

Argument unpacking to make `decoy.verify()` work with optional calls.

sanni-t changed the base branch from edge to AUTH-866-add-transfer-flow-builder-2 January 2, 2025 16:14

sanni-t marked this pull request as ready for review January 7, 2025 14:43

sanni-t requested review from a team as code owners January 7, 2025 14:43

sanni-t requested review from jbleon95 and ddcc4 and removed request for a team January 7, 2025 14:52

ddcc4 reviewed Jan 7, 2025

View reviewed changes

sanni-t commented Jan 13, 2025

View reviewed changes

jbleon95 approved these changes Jan 14, 2025

View reviewed changes

ddcc4 reviewed Jan 14, 2025

View reviewed changes

sanni-t force-pushed the AUTH-866-add-transfer-flow-builder-2 branch from bdfe17b to f1c2919 Compare January 15, 2025 16:27

sanni-t and others added 17 commits January 15, 2025 12:01

initial changes

585efdd

added transfer liquid implementation

8e7d08e

added a pipette api-load-name getter

c684fd0

code and type fixes

7b54c70

cleanup & fixes

41b9d6a

fixed small bugs

9ff1b83

updated tiprack names to URI names

dd4ac69

don't air gap if air gap volume is 0

43536b3

added an integration test

eda6c8c

address comments on PR-17138

b5f42d0

addressed comments

b84b544

added tests

6dd978d

include liquid classes directory in shared data bundle

761d2b8

fixed blowout location for source

e4b9d61

fixes for blowout and touch tip locations, dont add air gap in last s…

e5bce89

…tep if not using new tip

Silly Python syntax (#17256)

82b2fc8

Argument unpacking to make `decoy.verify()` work with optional calls.

sanni-t added 5 commits January 15, 2025 12:13

updated other tests with blowout

60dc7ae

formatting

a6d499f

fix validation test

0058516

fix the last step checker logic

6f3906a

resolved linter errors

3a1b24b

sanni-t force-pushed the AUTH-866-add-transfer-flow-builder-final branch from 7287185 to 3a1b24b Compare January 15, 2025 17:37

sanni-t changed the base branch from AUTH-866-add-transfer-flow-builder-2 to edge January 15, 2025 17:37

sanni-t merged commit eca564a into edge Jan 15, 2025
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): implement the liquid transfer function #17179

feat(api): implement the liquid transfer function #17179

sanni-t commented Jan 2, 2025 •

edited

Loading

ddcc4 Jan 7, 2025

ddcc4 Jan 7, 2025

sanni-t Jan 9, 2025

ddcc4 Jan 7, 2025

ddcc4 Jan 7, 2025

sanni-t Jan 9, 2025

This comment was marked as resolved.

jbleon95 left a comment

ddcc4 Jan 14, 2025

sanni-t Jan 14, 2025

ddcc4 Jan 14, 2025

sanni-t Jan 14, 2025

ddcc4 Jan 14, 2025

sanni-t Jan 14, 2025

ddcc4 Jan 14, 2025

sanni-t Jan 14, 2025

ddcc4 Jan 15, 2025

feat(api): implement the liquid transfer function #17179

feat(api): implement the liquid transfer function #17179

Conversation

sanni-t commented Jan 2, 2025 • 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

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

This comment was marked as resolved.

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

sanni-t commented Jan 2, 2025 •

edited

Loading