feat(api): Support V2 pipette configs in the OT-2 and Protocol Engine #13104

Laura-Danielle · 2023-07-14T14:47:47Z

Overview

Well..uh I am sorry in advance. This PR touches a lot of code. The most important things to check out are the changes to pipette_data_provider.py and ot2/pipette.py. There are few small changes in the shared-data/python code that could also use some eyes. Luckily some of our older tests caught some mistakes in the pipette configs which was great (and hence why there are a lot of shared-data data changes).

I think I'll still need to do one more follow-up PR in Protocol Engine to support dynamically changing fallback tip length during run-time but I decided not to do that for this PR.

To-Do

There is one more failing test related to supporting "old" and "new aspirate functions on super old pipettes. I haven't yet decided how to handle this, but it would not block folks from beginning to test the PR so I decided to just put this up for review.

Test Plan

This PR needs to be thoroughly tested on both an OT-2 and Flex. It touches a lot of core functionality including:

pipette quirks
pipette backwards compatibility
tip length lookup
return tip height
Aspiration functions
Current changing

At a minimum we need to test on both an OT2/Flex:

Attaching pipettes
Loading pipettes in a protocol (with both old and new Flex names)
Analyzing a protocol
Running a protocol on the robot
Simulating a protocol in the App

Specific to OT-2

another quick check of mutable configs on the OT2

Changelog

Lots of small shared-data data changes (sorry about that)
Update OT-2 configs to use new shared-data pipette configs
Update Protocol Engine to grab data from the V2 configs
Additional helper functions in shared-data to support the two points above
Small fix to mutable configurations

Review requests

Please look closely at changes to ot2/pipette and pipette_data_provision.py. Make sure I did not miss any critical components in Protocol Engine for loading pipette configs.

Risk assessment

High. This PR affects a critical part of the stack. Please see the test plan above.

codecov · 2023-07-14T14:51:24Z

Codecov Report

Merging #13104 (7919afd) into edge (fc2ffdb) will decrease coverage by 11.84%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             edge   #13104       +/-   ##
===========================================
- Coverage   72.08%   60.24%   -11.84%     
===========================================
  Files        1530      795      -735     
  Lines       50741    16173    -34568     
  Branches     3144     2892      -252     
===========================================
- Hits        36576     9744    -26832     
+ Misses      13657     5963     -7694     
+ Partials      508      466       -42

Flag	Coverage Δ
api	`?`
app	`43.93% <ø> (ø)`
components	`?`
g-code-testing	`96.44% <ø> (ø)`
hardware	`?`
hardware-testing	`∅ <ø> (∅)`
notify-server	`89.13% <ø> (ø)`
ot3-gravimetric-test	`?`
protocol-designer	`47.29% <ø> (ø)`
robot-server	`?`
shared-data	`78.40% <ø> (-0.95%)`	⬇️
step-generation	`87.17% <ø> (ø)`
system-server	`?`
update-server	`?`
usb-bridge	`?`

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

Impacted Files	Coverage Δ
.../python/opentrons_shared_data/pipette/load_data.py	`92.30% <ø> (-5.61%)`	⬇️
...n/opentrons_shared_data/pipette/model_constants.py	`100.00% <ø> (ø)`
...rons_shared_data/pipette/mutable_configurations.py	`91.24% <ø> (-0.07%)`	⬇️
...pentrons_shared_data/pipette/pipette_definition.py	`94.78% <ø> (-2.28%)`	⬇️
...ared_data/pipette/pipette_load_name_conversions.py	`86.25% <ø> (+1.54%)`	⬆️
...data/python/opentrons_shared_data/pipette/types.py	`96.11% <ø> (-1.89%)`	⬇️

... and 807 files with indirect coverage changes

sfoster1

I think this is gonna be rough but important to do, but I think there's some things we can do to make it a bit less of a bitter pill to swallow.

Couple inline questions and some larger stuff:

subtle but probably important distinctions

I don't quite get the difference between pipette.active_tip_settings.default_return_height and

small changes, lots and lots of small consequences

