feat(api): add execution of aspirate steps in a liquid class based transfer #17092

sanni-t · 2024-12-12T16:00:52Z

Overview

Part 1 of three-part series of implementing transfer function.

This PR adds a TransferComponentsExecutor class that executes individual parts of the overarching 'aspiration' process. These components are-

Submerge
Aspirate and wait
Dispense and wait
Mix
Pre-wet
Add/remove air gap
Retract

The InstrumentCore gets a new method aspirate_liquid_class() which utilizes the above executor to execute the aspiration steps in a specific order. aspirate_liquid_class() will then be utilized by the InstrumentCore.transfer_liquid() method to perform aspiration during each transfer step.
This method can also be accessed in the protocol by using private API accessors for testing purposes.

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

I landed on this architecture as it allows us to unit test the transfer using different configurations of the liquid class easily.
It also allows us to build the transfer step by step, and very importantly, allows hardware testing using individual aspirate and dispense steps (instead of testing one big transfer all at once).

Still, open to improvement suggestions.

There are a few TODOs in the code so far. Most of them will be addressed by the last PR of this 3-part series.

Risk assessment

Low. Makes no changes to the existing code.

…mponentsExecutor executes individual components delegated by aspirate/ dispense

ddcc4 · 2024-12-13T16:11:25Z

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

+        )
+        if new_tip == TransferTipPolicyV2.ONCE:
+            # TODO: update this once getNextTip is implemented
+            self.get_next_tip()


Hey, does the upcoming get_next_tip() return the tip to use (stateless), or does it alter some internal state to indicate the tip it chose?

It returns the tip to use in a stateless way

ddcc4 · 2024-12-13T16:19:53Z

api/tests/opentrons/conftest.py

+
+
+@pytest.fixture
+def maximal_liquid_class_def() -> LiquidClassSchemaV1:


ddcc4 · 2024-12-13T16:33:01Z

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

+                # TODO: update this once getNextTip is implemented
+                self.get_next_tip()
+
+            # TODO: add aspirate and dispense


Does this just call the aspirate_liquid_class() function below?

aspirate_liquid_class and the upcoming dispense_liquid_class functions

ddcc4 · 2024-12-13T16:37:30Z

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

+            assert delay_props.duration is not None
+            self._instrument.delay(delay_props.duration)
+
+    def dispense_and_wait(self, volume: float, push_out: Optional[float]) -> None:


So this PR primarily implements aspirate, which would not use functions like dispense_and_wait(), right? This is just here for illustration?

Edit: Oh wait, maybe you do use it because mix() is part of aspirate? Can you move this function below mix() so that the functions are arranged in roughly the order that they would be called during a transfer, so that a reader can read this file top-down and follow the program flow?

ddcc4 · 2024-12-13T16:45:47Z

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

@@ -38,6 +42,7 @@
 )
 from opentrons.protocol_api._nozzle_layout import NozzleLayout
 from . import overlap_versions, pipette_movement_conflict
+from . import transfer_components_executor as tx_comps_executor


Hehe, this name is really too long. Maybe just transfer_executor?

But it doesn't even behave like the traditional meaning of an executor, which is a thing where you submit high-level tasks to and it decides when and how to run the tasks -- here, you the caller are the one choosing when and how to invoke the functions in the executor.

I hate the name helper, but this file really is just a transfer_helper. Hm ...

Ya I struggled with the name..

As someone who dislikes the name helper almost as much as manager for classes, I agree that transfer_helper does appear to be the most straightforward, simplest, while still descriptive name for what this class is using

Cool, helper it is then

ddcc4 · 2024-12-13T16:50:52Z

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

