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

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

Merged
merged 10 commits into from
Apr 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
20a2066
feat(shared-data): return latest pipette version from pipetteName
jerader Apr 24, 2024
cf6ea64
update comment
jerader Apr 24, 2024
69a1b0c
fix some tests
jerader Apr 24, 2024
6bd7761
fix pipette data provider test
jerader Apr 25, 2024
7101eb0
fix test_context test
jerader Apr 25, 2024
fd1f11e
Merge branch 'edge' into shared-data_return-latest-pip-version
jerader Apr 25, 2024
5cb78ef
update test to return new latest def
jerader Apr 25, 2024
6f7f861
clean up version_from_generation and address feedback
jerader Apr 25, 2024
048f911
fix comment
jerader Apr 25, 2024
7b522d5
use type instead of Literal
jerader Apr 25, 2024
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
4 changes: 2 additions & 2 deletions api/tests/opentrons/hardware_control/test_instruments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ def fake_func2(mount, value):
{types.Mount.LEFT: "p10_single", types.Mount.RIGHT: "p300_single_gen2"}
)
attached = sim.attached_instruments
assert attached[types.Mount.LEFT]["model"] == "p10_single_v1"
assert attached[types.Mount.LEFT]["model"] == "p10_single_v1.5"
assert attached[types.Mount.LEFT]["name"] == "p10_single"

steps_mm_calls = [mock.call({"B": 768}), mock.call({"C": 3200})]
Expand Down Expand Up @@ -291,7 +291,7 @@ def fake_func2(mount, value):
# If we use prefixes, that should work too
await sim.cache_instruments({types.Mount.RIGHT: "p300_single"})
attached = sim.attached_instruments
assert attached[types.Mount.RIGHT]["model"] == "p300_single_v1"
assert attached[types.Mount.RIGHT]["model"] == "p300_single_v1.5"
assert attached[types.Mount.RIGHT]["name"] == "p300_single"
# If we specify instruments at init time, we should get them without
# passing an expectation
Expand Down
2 changes: 1 addition & 1 deletion api/tests/opentrons/protocol_api_old/test_context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ async def test_motion(ctx, hardware):
old_pos[Axis.X] = 0.0
old_pos[Axis.Y] = 0.0
old_pos[Axis.A] = 0.0
old_pos[Axis.C] = 2.0
old_pos[Axis.C] = 2.5
assert await hardware.current_position(instr._core.get_mount()) == pytest.approx(
old_pos
)
Expand Down
24 changes: 12 additions & 12 deletions api/tests/opentrons/protocol_engine/resources/test_pipette_data_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def test_get_virtual_pipette_static_config(
)