There's a lot of changes that are sometimes almost the only changes in the file that are the consequence of a little type change that we can avoid by changing the new types or changing the upstreams. For instance, if you define opentrons_shared_data.pipette.pipette_definitions.PipetteChannelType as PipetteChannelType(int, Enum) then you get int comparisons for free:

Python 3.7.17 (default, Jun 27 2023, 10:52:02)
[Clang 14.0.3 (clang-1403.0.22.14.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from enum import Enum
>>> class PipetteChannelType(int, Enum):
...  SINGLE_CHANNEL = 1
...  EIGHT_CHANNEL = 8
...  NINETY_SIX_CHANNEL = 96
...
>>> PipetteChannelType.SINGLE_CHANNEL
<PipetteChannelType.SINGLE_CHANNEL: 1>
>>> PipetteChannelType.SINGLE_CHANNEL ==1
True
>>> PipetteChannelType.SINGLE_CHANNEL == 8
False
>>> PipetteChannelType.EIGHT_CHANNEL == 8
True
>>> PipetteChannelType.EIGHT_CHANNEL == 1
False

Combine that, maybe a custom comparison operator (if it's even needed, may get that for free too) to sort them, and some handling for the PipetteInfo model in the old calibration stuff to build its channels element from the enum, and a lot of the changes in robot_server.robot.calibration fall away.

This seems like not so big a win, but one of the reasons prs like this are tough to deal with is that git(hub) has no way of emphasizing or separating parts of the diff, and so stuff that's complete boilerplate like adding as_int everywhere is presented on the same level as stuff that's more important. Let's cut down on some of those boilerplate changes.

sfoster1 · 2023-07-14T16:48:15Z

api/src/opentrons/hardware_control/backends/simulator.py

            raise KeyError(
                "If you specify attached_instruments, the model "
                "should be pipette names or pipette models, but "
                f'{passed_ai["model"]} is not'
            )

        self._attached_instruments = {
-            m: _sanitize_attached_instrument(attached_instruments.get(m))
+            m: _sanitize_attached_instrument(attached_instruments.get(m))  # type: ignore


what's this suppressing?

Just moved the #type: ignore from the internal function to have that be slightly better typed. It's suppressing a TypeDict type.

sfoster1 · 2023-07-14T17:01:04Z

api/src/opentrons/hardware_control/instruments/instrument_abc.py

+
+
+# TODO (lc 07-05-2023) Move this back to the combined Pipette object file.
+def piecewise_volume_conversion(


this really should not be here. we should avoid touching it if we possibly can and if we can't we should just move it once

okay. I'll leave them both in their respective pipette.py files.

sfoster1 · 2023-07-14T17:03:43Z

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

+            self._config.supported_tips[list(self._config.supported_tips.keys())[0]],
+        )
+        self._fallback_tip_length = self._active_tip_settings.default_tip_length
+        self._aspirate_flow_rates_lookup = (


i'm suspicious of these parentheses I think there might be a syntax problem here

I think this is what the formatter did but I'll double check.

Oh okay, I just see that a lot when there's a missing comma or somethign so the formatter groups statements wrong

sfoster1 · 2023-07-14T17:06:05Z

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

@@ -302,6 +428,10 @@ def working_volume(self) -> float:
    def working_volume(self, tip_volume: float) -> None:
        """The working volume is the current tip max volume"""
        self._working_volume = min(self.config.max_volume, tip_volume)
+        self._active_tip_settings = self._config.supported_tips[
+            PipetteTipType(int(self._working_volume))


do we definitely have configurations for all the pipettes for all the working volumes they use, including the ot2 filter tips?

Will double check

sfoster1 · 2023-07-14T17:37:03Z

api/src/opentrons/protocol_engine/resources/pipette_data_provider.py

-        return_tip_scale=config.return_tip_height,
-        nominal_tip_overlap=config.tip_overlap,
+        return_tip_scale=tip_configuration.default_return_tip_height,
+        nominal_tip_overlap=config.tip_overlap_dictionary,


don't you have to modify get_pipette_static_config too?

No because I kept the old names in the pipette dict as keys.

sfoster1 · 2023-07-14T17:54:55Z

shared-data/python/opentrons_shared_data/pipette/load_data.py

@@ -105,6 +106,69 @@ def load_serial_lookup_table() -> Dict[str, str]:
    return _lookup_table


+def load_liquid_model(
+    max_volume: PipetteModelType,


i don't love this match of argname and argtype. The point of this pr is to continue to tease apart the overloaded concepts that were smushed into pipette names and models into structured data; this conflates two of them again. let's call it model or something and get the max volume from the model def.

Not sure what you're asking for here. Do you just want me to call the max_volume, model instead? I guess it ultimately doesn't matter to me. I would also make the change in the load_data function in this file since it's using the same nomenclature.

Yeah that makes sense to me

sfoster1 · 2023-07-14T17:55:25Z

shared-data/python/opentrons_shared_data/pipette/load_data.py

+    return PipetteLiquidPropertiesDefinition.parse_obj(liquid_dict)
+
+
+def _change_to_camel_case(c: str) -> str:


sfoster1 · 2023-07-14T17:55:45Z

shared-data/python/opentrons_shared_data/pipette/load_data.py

+
+
+def _change_to_camel_case(c: str) -> str:
+    # Tiny helper function to convert to camelCase.


i would almost infinitely prefer an explicit map of old to new keys

sigh Okayyyy

sfoster1 · 2023-07-14T17:56:30Z

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

@@ -88,20 +93,65 @@ def as_tuple(
        return (self.major, self.minor)


+# TODO (lc 12-5-2022) Ideally we can deprecate this


honestly i think this sort of class is entirely reasonable

I suppose it could be, but ideally we move away from this being an external concept entirely. You truly should be able to request a certain pipette size/channels without having to worry about generation of the pipette.

I suppose it could be, but ideally we move away from this being an external concept entirely. You truly should be able to request a certain pipette size/channels without having to worry about generation of the pipette.

Hmm. Even in a world where protocol authors request pipettes like that, it seems like this concept would still exist in HTTP. Something would still need to tell the app what pipettes the user needs to attach in order to run a protocol, and the app would still need to determine whether the pipettes that are already attached are a match.

Or is your vision that those would get reported as a fine-grained list of models? Like: "You can attach a p1000_single_1_0, or p1000_single_1_3, or p1000_single_1_4, or ..." etc.

In my ideal world, the app shouldn't have to determine if pipettes match at all or not. The robot server/protocol engine should just say 'hey this doesn't match what's currently on the robot'.

For simulation, we should have something that sends back a list of compatible generations (like we kinda talked about in the partial tip meeting). We haven't -- and probably won't in the future -- cared about the functionality between models.

sfoster1 · 2023-07-14T18:00:08Z

api/src/opentrons/hardware_control/api.py

@@ -555,14 +559,16 @@ async def _do_plunger_home(
                await stack.enter_async_context(self._motion_lock)
            with self._backend.save_current():
                self._backend.set_active_current(
-                    {checked_axis: instr.config.plunger_current}
+                    {checked_axis: instr.plunger_motor_current.run}


honestly wondering if, just for the sake of this pr, we mapped at least some of the new values to the old names. most of the time we wouldn't use them, but it would prevent us from needing a million changes that have no semantic meaning but just need different letters in sequence

in the config itself? Otherwise if we mapped it in the Pipette class there would still be a million changes. Plus mapping in a Pydantic model will get confusing since it's supposed to server as a schema.

sfoster1

Okay, this structure looks good to me. We obviously need to fix up tests and such, and run some protocols on a machine with this, but in particular we should pay close attention to anything going on with hardware testing - they poke into internals a lot and I'm a bit worried.

SyntaxColoring

Makin' my way downtown. I still have a way left to go, but here are some questions so far, and a couple requests. 🕺

shared-data/pipette/schemas/2/pipetteLiquidPropertiesSchema.json

shared-data/pipette/definitions/2/liquid/single_channel/p50/1_4.json

SyntaxColoring · 2023-07-17T15:19:09Z

shared-data/pipette/schemas/2/pipetteLiquidPropertiesSchema.json

+    "defaultTipOverlapDictionary": {
+      "type": "object",
+      "description": "Map of tip overlap values with defaults",
+      "required": ["default"],
+      "$comment": "Other keys in here should be labware URIs",
+      "properties": {
+        "default": { "type": "number" }
+      },
+      "additionalProperties": { "type": "number" }
+    },


Hmm, what makes tip overlap different from flow rate, tip length, and tip height? Tip overlap is specified here as a dict that's keyed on labware URIs, but those other things are specified as a dict that's keyed on a "tip type" like "t300."

It was was previously used. I made it match the V1 schema for sake of not touching a bunch more code. Protocol Engine and other users of this data still pass in LabwareUri to get this data.

The tip length imo should also eventually be removed from the schema all together because it doesn't make sense to be there. It is not a property of a pipette but rather a property of a tip rack.

You can think of the top level t... keys as being notation for representing all tips under the 300ul volume which may infact have different properties. That is why right now we're keying the aspirate/dispense by labware uri as well. Unfortunately don't have a fully fleshed out future yet for Liquid classes but you can imagine this type of stuff expanding when we get there.

SyntaxColoring · 2023-07-17T15:52:46Z

shared-data/python/opentrons_shared_data/pipette/load_data.py

+    return f"{config_name[0]}" + "".join(s.capitalize() for s in config_name[1::])
+
+
+def update_pipette_configuration(


You might just not have gotten to this yet, but this function seems like a high-value target for unit test coverage, since it has a type-unsafe Dict[str, Any] parameter and it's doing legacy name mapping and stuff.

yep, thanks for the reminder though :D

shared-data/python/opentrons_shared_data/pipette/load_data.py

shared-data/python/opentrons_shared_data/pipette/model_constants.py

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

SyntaxColoring · 2023-07-17T17:55:52Z

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

+
+
+@dataclass
+class PipetteModelVersionType:


Question:

What does the Type suffix signify? What kind of Python type isn't a Type? I've seen this in other places in our codebase, too.

In this instance, I did it to try and distinguish as much as possible from the PipetteModel/PipetteName dev_types we already have in our code base.

Normally in the past, it's been used when we have type defines at the top of the file. IMO I think it just sounds nicer. For example in my most recent PR, I had a type called something to the effect of OverrideType.

SyntaxColoring · 2023-07-17T18:14:41Z

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

@@ -88,20 +93,65 @@ def as_tuple(
        return (self.major, self.minor)


+# TODO (lc 12-5-2022) Ideally we can deprecate this


I suppose it could be, but ideally we move away from this being an external concept entirely. You truly should be able to request a certain pipette size/channels without having to worry about generation of the pipette.

Hmm. Even in a world where protocol authors request pipettes like that, it seems like this concept would still exist in HTTP. Something would still need to tell the app what pipettes the user needs to attach in order to run a protocol, and the app would still need to determine whether the pipettes that are already attached are a match.

Or is your vision that those would get reported as a fine-grained list of models? Like: "You can attach a p1000_single_1_0, or p1000_single_1_3, or p1000_single_1_4, or ..." etc.

We were in a state where the hardware controller was using the updated schemas but the Protocol Engine was not. This is the first step to porting over all of the systems to use the new schema.

… pip functions

jbleon95 · 2023-07-20T17:50:44Z

Successfully smoke tested, for Flex and OT-2, attaching single and multi-channel pipettes, loading pipettes in protocols (with both GEN-3 and user facing names for Flex), analyzing on both app and robot and running protocols on robot. Additionally confirmed that mutable configs were functioning on OT-2.

Laura-Danielle requested a review from a team July 14, 2023 14:47

Laura-Danielle requested a review from a team as a code owner July 14, 2023 14:47

sfoster1 requested changes Jul 14, 2023

View reviewed changes

sfoster1 approved these changes Jul 17, 2023

View reviewed changes

SyntaxColoring reviewed Jul 17, 2023

View reviewed changes

Laura-Danielle added 24 commits July 20, 2023 18:36

add quirks and mount configs to v2 pipette configurations

1f2cbc8

support mutable configs

a224613

add support in the routers

8f997cc

add quirks to schema

a44b33b

fix tavern tests

7a67e68

add pieptte name back in

cd42f7d

feat(api): support V2 pipette schemas for the OT2 and Protocol Engine

cdc34ec

We were in a state where the hardware controller was using the updated schemas but the Protocol Engine was not. This is the first step to porting over all of the systems to use the new schema.

update flow rates to be dicts, support dict tip overlaps

70b2dbe

changes to match new schema

f91f2e4

schema fixes and accidental typos

bcc99bd

test fix-ups

a91dc30

update default idle current

d58a7b6

update a few helpers and correct some config mistakes

a85f108

modify update pipette dict to support new configs

f01b9af

json formatting

1b612f1

fixup from rebase

f585b56

fixup update key func in pip config

dae2269

fix up linter

fae2363

accidental fixture duplicate from rebase

3bc8db6

channels inherit from int, do not type tipracks as LabwareUri

3d499a4

fixup channels name conversion

dd1e660

fix linter and mistake with quirks union

07b8d6c

fixup shared-data copy error

dfa403b

move definition types to pipette/types

d21d9f9

Laura-Danielle added 7 commits July 20, 2023 18:42

change lookup dictionary to use pipette types

68d42df

better schema comment for flowrate

63eafcf

change max_volume to model

e653868

add test for configuration updater

bcbbdba

support multiple versions of a pip function, remove tiprack uri keyed…

c3ec3e0

… pip functions

fix backwards compatability from channels change

ea2d88a

formatting, linting, missed changes to pipetting functions

2b19e8e

Laura-Danielle force-pushed the RSS-271-RLIQ-420-remove-old-pipette-config-load branch from ca3e159 to 2b19e8e Compare July 20, 2023 15:48

jbleon95 approved these changes Jul 20, 2023

View reviewed changes

fixup linter and hardware test functions

7919afd

Laura-Danielle merged commit 11fa4eb into edge Jul 20, 2023
46 of 47 checks passed

Laura-Danielle deleted the RSS-271-RLIQ-420-remove-old-pipette-config-load branch July 20, 2023 20:43

SyntaxColoring mentioned this pull request Sep 14, 2023

fix(api): Remove obsolete "_gen3" pipette names #13514

Merged

2 tasks



		# TODO (lc 07-05-2023) Move this back to the combined Pipette object file.
		def piecewise_volume_conversion(

		return PipetteLiquidPropertiesDefinition.parse_obj(liquid_dict)


		def _change_to_camel_case(c: str) -> str:



		def _change_to_camel_case(c: str) -> str:
		# Tiny helper function to convert to camelCase.

		@@ -88,20 +93,65 @@ def as_tuple(
		return (self.major, self.minor)


		# TODO (lc 12-5-2022) Ideally we can deprecate this

		return f"{config_name[0]}" + "".join(s.capitalize() for s in config_name[1::])


		def update_pipette_configuration(

feat(api): Support V2 pipette configs in the OT-2 and Protocol Engine #13104

feat(api): Support V2 pipette configs in the OT-2 and Protocol Engine #13104

Conversation

Laura-Danielle commented Jul 14, 2023 • edited by jbleon95 Loading

Overview

To-Do

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Jul 14, 2023 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

subtle but probably important distinctions

small changes, lots and lots of small consequences

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

Laura-Danielle Jul 17, 2023 • 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

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

sfoster1 left a comment

Choose a reason for hiding this comment

SyntaxColoring left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laura-Danielle Jul 17, 2023 • 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

jbleon95 commented Jul 20, 2023

Laura-Danielle commented Jul 14, 2023 •

edited by jbleon95

Loading

codecov bot commented Jul 14, 2023 •

edited

Loading

Laura-Danielle Jul 17, 2023 •

edited

Loading

SyntaxColoring left a comment •

edited

Loading

Laura-Danielle Jul 17, 2023 •

edited

Loading