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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
414 changes: 412 additions & 2 deletions api/src/opentrons/protocol_api/core/engine/instrument.py
View file
Open in desktop

This comment was marked as outdated.

Sign in to view

Large diffs are not rendered by default.

40 changes: 37 additions & 3 deletions api/src/opentrons/protocol_api/instrument_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from __future__ import annotations
import logging
from contextlib import ExitStack
from typing import Any, List, Optional, Sequence, Union, cast, Dict
from typing import Any, List, Optional, Sequence, Union, cast, Dict, Literal, overload
from opentrons.protocol_engine.errors.exceptions import TipNotAttachedError
from opentrons_shared_data.errors.exceptions import (
CommandPreconditionViolated,
Expand Down Expand Up @@ -32,12 +32,12 @@

from .core.common import InstrumentCore, ProtocolCore
from .core.engine import ENGINE_CORE_API_VERSION
from .core.engine.instrument import SingleTransfer, PickUpTipInfo, DropTipInfo
from .core.legacy.legacy_instrument_core import LegacyInstrumentCore
from .config import Clearances
from .disposal_locations import TrashBin, WasteChute
from ._nozzle_layout import NozzleLayout
from . import labware, validation

from . import labware, validation, LiquidClass

AdvancedLiquidHandling = transfers.AdvancedLiquidHandling

Expand Down Expand Up @@ -1279,6 +1279,40 @@ def consolidate(

return self.transfer(volume, source, dest, **kwargs)

@overload
def transfer(
self,
liquid_class: LiquidClass,
sanni-t marked this conversation as resolved.
Show resolved Hide resolved
volume: Union[float, Sequence[float]],
source: AdvancedLiquidHandling,
dest: AdvancedLiquidHandling,
trash_location: Union[types.Location, TrashBin, WasteChute] = True,
new_tip: Literal["once", "never", "always"] = "once",
):
"""Transfer liquid from source to dest using the specified liquid class properties."""
# 1. Figure out tip locations to pick up from
# 2. Disambiguate src and dest locations
# 3. Wrap the above info in a dataclass that stores 1-to-1 transfer info

transfers = [
SingleTransfer(
pick_up_tip=PickUpTipInfo(pick_up_new=True, tip_location=tip_loc1),
source_well=src_well_location1,
dest_well=dest_well_location1,
drop_tip=DropTipInfo(drop_tip=True, location=trash_location),
),
SingleTransfer(...),
...,
]

# This is for same volume aspirate-dispense pairs
self._core.transfer(
liquid_class=liquid_class,
volume=volume,
transfers=transfers,
)

@overload
@publisher.publish(command=cmds.transfer)
@requires_version(2, 0)
def transfer( # noqa: C901
Expand Down
14 changes: 14 additions & 0 deletions api/src/opentrons/protocol_engine/commands/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,14 @@
TryLiquidProbeCommandType,
)

from .load_liquid_class import (
LoadLiquidClass,
LoadLiquidClassParams,
LoadLiquidClassResult,
LoadLiquidClassCreate,
LoadLiquidClassCommandType,
)

__all__ = [
# command type unions
"Command",
Expand Down Expand Up @@ -591,4 +599,10 @@
"TryLiquidProbeCreate",
"TryLiquidProbeResult",
"TryLiquidProbeCommandType",
# Load liquid class command bundle
"LoadLiquidClass",
"LoadLiquidClassParams",
"LoadLiquidClassResult",
"LoadLiquidClassCreate",
"LoadLiquidClassCommandType",
]
12 changes: 12 additions & 0 deletions api/src/opentrons/protocol_engine/commands/command_unions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,14 @@
TryLiquidProbeCommandType,
)

from .load_liquid_class import (
LoadLiquidClass,
LoadLiquidClassParams,
LoadLiquidClassResult,
LoadLiquidClassCreate,
LoadLiquidClassCommandType,
)

Command = Annotated[
Union[
Aspirate,
Expand All @@ -339,6 +347,7 @@
LoadLabware,
ReloadLabware,
LoadLiquid,
LoadLiquidClass,
LoadModule,
LoadPipette,
MoveLabware,
Expand Down Expand Up @@ -416,6 +425,7 @@
LoadLabwareParams,
ReloadLabwareParams,
LoadLiquidParams,
LoadLiquidClassParams,
LoadModuleParams,
LoadPipetteParams,
MoveLabwareParams,
Expand Down Expand Up @@ -491,6 +501,7 @@
LoadLabwareCommandType,
ReloadLabwareCommandType,
LoadLiquidCommandType,
LoadLiquidClassCommandType,
LoadModuleCommandType,
LoadPipetteCommandType,
MoveLabwareCommandType,
Expand Down Expand Up @@ -644,6 +655,7 @@
LoadLabwareResult,
ReloadLabwareResult,
LoadLiquidResult,
LoadLiquidClassResult,
LoadModuleResult,
LoadPipetteResult,
MoveLabwareResult,
Expand Down
111 changes: 111 additions & 0 deletions api/src/opentrons/protocol_engine/commands/load_liquid_class.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
"""Load liquid class command request, result, and implementation models."""
from __future__ import annotations

from opentrons_shared_data.liquid_classes.liquid_class_definition import (
AspirateProperties,
SingleDispenseProperties,
MultiDispenseProperties,
)
from pydantic import BaseModel, Field
from typing import Optional, Type, Dict, TYPE_CHECKING
from typing_extensions import Literal

from .command import AbstractCommandImpl, BaseCommand, BaseCommandCreate, SuccessData
from ..errors.error_occurrence import ErrorOccurrence
from ..types import ImmutableLiquidClass

if TYPE_CHECKING:
from ..state.state import StateView

LoadLiquidClassCommandType = Literal["loadLiquidClass"]


class LoadLiquidClassParams(BaseModel):
"""Payload required to load a liquid into a well."""

pipetteId: str = Field(
Copy link
Member

Choose a reason for hiding this comment

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

having the pipette id and tiprack id in here doesn't seem right to me. It means the command orchestration layer has to do a bunch of extra work to load the full liquid class and tie it back to a possibly-generated ID for the relevant pipette. It seems better to have the command take in the full liquid class, and then later when the command orchestration layer references the LC in a command like aspirate the engine is the one to verify that the LC has properties for the pipette specified by ID in the aspirate and the tip that pipette is using and pulls the right kind of properties.

Also is tiprackId an engine instance ID? or is it a labware URI? If it's an instance ID, wouldn't you have to load a huge number of liquid classes?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're totally right. Going to change this to accept the liquid class as is, and the engine then saves only the properties related to the pipette ID & tiprack uri in question. I want the engine to save only relevant parts of the liquid class so that the run summary doesn't get bloated because of parts of the liquid class that will be irrelevant not just to the transfer in question but the entire run. This will be definitely useful when handling built-in liquid classes which will have properties corresponding to all pipette+tiprack combos we support.

Copy link
Member

Choose a reason for hiding this comment

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

Even trimming out unnecessary stuff strikes me as a bit of a premature optimization. IMO if we're going to put the data in the engine, we should put the entire data in the engine. Sure, the liquid classes are big, but you're gonna be using like 1-10ish of them per protocol, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, unlike other definitions like labware and modules, liquid class properties are going to be mutable. If the engine were to save the entire liquid class, then every time there's a change to the properties for a specific pipette+tip combination, the snapshots of the entire liquid class will need to be updated. If we save split properties per pipette+tip combination then only the snapshot of the specific combo will need to be updated.

But I understand your point about pre-mature optimization; maybe the optimization above isn't worth the code bloat. I'll change this to saving complete liquid class and if we run into the above issue more often than we'd like to then I'll add the optimization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @sfoster1, I don't know if saving the entire LiquidClass is a good idea, and trying to save the whole thing is making me struggle with how to represent the data in the PE.

So LiquidClass starts out with the RECOMMENDED settings for every possible pipette and tip type, right? So it looks something like:

LiquidClass {
  - pipette1 + tip2 -> TransferProperties(flowrate=5, ...)
  - pipette2 + tip3 -> TransferProperties(flowrate=6, ...)
  - etc ...
}

But for any given transfer, the user can customize the settings. So the user's code is going to look something like this:

my_liquid_class = ...retrieve LiquidClass for viscous liquids...
transfer_settings = my_liquid_class.get_for(pipette2, tip3) # the particular pipette/tip we're using

# Transfer #1
transfer_settings.blah.flowrate = 7 # customize the flow rate
transfer_liquid(..., liquid_class=my_liquid_class)

# Transfer #2
transfer_settings.blah.flowrate = 8 # customize the flow rate differently
transfer_liquid(..., liquid_class=my_liquid_class)

Inside the implementation of transfer_liquid(), Sanniti is going to upload the settings for that transfer onto the PE. In her original proposal, she would upload something like this to record the settings we actually used:

# Transfer #1:
pipette=pipette2, tip=tip3, flowrate=7
# Transfer #2:
pipette=pipette2, tip=tip3, flowrate=8

With your proposal to store the entire LiquidClass, Sanniti would have to upload the settings for every possible pipette/tip for every transfer, even though only one of the pipettes is used for each transfer. Semantically, I don't know what it means to store the settings for the all pipettes we did NOT use (especially since we would probably have modified the settings if we HAD used those pipettes instead).

And in terms of implementation, if we have to store the entire LiquidClass, but we also want to know what settings were used for each particular transfer, then Sanniti wants me to build a fancy mechanism to compare the LiquidClasses from transfer to transfer and assign a new UUID to each distinct version of the LiquidClass we used (AUTH-851). It might make my life easier if we only had to store the settings we actually used for each transfer like in the original proposal here.

...,
description="Unique identifier of pipette to use with liquid class.",
)
tiprackId: str = Field(
...,
description="Unique identifier of tip rack to use with liquid class.",
)
aspirateProperties: AspirateProperties
singleDispenseProperties: SingleDispenseProperties
multiDispenseProperties: Optional[MultiDispenseProperties]
liquidClassId: Optional[str] = Field(
None,
description="An optional ID to assign to this liquid class. If None, an ID "
"will be generated.",
)


class LoadLiquidClassResult(BaseModel):
"""Result data from the execution of a LoadLiquidClass command."""

loadedLiquidClass: ImmutableLiquidClass = Field(
..., description="An immutable liquid class created from the load params."
)


class LoadLiquidClassImplementation(
AbstractCommandImpl[LoadLiquidClassParams, SuccessData[LoadLiquidClassResult, None]]
):
"""Load liquid command implementation."""

def __init__(self, state_view: StateView, **kwargs: object) -> None:
self._state_view = state_view

async def execute(
self, params: LoadLiquidClassParams
) -> SuccessData[LoadLiquidClassResult, None]:
"""Load data necessary for a liquid class."""

liq_class_hash = "create-a-hash"
existing_liq_class = self._state_view.liquid.get_liq_class_by_hash(
liq_class_hash
)
if existing_liq_class:
liq_class = existing_liq_class
else:
liq_class = ImmutableLiquidClass(
id="get-a-uuid",
hash="create-a-hash",
pipetteId=params.pipetteId,
tiprackId=params.tiprackId,
aspirateProperties=params.aspirateProperties,
singleDispenseProperties=params.singleDispenseProperties,
multiDispenseProperties=params.multiDispenseProperties,
)
# **** TODO: save liquid class to state *****

return SuccessData(
public=LoadLiquidClassResult(
loadedLiquidClass=liq_class,
),
private=None,
)


class LoadLiquidClass(
BaseCommand[LoadLiquidClassParams, LoadLiquidClassResult, ErrorOccurrence]
):
"""Load liquid command resource model."""

commandType: LoadLiquidClassCommandType = "loadLiquidClass"
params: LoadLiquidClassParams
result: Optional[LoadLiquidClassResult]

_ImplementationCls: Type[
LoadLiquidClassImplementation
] = LoadLiquidClassImplementation


class LoadLiquidClassCreate(BaseCommandCreate[LoadLiquidClassParams]):
"""Load liquid command creation request."""

commandType: LoadLiquidClassCommandType = "loadLiquidClass"
params: LoadLiquidClassParams

_CommandCls: Type[LoadLiquidClass] = LoadLiquidClass
8 changes: 6 additions & 2 deletions api/src/opentrons/protocol_engine/state/liquids.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""Basic liquid data state and store."""
from dataclasses import dataclass
from typing import Dict, List
from opentrons.protocol_engine.types import Liquid
from opentrons.protocol_engine.types import Liquid, ImmutableLiquidClass

from ._abstract_store import HasState, HandlesActions
from ..actions import Action, AddLiquidAction
Expand All @@ -13,6 +13,7 @@ class LiquidState:
"""State of all loaded liquids."""

liquids_by_id: Dict[str, Liquid]
liquid_classes_by_id: Dict[str, ImmutableLiquidClass]


class LiquidStore(HasState[LiquidState], HandlesActions):
Expand All @@ -22,7 +23,10 @@ class LiquidStore(HasState[LiquidState], HandlesActions):

def __init__(self) -> None:
"""Initialize a liquid store and its state."""
self._state = LiquidState(liquids_by_id={})
self._state = LiquidState(
liquids_by_id={},
liquid_classes_by_id={},
)

def handle_action(self, action: Action) -> None:
"""Modify state in reaction to an action."""
Expand Down
20 changes: 19 additions & 1 deletion api/src/opentrons/protocol_engine/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@
LabwareUri as LabwareUri,
)
from opentrons_shared_data.module.types import ModuleType as SharedDataModuleType

from opentrons_shared_data.liquid_classes.liquid_class_definition import (
AspirateProperties,
SingleDispenseProperties,
MultiDispenseProperties,
)

# todo(mm, 2024-06-24): This monolithic status field is getting to be a bit much.
# We should consider splitting this up into multiple fields.
Expand Down Expand Up @@ -1135,3 +1139,17 @@ class CSVParameter(RTPBase):


ABSMeasureMode = Literal["single", "multi"]


# TODO: add frozen dataclass of all properties
# These properties can skip the 'enable' prop and just have params as optional.
# If the params exist, the action is performed, if not, it's not performed.
@dataclass(frozen=True)
class ImmutableLiquidClass:
id: str
hash: str
pipetteId: str
tiprackId: str
aspirateProperties: AspirateProperties
singleDispenseProperties: SingleDispenseProperties
multiDispenseProperties: MultiDispenseProperties
Loading