feat(shared-data, api): return latest pipette version from pipetteName #15002

jerader · 2024-04-24T19:46:26Z

Overview

Previously, if you have access to only the pipetteName, the pipette definitions you get back are the first pipette version for the generation. For example, if you have a p1000_single_flex, you would get the pipette definitions for 3_0. After speaking with Seth, Andy, Ed, and Sanniti, we decided that this is not great because 1) version .0s are prototypes and aren't in the wild and 2) the latest versions have the most up to date specs for things such as flowRates. (slack conversation about this if interested)

We thought about making the prototype versions the "perfect definitions" but that requires maintenance and it is much easier in the long run just to return the latest version that exists.

This change is already implemented in the frontend (see getPipetteV2Specs), so Protocol Designer (et al) will have the latest flow rates 🚀 .

But, we needed to align in the api as well so opentrons-simulate and analysis also have the latest data. This PR updates the version_from_generation def in shared-data to return the latest version.

Note: the downfall to the current approach is there are a few api tests that will need to be updated whenever the specific pipette has a new version - see the PR for which tests those are.

Test Plan

upload this to the app on edge and look at the run log - the aspirate/dispense flow rate should be 160 ul/sec, which is the 1000uL tip default flow rate for the 3_0 model.

then upload this to the app on this branch and look at the run log - the aspirate/dispense flow rate should be 716 ul/sec, which is the 1000uL tip default flow rate for the 3_6 model.

requirements = {
	"robotType": "Flex",
	"apiLevel": "2.16"
}

def run(ctx):
  tip_rack2 = ctx.load_labware("opentrons_flex_96_tiprack_1000ul", "D3")
  instrument = ctx.load_instrument('flex_1channel_1000', tip_racks=[tip_rack2], mount="left")
  
  my_pcr_plate = ctx.load_labware('nest_96_wellplate_200ul_flat', "C2")
  my_other_plate = ctx.load_labware('nest_96_wellplate_200ul_flat', "C1")
  ctx.load_trash_bin("A3")
    
  instrument.pick_up_tip()  
  instrument.aspirate(50, my_pcr_plate.wells_by_name()["A4"])
  instrument.dispense(20, my_other_plate.wells_by_name()["A2"])

  instrument.drop_tip()

Changelog

