feat(api): add new InstrumentContext.transfer_liquid() method #16819

sanni-t · 2024-11-14T15:58:56Z

Overview

Adds InstrumentContext.transfer_liquid() method that does the following-

validates parameters of transfer_liquid()
loads the liquid class properties for the relevant pipette and tiprack into protocol engine
delegates to engine core to perform the actual transfer

This PR does not cover engine core's transfer method execution.

Test Plan and Hands on Testing

Since this is mostly adding the scaffolding to implement the actual execution of transfer method, this PR is not testable on the robot yet. Unit tests are crucial at this stage and have been added for all changes.

Changelog

adds InstrumentContext.transfer_liquid()
adds InstrumentCore.load_liquid_class() and a placeholder InstrumentCore.transfer_liquid()
adds validator methods and their tests to protocol_api.validation that get used in InstrumentContext.transfer_liquid()
adds as_schema_v1_model() to the liquid properties in order to fetch the pydantic model representation of the liquid class's properties. This is used by load_liquid_class() in order to create a liquidClassRecord
adds tests

Review requests

Do the validations make sense? Am I missing anything?
Does the transfer_liquid() interface look good?

Risk assessment

No risk so far since this is a code-only change.

…_liquid in all cores

…ers in cores, add tests

codecov · 2024-11-25T22:32:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.17%. Comparing base (d5b7e61) to head (b551782).
Report is 19 commits behind head on edge.

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #16819      +/-   ##
==========================================
+ Coverage   73.90%   79.17%   +5.26%     
==========================================
  Files          43      120      +77     
  Lines        3231     4514    +1283     
==========================================
+ Hits         2388     3574    +1186     
- Misses        843      940      +97

