feat(api): add command to configure pipette volume in Protocol Engine #13473

Laura-Danielle · 2023-09-06T16:36:43Z

Overview

Add the configure for liquid class command to protocol engine and allow virtual configs to return different liquid class configurations.

Changelog

Add new command to configure liquid for volume
Add a new equipment command that will configure the volume in the hardware controller and update the pipette state store
Update the hardware controller to again have a command to configure liquid for volume (a copy of work @sfoster1 had previously done)

codecov · 2023-09-06T16:39:48Z

Codecov Report

Merging #13473 (df0b79b) into low-volume-pipetting (e9243b1) will increase coverage by 0.03%.
The diff coverage is 69.23%.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           low-volume-pipetting   #13473      +/-   ##
========================================================
+ Coverage                 71.35%   71.38%   +0.03%     
========================================================
  Files                      2418     2433      +15     
  Lines                     67903    68064     +161     
  Branches                   7876     7919      +43     
========================================================
+ Hits                      48452    48589     +137     
- Misses                    17609    17616       +7     
- Partials                   1842     1859      +17

Flag	Coverage Δ
app	`69.05% <ø> (+0.17%)`	⬆️
g-code-testing	`96.44% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`
protocol-designer	`46.09% <ø> (ø)`
shared-data	`74.26% <69.23%> (-0.03%)`	⬇️
step-generation	`87.18% <ø> (ø)`

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

Files Changed	Coverage Δ
api/src/opentrons/hardware_control/ot3api.py	`79.26% <ø> (ø)`
.../src/opentrons/protocol_engine/actions/__init__.py	`100.00% <ø> (ø)`
...i/src/opentrons/protocol_engine/actions/actions.py	`100.00% <ø> (ø)`
...src/opentrons/protocol_engine/commands/__init__.py	`100.00% <ø> (ø)`
.../src/opentrons/protocol_engine/commands/command.py	`94.11% <ø> (-0.17%)`	⬇️
...entrons/protocol_engine/commands/command_unions.py	`100.00% <ø> (ø)`
...opentrons/protocol_engine/commands/load_pipette.py	`100.00% <ø> (ø)`
...rc/opentrons/protocol_engine/execution/__init__.py	`100.00% <ø> (ø)`
...rons/protocol_engine/execution/command_executor.py	`100.00% <ø> (ø)`
...c/opentrons/protocol_engine/execution/equipment.py	`100.00% <ø> (ø)`
... and 4 more

... and 40 files with indirect coverage changes

sfoster1

I don't think this is quite what we want. We want an action that changes the liquid class by sending over the new name, and is handled by calling the set_liquid_class method in hardware and then updating the loaded pipette config from hardware (or calling an equivalent function when simulating).

sfoster1 · 2023-09-06T18:25:30Z

api/src/opentrons/protocol_engine/actions/actions.py

+ """Update a pipette's current static config class based on a new liquid class."""
+
+ pipette_id: str
+ serial_number: str


the serial number definitely shouldn't be in here - that would make it impossible for the action to exist in a json protocol that won't know the serial number

sfoster1 · 2023-09-06T18:25:57Z

api/src/opentrons/protocol_engine/actions/actions.py

+
+ pipette_id: str
+ serial_number: str
+ config: pipette_data_provider.LoadedStaticPipetteData


This also doesn't seem right - the payload here should be the liquid class name and not much else. That will get executed by being passed down to the hardware.

sfoster1

Looks like exactly what we want, save the copypasta in the command implementation, sharing the logic, and fixing that one hysteresis logic defn

sfoster1 · 2023-09-07T18:31:57Z

api/src/opentrons/hardware_control/instruments/ot3/pipette.py

+ if volume > (self.liquid_class.max_volume * 0.75):
+ return pip_types.LiquidClasses.default.name
+ if self._liquid_class_name == pip_types.LiquidClasses.default:
+ if volume < (self.liquid_class.max_volume * 0.5):


I think this wants to be < than half the max volume of the low volume default