refactor version_from_generation
update the test_convert_pipette_name
update all affected tests that now return the latest pipette version from the pipetteName
update the low volume default max flow rate for the p50_single_3.6 pipette definition to match the ui max flow rate (see feat(shared-data, api): add uiMaxFlowRate key to pipette definitions #14859 for details on why)

Review requests

Please review the code and logic and let me know if there is anything else I need to change?

Risk assessment

low

closes AUTH-357

jerader · 2024-04-25T12:09:19Z

shared-data/pipette/definitions/2/liquid/single_channel/p50/lowVolumeDefault/3_6.json

@@ -5,15 +5,15 @@
      "uiMaxFlowRate": 26.7,
      "defaultAspirateFlowRate": {
        "default": 35,
-        "valuesByApiLevel": { "2.14": 35 }
+        "valuesByApiLevel": { "2.14": 26.7 }


i updated these to match the uiMaxFlowRate - see #14859 for more details

codecov · 2024-04-25T12:18:49Z

Codecov Report

Attention: Patch coverage is 97.36842% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 76.31%. Comparing base (8f50b08) to head (5cb78ef).
Report is 134 commits behind head on edge.

❗ Current head 5cb78ef differs from pull request most recent head 7b522d5. Consider uploading reports for the commit 7b522d5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #15002      +/-   ##
==========================================
+ Coverage   67.50%   76.31%   +8.81%     
==========================================
  Files        2521       42    -2479     
  Lines       72090     2804   -69286     
  Branches     9311        0    -9311     
==========================================
- Hits        48666     2140   -46526     
+ Misses      21228      664   -20564     
+ Partials     2196        0    -2196

Flag	Coverage Δ
api	`?`
app	`?`
components	`?`
g-code-testing	`?`
hardware	`?`
hardware-testing	`?`
labware-library	`?`
notify-server	`?`
ot3-gravimetric-test	`?`
protocol-designer	`?`
react-api-client	`?`
robot-server	`?`
shared-data	`76.31% <97.36%> (+1.00%)`	⬆️
step-generation	`?`
system-server	`?`
update-server	`?`
usb-bridge	`?`

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

Files	Coverage Δ
...ared_data/pipette/pipette_load_name_conversions.py	`89.28% <97.36%> (+3.03%)`	⬆️

... and 2488 files with indirect coverage changes

ecormany

It worrrrrrrrrks! 🎉

Users ask about this all the time — so glad this will be accurate now.

sfoster1

Some inline things, I think we can rework that main function a bit.

It's weird that the test asks for a kind of pipette that does not exist... maybe we can add a couple more cases to it?

sfoster1 · 2024-04-25T17:07:00Z

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

+    else:
+        return 1
+
+
 def version_from_generation(pipette_name_list: List[str]) -> PipetteVersionType:


let's drop an lru_cache on this, it's a perfect candidate:

it will often be called with the same thing when something tries to load data

it is pretty expensive to call since it does a whole ton of filesystem interactions

sfoster1 · 2024-04-25T17:07:34Z

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

+        get_shared_data_root() / "pipette" / "definitions" / "2" / "general"
+    )
+
+    highest_minor_version: Literal[0, 1, 2, 3, 4, 5, 6] = 0


we'd have to update this whenever we add a new minor version, right? can we weaken its type to int?

yeah, so the return type of version_from_generation is PipetteVersionType, which is a Literal. I guess i could weaken it so the return type is just an int. i agree it should be weakened so that we don't have to always update it

nevermind, this is giving me a hard time updating because all the other utils in the file use PipetteVersionType as well 🤔 maybe it can be refactored in a followup? Plus there is this comment in types.py

# TODO Literals are only good for writing down # exact values. Is there a better typing mechanism # so we don't need to keep track of versions in two # different places? PipetteModelMajorVersionType = Literal[1, 2, 3] PipetteModelMinorVersionType = Literal[0, 1, 2, 3, 4, 5, 6]

yeah, a followup is fine i suppose. could we maybe have this be highest_minor_version: PipetteModelMinorVersionType = 0 then? that way we wouldn't have to change it in two places

sfoster1 · 2024-04-25T17:09:04Z

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

+                    if highest_minor_version < minor_version_lit:
+                        highest_minor_version = minor_version_lit
+
+    if highest_minor_version == 0:


i don't think we need this, right? 0 is a valid value there, and the major version is the same, so we can unconditionally return PipetteVersionType(major_version_from_pipette_name, highest_minor_version) and it's fine if highest_minor_version is 0

sfoster1 · 2024-04-25T17:12:41Z

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

+
+    highest_minor_version: Literal[0, 1, 2, 3, 4, 5, 6] = 0
+
+    for channel_dir in os.listdir(paths_to_validate):


The outer two for loops don't seem to be doing much, I think. we're iterating through them and checking for exact equality to the values we already have, so let's just construct the path at the beginning:

versions_path = (paths_to_validate / channel_from_pipette_name / model_from_pipette_name) for version_file in version_paths.iterdir(): ...

Path.iterdir

sfoster1 · 2024-04-25T17:14:09Z

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

+                continue
+
+            for version_file in os.listdir(paths_to_validate / channel_dir / model_dir):
+                version_list = version_file.split(".json")[0].split("_")


Suggested change

version_list = version_file.split(".json")[0].split("_")

version_list = version_file.stem.split("_")

PurePath.stem

(as long as we're using e.g. (paths_to_validate/channel_dir/model_dir).iterdir() rather than os.listdir so we get path objects)

sfoster1 · 2024-04-25T17:16:02Z

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

+                minor_version = version_list[1]
+
+                # Check if the major version matches the expected major version
+                if major_version == str(major_version_from_pipette_name):


maybe invert logic and continue to save some indentation, i.e.

if major_version != str(major_version_from_pipette_name): continue

sfoster1

Looks great!

#15002) closes AUTH-357 This PR updates the `version_from_generation` def in `shared-data` to return the latest version.

jerader added 2 commits April 24, 2024 15:39

feat(shared-data): return latest pipette version from pipetteName

20a2066

closes AUTH-357

update comment

cf6ea64

jerader changed the title ~~feat(shared-data): return latest pipette version from pipetteName~~ feat(shared-data, api): return latest pipette version from pipetteName Apr 24, 2024

jerader added 5 commits April 24, 2024 16:20

fix some tests

69a1b0c

fix pipette data provider test

6bd7761

fix test_context test

7101eb0

Merge branch 'edge' into shared-data_return-latest-pip-version

fd1f11e

update test to return new latest def

5cb78ef

jerader commented Apr 25, 2024

View reviewed changes

jerader marked this pull request as ready for review April 25, 2024 12:24

jerader requested review from a team as code owners April 25, 2024 12:24

jerader requested review from sfoster1 and sanni-t April 25, 2024 12:24

ecormany reviewed Apr 25, 2024

View reviewed changes

sfoster1 requested changes Apr 25, 2024

View reviewed changes

jerader added 2 commits April 25, 2024 16:50

clean up version_from_generation and address feedback

6f7f861

fix comment

048f911

sfoster1 approved these changes Apr 25, 2024

View reviewed changes

use type instead of Literal

7b522d5

jerader merged commit 264883a into edge Apr 25, 2024
52 checks passed

jerader deleted the shared-data_return-latest-pip-version branch April 25, 2024 21:38

sanni-t mentioned this pull request Apr 30, 2024

fix(test): snapshot renaming #15038

Merged

Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024

feat(shared-data, api): return latest pipette version from pipetteName (

f913db2

#15002) closes AUTH-357 This PR updates the `version_from_generation` def in `shared-data` to return the latest version.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(shared-data, api): return latest pipette version from pipetteName #15002

feat(shared-data, api): return latest pipette version from pipetteName #15002

jerader commented Apr 24, 2024 •

edited

Loading

jerader Apr 25, 2024

codecov bot commented Apr 25, 2024 •

edited

Loading

ecormany left a comment

sfoster1 left a comment

sfoster1 Apr 25, 2024

sfoster1 Apr 25, 2024

jerader Apr 25, 2024

jerader Apr 25, 2024

sfoster1 Apr 25, 2024

sfoster1 Apr 25, 2024

sfoster1 Apr 25, 2024

sfoster1 Apr 25, 2024

sfoster1 Apr 25, 2024

sfoster1 left a comment


		highest_minor_version: Literal[0, 1, 2, 3, 4, 5, 6] = 0

		for channel_dir in os.listdir(paths_to_validate):

	version_list = version_file.split(".json")[0].split("_")
	version_list = version_file.stem.split("_")

feat(shared-data, api): return latest pipette version from pipetteName #15002

feat(shared-data, api): return latest pipette version from pipetteName #15002

Conversation

jerader commented Apr 24, 2024 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

Choose a reason for hiding this comment

codecov bot commented Apr 25, 2024 • edited Loading

Codecov Report

ecormany 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

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

jerader commented Apr 24, 2024 •

edited

Loading

codecov bot commented Apr 25, 2024 •

edited

Loading