Flag	Coverage Δ
g-code-testing	`92.43% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 77 files with indirect coverage changes

jbleon95

Some minor requests but otherwise looks good for the initial transfer_liquid code

jbleon95 · 2024-11-27T15:54:23Z

api/src/opentrons/protocol_api/_liquid_properties.py

@@ -37,6 +41,10 @@ def as_dict(self) -> Dict[float, float]:
        """Get a dictionary representation of all set volumes and values along with the default."""
        return self._properties_by_volume

+    def as_list_of_tuples(self) -> List[Tuple[float, float]]:
+        """Get as list of tuples."""
+        return [(k, v) for k, v in self._properties_by_volume.items()]


this could be simplified to list(self._properties_by_volume.items())

jbleon95 · 2024-11-27T16:48:26Z

api/src/opentrons/protocol_engine/clients/sync_client.py

@@ -89,6 +89,12 @@ def execute_command_without_recovery(
    ) -> commands.TryLiquidProbeResult:
        pass

+    @overload
+    def execute_command_without_recovery(


Is this meant to be temporary until further transfer builder work? If so could there be a TODO here

No, it's not temporary. This just defining the overload signature that uses LoadLiquidClassParams and returns LoadLiquidClassResult. The actual execution is passed on to an existing function in this file.

The function right below actually

ddcc4

Hey, I got partway through this PR, and I'll continue looking at this later.

But this is my first time looking into a lot of these files, and I need to familiarize myself with them and the conventions we use.

ddcc4 · 2024-11-27T16:33:57Z

api/src/opentrons/protocol_api/_liquid_properties.py

@@ -37,6 +41,10 @@ def as_dict(self) -> Dict[float, float]:
        """Get a dictionary representation of all set volumes and values along with the default."""
        return self._properties_by_volume

+    def as_list_of_tuples(self) -> List[Tuple[float, float]]:
+        """Get as list of tuples."""
+        return [(k, v) for k, v in self._properties_by_volume.items()]


I think list(self._properties_by_volume.items()) will do what you want?

ddcc4 · 2024-11-27T16:44:15Z

api/src/opentrons/protocol_api/_liquid_properties.py

@@ -101,6 +109,14 @@ def duration(self, new_duration: float) -> None:
        validated_duration = validation.ensure_positive_float(new_duration)
        self._duration = validated_duration

+    def as_schema_v1_model(self) -> SharedDataDelayProperties:


Question on the versioning: Do you expect the v1 to stick around forever? Like, if we later create a v2 of the schema, are you going to have an as_schema_v2_model() function too? And how would that even work -- like, wouldn't you need

def as_schema_v1_model(self) -> SharedDataDelayPropertiesV1 ... def as_schema_v2_model(self) -> SharedDataDelayPropertiesV2 ...

What I'm getting at is: if this code is NOT expected to handle v2, v3, etc., the same way, then having v1 in all the function names feels like it's just adding clutter that won't really help make the code extensible for future versions. But is that just a convention we follow?

ddcc4 · 2024-11-27T17:08:40Z

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

+        liquid_class: LiquidClass,
+        pipette_load_name: str,
+        tiprack_uri: str,
+    ) -> str:


Hey, could you explain what Instrument is supposed to represent conceptually? Like, I think it makes sense that an instrument can aspirate or transferLiquid, but is loading liquid classes also something that belongs to the instrument?

ddcc4 · 2024-11-27T17:09:48Z

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

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


The implementation just treats pipetteModel as an opaque string, so it can be whatever you want :)

Ya, but these need to be consistent. We have two different ways to refer to pipettes- the API load name and the name used by rest of the system. The definition contains properties keyed by API load name and so when looking up values from the definition we have to use the API load name. But most of the get_name or get_model functions use the other name.

ddcc4 · 2024-11-27T17:11:42Z

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

@@ -309,6 +310,32 @@ def configure_nozzle_layout(
        """
        ...

+    @abstractmethod
+    def load_liquid_class(


Hm, and what's the difference between protocol_api/core/engine/instrument.py and protocol_api/core/instrument.py?

protocol_api/core/instrument.py has the outside-facing interfaces that the public context refers to. So just a bunch of abstractmethos declarations. The actual implementation of these methods is carried out by the three 'cores'- engine core/ legacy core/ legacy simulator core. The public context gets the relevant 'core' to use during initialization. All our new features go on the engine core.

ddcc4 · 2024-11-27T17:13:22Z

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

+        liquid_class_id: str,
+        volume: float,
+        source: List[WellCore],
+        dest: List[WellCore],


Did you say yesterday that source and dest are expected to be exactly the same length?

If so, maybe it's less error-prone if we made the user give us a single list of (source, dest) well pairs, rather than 2 lists?

It will just be more work for protocol authors to create a paired list. Most of the time users use things like source=labware1.columns()[0] and dest=labware2.columns()[0].

ddcc4 · 2024-11-27T17:15:50Z

api/src/opentrons/protocol_api/instrument_context.py

+            source
+        )
+        flat_dest_list = validation.ensure_valid_flat_wells_list_for_transfer_v2(dest)
+        for well in flat_sources_list + flat_dest_list:


Is there any restriction on how wacky the user can get with sources_list and dest_list? Like:

Can the same well appear multiple times in either list?

Can the same well appear in both source and dest?

Can you transfer from a well to the same well? (source=[A1, A2], dest=[A1, A2])

Yes to all above. And they will be valid transfers as far as the API is concerned. Whether it makes sense scientifically, we can't predict.

ddcc4 · 2024-11-27T17:16:54Z

api/src/opentrons/protocol_api/instrument_context.py

+        flat_sources_list = validation.ensure_valid_flat_wells_list_for_transfer_v2(
+            source
+        )
+        flat_dest_list = validation.ensure_valid_flat_wells_list_for_transfer_v2(dest)


Small nitpick: source and dest should both either be singular or plural, for symmetry.

ddcc4

Hey, I had some more small comments and questions, but overall this seems fine, so approving.

ddcc4 · 2024-11-27T18:26:13Z

api/src/opentrons/protocol_api/instrument_context.py

+                "A transfer on a liquid class cannot start with liquid already in the tip."
+                " Ensure that all previously aspirated liquid is dispensed before starting"
+                " a new transfer."
+            )


Hm, is this restriction necessary? There might be some cool mixes or dilutions you could do if you could start with something in the tip and then do a transfer.

ddcc4 · 2024-11-27T18:34:54Z

api/src/opentrons/protocol_api/validation.py

+        return [target]
+
+    if isinstance(target, list) or isinstance(target, tuple):
+        if isinstance(target[0], list) or isinstance(target[0], tuple):


Tip: isinstance takes multiple types, so you can simplify your code to:
if isinstance(blah, (list, tuple))

ddcc4 · 2024-11-27T18:36:32Z

api/src/opentrons/protocol_api/validation.py

+        return [target]
+
+    if isinstance(target, list) or isinstance(target, tuple):
+        if isinstance(target[0], list) or isinstance(target[0], tuple):


But I don't think you've checked that the lists are non-zero in length before, so this will crash if someone passes in target=[].

ddcc4 · 2024-11-27T18:39:37Z

api/src/opentrons/protocol_api/validation.py

+            f"If specified, location should be an instance of"
+            f" `types.Location` (e.g. the return value from `Well.top()`)"
+            f" or `Well` (e.g. `tiprack.wells()[0]`) or an instance of `TrashBin` or `WasteChute`."
+            f" However, it is {trash_location}."


Could you try triggering this error message to see what it prints? (You don't have to check in a test for it, I just want to make sure that you won't make the code crash when you try to stringify {trash_location} when the user passes in something unexpected.)

ddcc4 · 2024-11-27T18:42:56Z

api/src/opentrons/protocol_api/validation.py

+                " (for instance, as the result of a call to `Well.top()`),"
+                " it must be a location relative to a well,"
+                " since that is where a tip is dropped."
+                f" However, the given location refers to {trash_location.labware}"


What is trash_location.labware? And does it always have a printable value?

ddcc4 · 2024-11-27T18:45:27Z

api/src/opentrons/protocols/advanced_control/transfers/common.py

+class TransferTipPolicyV2(enum.Enum):
+    ONCE = "once"
+    NEVER = "never"
+    ALWAYS = "always"


Hm, in PD, the allowed options for single-path transfers are:

before every aspirate,

once at start of step,

per source well,

never

Do you know how "per source well" should be implemented with this API?

sanni-t added 4 commits November 13, 2024 15:31

added skeleton

57eff4d

add transfer_liquid args validation and tests

1b33bd2

more validation and use get_transfer_steps

298218d

add instr context tests

1ce8da6

sanni-t mentioned this pull request Nov 18, 2024

RFC: prototyping papi transfer + engine core + pe load liquid class #16602

Closed

sanni-t added 6 commits November 22, 2024 08:52

remove transfer builder, improve validation, add placeholder transfer…

bdfbc51

…_liquid in all cores

Merge branch 'edge' into AUTH-843-really-add-transfer-liquid-method

8a128b2

add schema v1 converters to api liquid class properties

4bc6960

add trash location validation and update transfer_liquid structure

a682ea5

add load_liquid_class() to engine core, add transfer_liquid placehold…

13b38f8

…ers in cores, add tests

Merge branch 'edge' into AUTH-843-really-add-transfer-liquid-method

b551782

sanni-t marked this pull request as ready for review November 25, 2024 23:04

sanni-t requested a review from a team as a code owner November 25, 2024 23:04

jbleon95 approved these changes Nov 27, 2024

View reviewed changes

ddcc4 reviewed Nov 27, 2024

View reviewed changes

ddcc4 approved these changes Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add new InstrumentContext.transfer_liquid() method #16819

feat(api): add new InstrumentContext.transfer_liquid() method #16819

sanni-t commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024

jbleon95 left a comment

jbleon95 Nov 27, 2024

jbleon95 Nov 27, 2024

sanni-t Nov 27, 2024

sanni-t Nov 27, 2024

ddcc4 left a comment

ddcc4 Nov 27, 2024

ddcc4 Nov 27, 2024

ddcc4 Nov 27, 2024

ddcc4 Nov 27, 2024

sanni-t Nov 27, 2024

ddcc4 Nov 27, 2024

sanni-t Nov 27, 2024

ddcc4 Nov 27, 2024

sanni-t Nov 27, 2024

ddcc4 Nov 27, 2024 •

edited

Loading

sanni-t Nov 27, 2024

ddcc4 Nov 27, 2024

ddcc4 left a comment

ddcc4 Nov 27, 2024

ddcc4 Nov 27, 2024

ddcc4 Nov 27, 2024

ddcc4 Nov 27, 2024

ddcc4 Nov 27, 2024

ddcc4 Nov 27, 2024

feat(api): add new InstrumentContext.transfer_liquid() method #16819

Are you sure you want to change the base?

feat(api): add new InstrumentContext.transfer_liquid() method #16819

Conversation

sanni-t commented Nov 14, 2024 • edited Loading

Overview

Test Plan and Hands on Testing

Changelog

Review requests

Risk assessment

codecov bot commented Nov 25, 2024

Codecov Report

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

ddcc4 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

ddcc4 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddcc4 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

sanni-t commented Nov 14, 2024 •

edited

Loading

ddcc4 Nov 27, 2024 •

edited

Loading