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 9 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
90 changes: 75 additions & 15 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 re
from typing import List, Optional, Union, cast
from functools import lru_cache
from typing import List, Optional, Union, cast, Literal, Tuple
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,78 @@ def version_from_string(version: str) -> PipetteVersionType:
return PipetteVersionType(major, minor)


def version_from_generation(pipette_name_list: List[str]) -> PipetteVersionType:
"""Convert a string generation name to a py:obj:PipetteVersionType.
def get_channel_from_pipette_name(pipette_name_tuple: Tuple[str, ...]) -> str:
if "single" in pipette_name_tuple:
return "single_channel"
elif "96" in pipette_name_tuple:
return "ninety_six_channel"
else:
return "eight_channel"


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


@lru_cache(4)
def version_from_generation(pipette_name_tuple: Tuple[str, ...]) -> 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_tuple, 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_tuple (Tuple[str, ...]): A tuple 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_tuple
)
model_from_pipette_name = pipette_name_tuple[0]
channel_from_pipette_name = get_channel_from_pipette_name(pipette_name_tuple)

paths_to_validate = (
get_shared_data_root() / "pipette" / "definitions" / "2" / "general"
)
version_paths = (
paths_to_validate / channel_from_pipette_name / model_from_pipette_name
)

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 version_file in version_paths.iterdir():
version_list = version_file.stem.split("_")
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):
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

return PipetteVersionType(major_version_from_pipette_name, highest_minor_version)


def generation_from_string(pipette_name_list: List[str]) -> PipetteGenerationType:
Expand Down Expand Up @@ -194,7 +249,12 @@ def convert_pipette_name(
if provided_version:
version = version_from_string(provided_version)
else:
version = version_from_generation(split_pipette_name)
pipette_name_tuple: Tuple[str, str, str] = (
split_pipette_name[0],
split_pipette_name[1],
split_pipette_name[2] if len(split_pipette_name) > 2 else "",
)
version = version_from_generation(pipette_name_tuple)

pipette_type = PipetteModelType[split_pipette_name[0]]

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