Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Jan 15, 2025
139 changes: 132 additions & 7 deletions api/src/opentrons/protocol_api/core/engine/instrument.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@

from __future__ import annotations

from typing import Optional, TYPE_CHECKING, cast, Union, List
from typing import Optional, TYPE_CHECKING, cast, Union, List, Tuple
from opentrons.types import Location, Mount, NozzleConfigurationType, NozzleMapInterface
from opentrons.hardware_control import SyncHardwareAPI
from opentrons.hardware_control.dev_types import PipetteDict
from opentrons.protocols.api_support.util import FlowRates, find_value_for_api_version
from opentrons.protocols.api_support.types import APIVersion
from opentrons.protocols.advanced_control.transfers.common import TransferTipPolicyV2
from opentrons.protocols.advanced_control.transfers.common import (
TransferTipPolicyV2,
check_valid_volume_parameters,
expand_for_volume_constraints,
)
from opentrons.protocol_engine import commands as cmd
from opentrons.protocol_engine import (
DeckPoint,
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I struggled with the name..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, helper it is then


from .well import WellCore
from ..instrument import AbstractInstrument
Expand All @@ -46,6 +51,7 @@
if TYPE_CHECKING:
from .protocol import ProtocolCore
from opentrons.protocol_api._liquid import LiquidClass
from opentrons.protocol_api._liquid_properties import TransferProperties

_DISPENSE_VOLUME_VALIDATION_ADDED_IN = APIVersion(2, 17)

Expand Down Expand Up @@ -892,16 +898,131 @@ def load_liquid_class(
)
return result.liquidClassId

# TODO: update with getNextTip implementation
def get_next_tip(self) -> None:
"""Get the next tip to pick up."""

def transfer_liquid(
self,
liquid_class_id: str,
liquid_class: LiquidClass,
volume: float,
source: List[WellCore],
dest: List[WellCore],
source: List[Tuple[Location, WellCore]],
dest: List[Tuple[Location, WellCore]],
new_tip: TransferTipPolicyV2,
trash_location: Union[WellCore, Location, TrashBin, WasteChute],
tiprack_uri: str,
tip_drop_location: Union[WellCore, Location, TrashBin, WasteChute],
) -> None:
"""Execute transfer using liquid class properties.

Args:
liquid_class: The liquid class to use for transfer properties.
volume: Volume to transfer per well.
source: List of source wells, with each well represented as a tuple of
types.Location and WellCore.
types.Location is only necessary for saving the last accessed location.
dest: List of destination wells, with each well represented as a tuple of
types.Location and WellCore.
types.Location is only necessary for saving the last accessed location.
new_tip: Whether the transfer should use a new tip 'once', 'never', 'always',
or 'per source'.
tiprack_uri: The URI of the tiprack that the transfer settings are for.
tip_drop_location: Location where the tip will be dropped (if appropriate).
"""
# This function is WIP
# TODO: use the ID returned by load_liquid_class in command annotations
self.load_liquid_class(
liquid_class=liquid_class,
pipette_load_name=self.get_pipette_name(), # TODO: update this to use load name instead
tiprack_uri=tiprack_uri,
)
transfer_props = liquid_class.get_for(
# update this to fetch load name instead
pipette=self.get_pipette_name(),
tiprack=tiprack_uri,
)
aspirate_props = transfer_props.aspirate

check_valid_volume_parameters(
disposal_volume=0, # No disposal volume for 1-to-1 transfer
air_gap=aspirate_props.retract.air_gap_by_volume.get_for_volume(volume),
max_volume=self.get_max_volume(),
)
source_dest_per_volume_step = expand_for_volume_constraints(
volumes=[volume for _ in range(len(source))],
targets=zip(source, dest),
max_volume=self.get_max_volume(),
)
if new_tip == TransferTipPolicyV2.ONCE:
# TODO: update this once getNextTip is implemented
self.get_next_tip()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It returns the tip to use in a stateless way

for step_volume, (src, dest) in source_dest_per_volume_step: # type: ignore[assignment]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

# TODO: add aspirate and dispense
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this just call the aspirate_liquid_class() function below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aspirate_liquid_class and the upcoming dispense_liquid_class functions


if new_tip == TransferTipPolicyV2.ALWAYS:
if isinstance(tip_drop_location, (TrashBin, WasteChute)):
self.drop_tip_in_disposal_location(
disposal_location=tip_drop_location,
home_after=False,
alternate_tip_drop=True,
)
elif isinstance(tip_drop_location, Location):
self.drop_tip(
location=tip_drop_location,
well_core=tip_drop_location.labware.as_well()._core, # type: ignore[arg-type]
home_after=False,
alternate_drop_location=True,
)

def aspirate_liquid_class(
self,
volume: float,
source: Tuple[Location, WellCore],
transfer_properties: TransferProperties,
) -> None:
"""Execute transfer using liquid class properties."""
"""Execute aspiration steps.

1. Submerge
2. Mix
3. pre-wet
4. Aspirate
5. Delay- wait inside the liquid
6. Aspirate retract
"""
aspirate_props = transfer_properties.aspirate
source_loc, source_well = source
aspirate_point = (
tx_comps_executor.absolute_point_from_position_reference_and_offset(
well=source_well,
position_reference=aspirate_props.position_reference,
offset=aspirate_props.offset,
)
)
aspirate_location = Location(aspirate_point, labware=source_loc.labware)

components_executer = tx_comps_executor.get_transfer_components_executor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@sanni-t sanni-t Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-

  1. 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.
  2. 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.

instrument_core=self,
transfer_properties=transfer_properties,
target_location=aspirate_location,
target_well=source_well,
)
components_executer.submerge(
submerge_properties=aspirate_props.submerge,
# Assuming aspirate is not called with *liquid* in the tip
# TODO: evaluate if using the current volume to find air gap is not a good idea.
air_gap_volume=self.get_current_volume(),
)
# TODO: when aspirating for consolidation, do not perform mix
components_executer.mix(mix_properties=aspirate_props.mix)
# TODO: when aspirating for consolidation, do not preform pre-wet
components_executer.pre_wet(
volume=volume,
)
components_executer.aspirate_and_wait(volume=volume)
components_executer.retract_after_aspiration(volume=volume)

def retract(self) -> None:
"""Retract this instrument to the top of the gantry."""
Expand Down Expand Up @@ -994,3 +1115,7 @@ def nozzle_configuration_valid_for_lld(self) -> bool:
return self._engine_client.state.pipettes.get_nozzle_configuration_supports_lld(
self.pipette_id
)

def delay(self, seconds: float) -> None:
"""Call a protocol delay."""
self._protocol_core.delay(seconds=seconds, msg=None)
Loading
Loading