assert result == LoadedStaticPipetteData(
model="p20_single_v2.0",
model="p20_single_v2.2",
display_name="P20 Single-Channel GEN2",
min_volume=1,
max_volume=20.0,
Expand Down Expand Up @@ -69,17 +69,17 @@ def test_configure_virtual_pipette_for_volume(
PipetteNameType.P50_SINGLE_FLEX.value, "my-pipette"
)
assert result1 == LoadedStaticPipetteData(
model="p50_single_v3.0",
model="p50_single_v3.6",
display_name="Flex 1-Channel 50 μL",
min_volume=5,
max_volume=50.0,
channels=1,
nozzle_offset_z=-259.15,
home_position=230.15,
flow_rates=FlowRates(
default_blow_out={"2.14": 4.0},
default_aspirate={"2.14": 8.0},
default_dispense={"2.14": 8.0},
default_blow_out={"2.14": 57},
default_aspirate={"2.14": 35},
default_dispense={"2.14": 57},
),
tip_configuration_lookup_table=result1.tip_configuration_lookup_table,
nominal_tip_overlap=result1.nominal_tip_overlap,
Expand All @@ -94,17 +94,17 @@ def test_configure_virtual_pipette_for_volume(
PipetteNameType.P50_SINGLE_FLEX.value, "my-pipette"
)
assert result2 == LoadedStaticPipetteData(
model="p50_single_v3.0",
model="p50_single_v3.6",
display_name="Flex 1-Channel 50 μL",
min_volume=1,
max_volume=30,
channels=1,
nozzle_offset_z=-259.15,
home_position=230.15,
flow_rates=FlowRates(
default_blow_out={"2.14": 4.0},
default_aspirate={"2.14": 8.0},
default_dispense={"2.14": 8.0},
default_blow_out={"2.14": 26.7},
default_aspirate={"2.14": 26.7},
default_dispense={"2.14": 26.7},
),
tip_configuration_lookup_table=result2.tip_configuration_lookup_table,
nominal_tip_overlap=result2.nominal_tip_overlap,
Expand Down Expand Up @@ -162,19 +162,19 @@ def test_load_virtual_pipette_nozzle_layout(
assert result.configuration.value == "FULL"

subject_instance.configure_virtual_pipette_nozzle_layout(
"my-96-pipette", "p1000_96_v3.5", "A1", "A12", "A1"
"my-96-pipette", "p1000_96_v3.6", "A1", "A12", "A1"
)
result = subject_instance.get_nozzle_layout_for_pipette("my-96-pipette")
assert result.configuration.value == "ROW"

subject_instance.configure_virtual_pipette_nozzle_layout(
"my-96-pipette", "p1000_96_v3.5", "A1", "A1"
"my-96-pipette", "p1000_96_v3.6", "A1", "A1"
)
result = subject_instance.get_nozzle_layout_for_pipette("my-96-pipette")
assert result.configuration.value == "SINGLE"

subject_instance.configure_virtual_pipette_nozzle_layout(
"my-96-pipette", "p1000_96_v3.5", "A1", "H1"
"my-96-pipette", "p1000_96_v3.6", "A1", "H1"
)
result = subject_instance.get_nozzle_layout_for_pipette("my-96-pipette")
assert result.configuration.value == "COLUMN"
Expand Down
6 changes: 3 additions & 3 deletions shared-data/pipette/definitions/2/liquid/single_channel/p50/lowVolumeDefault/3_6.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
"uiMaxFlowRate": 26.7,
"defaultAspirateFlowRate": {
"default": 35,
"valuesByApiLevel": { "2.14": 35 }
"valuesByApiLevel": { "2.14": 26.7 }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

},
"defaultDispenseFlowRate": {
"default": 57,
"valuesByApiLevel": { "2.14": 57 }
"valuesByApiLevel": { "2.14": 26.7 }
},
"defaultBlowOutFlowRate": {
"default": 57,
"valuesByApiLevel": { "2.14": 57 }
"valuesByApiLevel": { "2.14": 26.7 }
},
"defaultFlowAcceleration": 1200.0,
"defaultTipLength": 57.9,
Expand Down
88 changes: 75 additions & 13 deletions shared-data/python/opentrons_shared_data/pipette/pipette_load_name_conversions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import os
import re
from typing import List, Optional, Union, cast
from typing import List, Optional, Union, cast, Literal
from opentrons_shared_data import get_shared_data_root
from .dev_types import PipetteModel, PipetteName

from .types import (
PipetteChannelType,
PipetteModelType,
Expand Down Expand Up @@ -106,26 +109,85 @@
return PipetteVersionType(major, minor)


def get_channel_from_pipette_name(pipette_name_list: List[str]) -> str:
if "single" in pipette_name_list:
return "single_channel"
elif "96" in pipette_name_list:
return "ninety_six_channel"
else:
return "eight_channel"


def get_major_version_from_pipette_name(
pipette_name_list: List[str],
) -> Literal[1, 2, 3]:
# special-casing for 96-channel to return version 3
if (
"flex" in pipette_name_list
or "gen3" in pipette_name_list
or "96" in pipette_name_list
):
return 3
elif "gen2" in pipette_name_list:
return 2
else:
return 1

Check warning on line 134 in shared-data/python/opentrons_shared_data/pipette/pipette_load_name_conversions.py

View check run for this annotation

Codecov / codecov/patch

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

Added line #L134 was not covered by tests


def version_from_generation(pipette_name_list: List[str]) -> PipetteVersionType:
Copy link
Member

Choose a reason for hiding this comment

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

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

"""Convert a string generation name to a py:obj:PipetteVersionType.
"""Convert pipetteName to a py:obj:PipetteVersionType

Pipette generations are strings in the format of "gen1" or "gen2", and
usually associated withe :py:data:PipetteName.
Given the pipette_name_list, cycle through each definition file path
and find the latest version (major and minor version combined) that
exists and return that version.

Args:
pipette_name_list (List[str]): A list of strings from the separated by `_`
py:data:PipetteName.
pipette_name_list (List[str]): A list of strings from the separated
by `_` py:data:PipetteName.

Returns:
PipetteVersionType: A pipette version object.

"""
if "flex" in pipette_name_list or "gen3" in pipette_name_list:
return PipetteVersionType(3, 0)
elif "gen2" in pipette_name_list:
return PipetteVersionType(2, 0)
else:
return PipetteVersionType(1, 0)
major_version_from_pipette_name = get_major_version_from_pipette_name(
pipette_name_list
)
model_from_pipette_name = pipette_name_list[0]
channel_from_pipette_name = get_channel_from_pipette_name(pipette_name_list)

paths_to_validate = (
get_shared_data_root() / "pipette" / "definitions" / "2" / "general"
)

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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]

Copy link
Member

Choose a reason for hiding this comment

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

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


for channel_dir in os.listdir(paths_to_validate):
Copy link
Member

Choose a reason for hiding this comment

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

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

if channel_dir != channel_from_pipette_name:
continue

for model_dir in os.listdir(paths_to_validate / channel_dir):
if model_dir != model_from_pipette_name:
continue

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

Choose a reason for hiding this comment

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

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)

major_version = version_list[0]
minor_version = version_list[1]

# Check if the major version matches the expected major version
if major_version == str(major_version_from_pipette_name):
Copy link
Member

Choose a reason for hiding this comment

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

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

if major_version != str(major_version_from_pipette_name):
    continue

minor_version_int = int(minor_version)
minor_version_lit: PipetteModelMinorVersionType = cast(
PipetteModelMinorVersionType, minor_version_int
)

# Update the highest minor version if this version is higher
if highest_minor_version < minor_version_lit:
highest_minor_version = minor_version_lit

if highest_minor_version == 0:
Copy link
Member

Choose a reason for hiding this comment

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

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

return PipetteVersionType(major_version_from_pipette_name, 0)

return PipetteVersionType(major_version_from_pipette_name, highest_minor_version)


def generation_from_string(pipette_name_list: List[str]) -> PipetteGenerationType:
Expand Down
4 changes: 2 additions & 2 deletions shared-data/python/tests/pipette/test_pipette_load_name_conversions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,15 @@ def test_convert_pipette_model_provided_version(
pc.PipetteModelVersionType(
PipetteModelType.p1000,
PipetteChannelType.EIGHT_CHANNEL,
PipetteVersionType(3, 0),
PipetteVersionType(3, 5),
),
],
[
"p1000_96",
pc.PipetteModelVersionType(
PipetteModelType.p1000,
PipetteChannelType.NINETY_SIX_CHANNEL,
PipetteVersionType(1, 0),
PipetteVersionType(3, 6),
),
],
],
Expand Down
Loading