sfoster1 · 2023-09-07T18:33:36Z

api/src/opentrons/protocol_engine/execution/equipment.py

+ Returns:
+ A LoadedConfiguredVolumeData object.
+ """
+ # TODO (spp, 2023-05-10): either raise error if using MountType.EXTENSION in


sfoster1 · 2023-09-07T18:34:11Z

api/src/opentrons/protocol_engine/execution/equipment.py

+ except RuntimeError as e:
+ raise FailedToLoadPipetteError(str(e)) from e
+
+ pipette_dict = self._hardware_api.get_attached_instrument(


don't need to do this

sfoster1 · 2023-09-07T18:35:50Z

api/src/opentrons/protocol_engine/execution/equipment.py

+ # behavior of protocol_context.load_instrument, and is used here as a
+ # pipette existence check
+ try:
+ await self._hardware_api.cache_instruments(cache_request)


don't need this

sfoster1 · 2023-09-07T18:36:01Z

api/src/opentrons/protocol_engine/execution/equipment.py

+ # is only to support protocol analysis, since the hardware simulator
+ # does not cache requested virtual instruments. Remove per
+ # https://opentrons.atlassian.net/browse/RLIQ-258
+ other_mount = mount.other_mount()


don't need the other mount and cache pipette stuff

sfoster1 · 2023-09-07T18:37:05Z

api/src/opentrons/protocol_engine/execution/equipment.py

+ is_p50_flex_pipette = (
+ "p50" in pipette_name_value and "flex" in pipette_name_value
+ )
+ if volume < 5 and is_p50_flex_pipette:


we should use the exact same logic - maybe make liquid_class_for_volume a free function taking

the requested volume

the current liquid class
and reutrning a liquid class, that can get called both here and in hardware?

TamarZanzouri · 2023-09-07T18:42:38Z

api/src/opentrons/protocol_engine/execution/equipment.py

+ use_virtual_pipettes = self._state_store.config.use_virtual_pipettes
+
+ pipette_name_value = (
+ pipette_name.value
+ if isinstance(pipette_name, PipetteNameType)
+ else pipette_name
+ )
+
+ pipette_id = pipette_id or self._model_utils.generate_id()
+
+ if not use_virtual_pipettes:
+ cache_request = {mount.to_hw_mount(): pipette_name_value}
+
+ # TODO(mc, 2022-12-09): putting the other pipette in the cache request
+ # is only to support protocol analysis, since the hardware simulator
+ # does not cache requested virtual instruments. Remove per
+ # https://opentrons.atlassian.net/browse/RLIQ-258
+ other_mount = mount.other_mount()
+ other_pipette = self._state_store.pipettes.get_by_mount(other_mount)
+ if other_pipette is not None:
+ cache_request[other_mount.to_hw_mount()] = (
+ other_pipette.pipetteName.value
+ if isinstance(other_pipette.pipetteName, PipetteNameType)
+ else other_pipette.pipetteName


as we spoke about in the morning call. I guess we can extract this logic to a method bc we are doing the same for the load_pipette?

we don't need to do this logic in here at all I think

Rather than importing the concept of liquid classes to pipette state, we should just add a command to update the static configs from the user facing python API.

This way it can be shared between hardware and the virtual pipette data provider. Went back and forth on where to put this but ultimately this only depends on direct inputs and values from configuration, so shared-data it is.

Unfortunately, the virtual pipette data provider now must be stateful because it's emulating something stateful, and we should not just move the state to state because it would be doubling for unavoidable hardware state like the position of the pipette, and hardware controller really wants to run this. That said, we can now nicely use the exact same logic for the virtual pipette mirror as the actual hardware control. Needs new tests but doesn't break old ones.

TamarZanzouri · 2023-09-08T13:52:10Z

api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py

+ pip = self.get_pipette(mount)
+ if pip.current_volume > 0:
+ # Switching liquid classes can't happen when there's already liquid
+ return


do we want to raise here instead?

yeah probably

TamarZanzouri

thank you for the changes! this looks great to me!

When PE loaded pipettes, it would set the static config in its state by having the load method of the equipment handler side-effect emit a special action that only existed to update the configuration. This worked ok as long as you weren't too picky about wanting to be sure that the action was handled before the result of the command was evaluated. The problem is that now we do care about that, a lot (though it's not quite as evident in this commit). When we switch pickup and drop tip to reset the tip state of a pipette, we're going to need to set the configuration using this new private result method and rely on the results in the command results handler. This is implemented by poking a new typevar into CommandUpdateAction that can optionally carry a "private result", something that will not be exposed via HTTP or via the database but can carry information in a lifetime-synchronous way to the result. This required a lot of refactoring, but I think it's ended up pretty cleanly; the only test changes are directly required by the changed code.

TamarZanzouri · 2023-09-11T14:47:19Z

api/src/opentrons/protocol_engine/commands/command_unions.py

@@ -500,3 +514,5 @@
 calibration.CalibrateModuleResult,
 calibration.MoveToMaintenancePositionResult,
 ]
+
+CommandPrivateResult = Union[None, LoadPipettePrivateResult]


are we missing ConfigureForVolumePrivateResult ?

TamarZanzouri · 2023-09-11T15:48:32Z

api/src/opentrons/protocol_engine/state/pipettes.py

- tip_configuration = static_config.tip_configuration_lookup_table[
- static_config.max_volume
- ]
+ tip_configuration = list(


do we want to leave a comment about defaulting?

TamarZanzouri · 2023-09-11T15:55:36Z

looks great to me! had a few questions but overall love the private setting change :-)

jbleon95

Reviewed the Protocol Engine portions of this PR, all look good to me outside of minor comments and fixing up the failing CI tests. Really like the private result addition, we can definitely use that in the future to clean up some result cruft.

jbleon95 · 2023-09-11T15:26:30Z

api/src/opentrons/protocol_engine/commands/command.py

+ ABC,
+ Generic[CommandParamsT, CommandResultT, CommandPrivateResultT],
+):
+ """Abstract command creation and execution implementation.