+        )
+        aspirate_location = Location(aspirate_point, labware=source_loc.labware)
+
+        components_executer = tx_comps_executor.get_transfer_components_executor(


Hm, I'm not a fan of this get_...() function. It makes it look like you're calling a method on an object named tx_comps_executor. (I was confused because you had renamed the file in your import, and I didn't realize that you had done that, so I assumed tx_comps_executor was a variable name.)

If get_transfer_components_executor() doesn't do anything besides call the TransferComponentsExecutor constructor, can you just invoke the constructor directly here, to make it more obvious what's going on?

I had to add the get..() function in order to create a mock of the TransferComponentsExecutor in the tests.

Oh, also, another thought was to lru_cache to cache the TransferComponentsExecutor instance so only one instance would be created when the same liquid class and (source well -> dest-well) combo is being used. The getter would allow us to do that.

I had to add the get..() function in order to create a mock of the TransferComponentsExecutor in the tests.

Hm, is there a way to mock out the class itself? (In my past projects, we used unittest.mock instead of PyTest, which let you mock the class constructor itself, so that you wouldn't need to create get_ function just for testing.)

We can mock out the class but because the class is not injected into the core and rather just instantiated in the aspirate/ dispense methods, unittest.mock will be insufficient. But correct me if I'm wrong.

I believe we should be able to monkeypatch this out of the unit tests using the existing pattern we have, especially if we instantiate it like tx_comps_executor. TransferComponentsExecutor (or whatever we rename it to)

ddcc4 · 2024-12-13T16:56:08Z

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

+        )
+        aspirate_location = Location(aspirate_point, labware=source_loc.labware)
+
+        components_executer = tx_comps_executor.get_transfer_components_executor(


But secondly, I think the only reason that TransferComponentsExecutor is a class at all is to save you the trouble of passing in arguments like aspirate_location and source_well to the method calls every time, right?

TransferComponentsExecutor doesn't really have any state of its own and it doesn't manage any state, right? In that case, I'm not sure it deserves to be a class. How bad would it be to make transfer_components_executor.py just a flat file of stateless helper functions, and to pass in the necessary arguments on every call?

Ya, that's another decision I struggled with. This class originally contained the aspirate_liquid_class and dispense_liquid_class implementations too and was built to manage state between aspirates and dispenses since we do have to keep track of air gaps between the aspirates and dispenses (and some more stuff when implementing consolidate and distribute).

But then we needed to have aspirate_liquid_class and dispense_liquid_class available in the core (or some way to call the aspirate process and dispense process separately instead of in a big transfer) because of two reasons-

It's necessary for hardware testing since they need to run test scripts after each aspirate/ dispense process. With these in the instrument core, they can access them using private methods in their scripts.

We anticipate needing a public API for these liquid-class-based aspirate and dispense processes in the near future. At which point we will face this question again.

So I had to move a lot of things around to facilitate this and wasn't fully satisfied with the final architecture.
Splitting the TransferComponentsExecutor into individual functions will make the arguments list for each of these functions quite long but it's not a bad option.

ddcc4 · 2024-12-13T17:48:56Z

Hey, so I don't know if you're planning to give TransferComponentsExecutor any internal state that it needs to manage on its own. But from the code so far in this PR, it looks like TransferComponentsExecutor's only reason for existing is so that we can split out some of the transfer-related functions so that we don't have to dump them all into InstrumentCore.

I think one typical way to do that is to define a mixin. So you would have something like:

class InstrumentCore(TransferHelperMixin):
  # The fields here are shared between this parent class and the mixin, e.g.:
  def move_to(): ...


class TransferHelperMixin:
  def submerge():
    ...
    self.move_to(...)
    ...

Depending on the type checker we're using, you may need to do a bit more work to tell the type checker that TransferHelperMixin has access to everything in InstrumentCore when it's mixed-in.

sanni-t · 2024-12-16T16:44:14Z

Hey, so I don't know if you're planning to give TransferComponentsExecutor any internal state that it needs to manage on its own. But from the code so far in this PR, it looks like TransferComponentsExecutor's only reason for existing is so that we can split out some of the transfer-related functions so that we don't have to dump them all into InstrumentCore.

Hmm, so some internal state does get added for dispense and the overarching transfer implementation. But it might not have to be internal to the TransferComponentsExecutor.
Ok, I'll do this- I'll put up the PRs that build the dispense and full transfer on top of the architecture of this PR so that will give us the full picture and we can decide if we want to change the arch to have Mixins

jbleon95 · 2024-12-17T16:29:04Z

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

+        assert (
+            mix_properties.repetitions is not None and mix_properties.volume is not None
+        )
+        for n in range(mix_properties.repetitions):


Suggested change

for n in range(mix_properties.repetitions):

for _ in range(mix_properties.repetitions):

The n gets used in the dispense implementation so going to leave this as is

jbleon95 · 2024-12-17T16:32:22Z

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

+            self._instrument.touch_tip(
+                location=retract_location,
+                well_core=self._target_well,
+                radius=0,


Suggested change

radius=0,

radius=1.0,

jbleon95 · 2024-12-17T16:37:52Z

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

+
+    def _add_air_gap(self, air_gap_volume: float) -> None:
+        """Add an air gap."""
+        aspirate_props = self._transfer_properties.aspirate


It seems a little inconsistent to be referring to self._transfer_properties here while also taking in a value that is retrieved from the self-same transfer properties. I'd almost prefer the arguments to this (and this pattern could be extended for these sub steps) to be air_gap_by_volume, flow_rate_by_volume, and delay properties, so the only self access is _instrument

Ya, I've renamed them .._overrides in the next part. But I'll try out your suggestion too

jbleon95 · 2024-12-17T16:42:28Z

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

+        if new_tip == TransferTipPolicyV2.ONCE:
+            # TODO: update this once getNextTip is implemented
+            self.get_next_tip()
+        for step_volume, (src, dest) in source_dest_per_volume_step:  # type: ignore[assignment]


This is more of a personal style thing, but this statement seems messy, aesthetically, with the tuple being unpacked from a tuple and the type ignore. I don't know if there's a better way to represent this, but maybe it could look cleaner by doing the second tuple unpacking elsewhere (and naming them source and destination, I sort of dislike the shortening of these variables)

Ya the transfer method is a work in progress. It's mostly there for showing how the aspirate and dispense methods will be used in the transfer. I'll address this in the last PR.

jbleon95 · 2024-12-17T16:43:23Z

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

@@ -38,6 +42,7 @@
 )
 from opentrons.protocol_api._nozzle_layout import NozzleLayout
 from . import overlap_versions, pipette_movement_conflict
+from . import transfer_components_executor as tx_comps_executor


As someone who dislikes the name helper almost as much as manager for classes, I agree that transfer_helper does appear to be the most straightforward, simplest, while still descriptive name for what this class is using

jbleon95 · 2024-12-17T16:45:39Z

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

+        )
+        aspirate_location = Location(aspirate_point, labware=source_loc.labware)
+
+        components_executer = tx_comps_executor.get_transfer_components_executor(


I believe we should be able to monkeypatch this out of the unit tests using the existing pattern we have, especially if we instantiate it like tx_comps_executor. TransferComponentsExecutor (or whatever we rename it to)

sanni-t added 11 commits December 10, 2024 20:31

added aspirate executor

a143a91

refactor- aspirate orchestration moved to instrument core, TransferCo…

27f114f

…mponentsExecutor executes individual components delegated by aspirate/ dispense

use tuples of location and well core

07c7b10

fixes after last change

6e1fbed

linter and misc fixes

dba4f40

updated and added tests

c74e318

added more tests

dabff5a

remove dispense stuff

aa189fa

add aspirate test

1851696

Merge branch 'edge' into AUTH-866-add-transfer-flow-builder

eb00776

docstrings and comments cleanup

fdb1b0b

sanni-t marked this pull request as ready for review December 12, 2024 21:41

sanni-t requested a review from a team as a code owner December 12, 2024 21:41

sanni-t requested review from jbleon95 and ddcc4 December 12, 2024 21:41

ddcc4 reviewed Dec 13, 2024

View reviewed changes

jbleon95 reviewed Dec 17, 2024

View reviewed changes

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

monketpatch the class instead of getter

14ffa19

Merge branch 'edge' into AUTH-866-add-transfer-flow-builder

3e7005c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add execution of aspirate steps in a liquid class based transfer #17092

feat(api): add execution of aspirate steps in a liquid class based transfer #17092

sanni-t commented Dec 12, 2024 •

edited

Loading

ddcc4 Dec 13, 2024

sanni-t Dec 13, 2024

ddcc4 Dec 13, 2024

ddcc4 Dec 13, 2024

sanni-t Dec 13, 2024

ddcc4 Dec 13, 2024

ddcc4 Dec 13, 2024

sanni-t Dec 13, 2024

jbleon95 Dec 17, 2024

sanni-t Dec 17, 2024

ddcc4 Dec 13, 2024

sanni-t Dec 13, 2024

sanni-t Dec 13, 2024 •

edited

Loading

ddcc4 Dec 13, 2024

sanni-t Dec 16, 2024

jbleon95 Dec 17, 2024

ddcc4 Dec 13, 2024

sanni-t Dec 13, 2024

ddcc4 commented Dec 13, 2024

sanni-t commented Dec 16, 2024

jbleon95 Dec 17, 2024

sanni-t Dec 17, 2024

jbleon95 Dec 17, 2024

jbleon95 Dec 17, 2024

sanni-t Dec 17, 2024

jbleon95 Dec 17, 2024

sanni-t Dec 17, 2024

jbleon95 Dec 17, 2024

jbleon95 Dec 17, 2024



		@pytest.fixture
		def maximal_liquid_class_def() -> LiquidClassSchemaV1:

	for n in range(mix_properties.repetitions):
	for _ in range(mix_properties.repetitions):

feat(api): add execution of aspirate steps in a liquid class based transfer #17092

Are you sure you want to change the base?

feat(api): add execution of aspirate steps in a liquid class based transfer #17092

Conversation

sanni-t commented Dec 12, 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

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 Dec 13, 2024 • edited Loading

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 commented Dec 13, 2024

sanni-t commented Dec 16, 2024

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 12, 2024 •

edited

Loading

sanni-t Dec 13, 2024 •

edited

Loading