Doc strings for this new Abstract Implementation should be updated to not just be copypasta'd from above.

api/src/opentrons/protocol_engine/commands/configure_for_volume.py

jbleon95 · 2023-09-11T15:44:42Z

api/src/opentrons/protocol_engine/execution/equipment.py

+ """Ensure the requested volume can be configured for the given pipette.
+
+ Args:
+ pipette_id: An optional identifier to assign the pipette. If None, an


Not optional for this method

jbleon95 · 2023-09-11T15:55:35Z

api/src/opentrons/protocol_engine/execution/equipment.py

+ pipette_id, volume, model
+ )
+
+ serial_number = self._model_utils.generate_id(prefix="fake-serial-number-")


the fact that we are regenerating serial number here for the simulated case feels a bit iffy even though in practice I don't believe it will have any actionable impact on how we do analysis. The only place we are using serial number is for get_calibrated_tip_length which already is referencing a fake serial number in the first place. Still just calling it out here for maybe a comment or not explaining this.

jbleon95 · 2023-09-11T16:01:15Z

api/tests/opentrons/protocol_engine/execution/test_equipment_handler.py

 ) -> None:
 """Mock out move_types.py functions."""
 for name, func in inspect.getmembers(pipette_data_provider, inspect.isfunction):
 monkeypatch.setattr(pipette_data_provider, name, decoy.mock(func=func))
+ monkeypatch.setattr(


Since VirtualPipetteDataProvider is now a class we should avoid monkeypatching it here and instead make the below pytest fixture a decoy mock of that class, unless there is some functional reason why we are doing it this way.

Laura-Danielle

Cannot approve my own pull request but I like the private results update a lot.

SyntaxColoring

Generally looks great to me. Thanks!

api/src/opentrons/protocol_engine/commands/configure_for_volume.py

api/src/opentrons/protocol_engine/commands/command.py

api/src/opentrons/protocol_engine/commands/load_pipette.py

sfoster1

get rid of my own negative review

sfoster1 · 2023-09-08T13:43:26Z

api/src/opentrons/protocol_engine/commands/configure_for_volume.py

+class ConfigureForVolumeResult(BaseModel):
+ """Result data from execution of an ConfigureForVolume command."""
+
+ pipetteId: str = Field(


we don't really, might remove it

sfoster1 · 2023-09-11T17:32:34Z

api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py

+ pip = self.get_pipette(mount)
+ if pip.current_volume > 0:
+ # Switching liquid classes can't happen when there's already liquid
+ return


yeah probably

Co-authored-by: Max Marrone <[email protected]>

…#13473) Adds a new protocol engine command ConfigureForLiquidVolume that calls the hardware control method configure_for_liquid_volume (though in this protocol, there is no upward interface other than direct command injection). In addition, this PR has a major refactor of PE command execution to support "private results" - values that can come from a command that do not have to be (and in fact cannot be and are not) serialized, whether to persistent storage or over the wire to clients - they're held entirely internally. This functionality lets us replace side-effect emitting actions and therefore get rid of PipetteConfigAction, which only existed so loading pipettes could go update pipette config data. Instead, that's now a private result, both of LoadPipette and of the new action. Finally, this PR makes the virtual pipette data provider stateful because it's emulating something stateful, and we should not just move the state to state because it would be doubling for unavoidable hardware state like the position of the pipette, and hardware controller really wants to run this. Co-authored-by: Max Marrone <[email protected]> Co-authored-by: Seth Foster <[email protected]>

Laura-Danielle requested review from a team as code owners September 6, 2023 16:36

sfoster1 force-pushed the rss-313-add-configure-for-volume branch from 71b0b00 to e189e6d Compare September 6, 2023 17:29

sfoster1 force-pushed the configure-liquid-class-protocol-engine branch from af0ec52 to 47b291e Compare September 6, 2023 18:24

sfoster1 requested changes Sep 6, 2023

View reviewed changes

sfoster1 force-pushed the rss-313-add-configure-for-volume branch from 431ac83 to cb6f5bc Compare September 6, 2023 18:50

sfoster1 force-pushed the configure-liquid-class-protocol-engine branch from 47b291e to 7971f28 Compare September 6, 2023 18:50

sfoster1 force-pushed the rss-313-add-configure-for-volume branch from cb6f5bc to 0f39436 Compare September 7, 2023 14:22

Base automatically changed from rss-313-add-configure-for-volume to low-volume-pipetting September 7, 2023 14:53

sfoster1 force-pushed the configure-liquid-class-protocol-engine branch from 7971f28 to 60dfbea Compare September 7, 2023 15:23

Laura-Danielle requested a review from sfoster1 September 7, 2023 18:01

sfoster1 requested changes Sep 7, 2023

View reviewed changes

TamarZanzouri reviewed Sep 7, 2023

View reviewed changes

Laura-Danielle and others added 8 commits September 8, 2023 09:42

feat(api): add command to configure liquid class in Protocol Engine

5e8eca6

Rather than importing the concept of liquid classes to pipette state, we should just add a command to update the static configs from the user facing python API.

do not add a liquid class action

59d7dca

add protocol engine command to configure liquid

ddb2445

re-add configure for volume so it can be called from PE

f1ad4de

refactor: get_liquid_class_for_volume to shared-data

adc4e57

This way it can be shared between hardware and the virtual pipette data provider. Went back and forth on where to put this but ultimately this only depends on direct inputs and values from configuration, so shared-data it is.

format shared-data

7c5a6f9

add command to unions and update schema

1f57a2b

sfoster1 force-pushed the configure-liquid-class-protocol-engine branch from eaffb51 to 1f57a2b Compare September 8, 2023 13:42

TamarZanzouri reviewed Sep 8, 2023

View reviewed changes

TamarZanzouri approved these changes Sep 8, 2023

View reviewed changes

Laura-Danielle and others added 2 commits September 8, 2023 18:43

add more test coverage for virtual pip config and new shared data func

18c21a2

Laura-Danielle changed the title ~~Configure liquid class protocol engine~~ feat(api): add command to configure pipette volume in Protocol Engine Sep 11, 2023

TamarZanzouri reviewed Sep 11, 2023

View reviewed changes

load tip return data from tip volume

de051e2

TamarZanzouri reviewed Sep 11, 2023

View reviewed changes

jbleon95 approved these changes Sep 11, 2023

View reviewed changes

Laura-Danielle commented Sep 11, 2023

View reviewed changes

brilliant

44aeaa2

sfoster1 self-requested a review September 11, 2023 17:31

sfoster1 added 2 commits September 11, 2023 14:05

format pipette-definition

63728a7

remove pipette_id from cfv result and change comments

e66a62c

SyntaxColoring reviewed Sep 11, 2023

View reviewed changes

api/src/opentrons/protocol_engine/commands/configure_for_volume.py Outdated Show resolved Hide resolved

api/src/opentrons/protocol_engine/commands/command.py Outdated Show resolved Hide resolved

api/src/opentrons/protocol_engine/commands/load_pipette.py Show resolved Hide resolved

some backups for serial number handling

e77de3b

sfoster1 approved these changes Sep 11, 2023

View reviewed changes

sfoster1 and others added 5 commits September 11, 2023 15:29

Update api/src/opentrons/protocol_engine/commands/load_pipette.py

2490ef8

Co-authored-by: Max Marrone <[email protected]>

format, comments

43fdb1a

docstring

12dd580

test mock fixups

7210ca6

remove pydantic from private results

df0b79b

sfoster1 merged commit 7448a1a into low-volume-pipetting Sep 11, 2023
42 of 46 checks passed

sfoster1 deleted the configure-liquid-class-protocol-engine branch September 11, 2023 20:25

SyntaxColoring mentioned this pull request Apr 18, 2024

refactor(api): Delete unused action_dispatcher argument #14954

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add command to configure pipette volume in Protocol Engine #13473

feat(api): add command to configure pipette volume in Protocol Engine #13473

Laura-Danielle commented Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading

sfoster1 left a comment

sfoster1 Sep 6, 2023

sfoster1 Sep 6, 2023

sfoster1 left a comment

sfoster1 Sep 7, 2023

sfoster1 Sep 7, 2023

sfoster1 Sep 7, 2023

sfoster1 Sep 7, 2023

sfoster1 Sep 7, 2023

sfoster1 Sep 7, 2023

TamarZanzouri Sep 7, 2023

sfoster1 Sep 7, 2023

TamarZanzouri Sep 8, 2023

sfoster1 Sep 11, 2023

TamarZanzouri left a comment

TamarZanzouri Sep 11, 2023

sfoster1 Sep 11, 2023

TamarZanzouri Sep 11, 2023

TamarZanzouri commented Sep 11, 2023

jbleon95 left a comment

jbleon95 Sep 11, 2023

jbleon95 Sep 11, 2023

jbleon95 Sep 11, 2023

jbleon95 Sep 11, 2023

Laura-Danielle left a comment

SyntaxColoring left a comment

sfoster1 left a comment

sfoster1 Sep 8, 2023

sfoster1 Sep 11, 2023

feat(api): add command to configure pipette volume in Protocol Engine #13473

feat(api): add command to configure pipette volume in Protocol Engine #13473

Conversation

Laura-Danielle commented Sep 6, 2023 • edited Loading

Overview

Changelog

codecov bot commented Sep 6, 2023 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfoster1 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

Choose a reason for hiding this comment

TamarZanzouri 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

TamarZanzouri commented Sep 11, 2023

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

Laura-Danielle left a comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laura-Danielle commented Sep 6, 2023 •

edited

Loading

codecov bot commented Sep 6, 2023 •

edited

Loading