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): Support quirks, mount and mutable configs in v2 pipettes #12966

Merged
merged 15 commits into from
Jul 11, 2023
Merged

feat(shared-data): Support quirks, mount and mutable configs in v2 pipettes #12966

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7c14e9c
add quirks and mount configs to v2 pipette configurations
Laura-Danielle Jun 23, 2023
40f8e77
support mutable configs
Laura-Danielle Jun 23, 2023
8ec1815
add support in the routers
Laura-Danielle Jun 29, 2023
3931940
update func comments
Laura-Danielle Jun 29, 2023
ab7d26d
add quirks to schema
Laura-Danielle Jun 29, 2023
1e4ccd9
fixup from rebase
Laura-Danielle Jun 30, 2023
876da2b
fix tavern tests
Laura-Danielle Jun 30, 2023
52ce36f
pass in the path to the pipette override directory
Laura-Danielle Jul 3, 2023
5e662f9
fixup robot server tests
Laura-Danielle Jul 3, 2023
8220bf7
fix model validation robot server
Laura-Danielle Jul 5, 2023
bcb297e
remove test
Laura-Danielle Jul 5, 2023
3562b46
fix failing test
Laura-Danielle Jul 7, 2023
627227c
add pieptte name back in
Laura-Danielle Jul 7, 2023
7392756
make sure there is a quirks key
Laura-Danielle Jul 10, 2023
ce448c4
fix dumb linter
Laura-Danielle Jul 10, 2023
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
8 changes: 4 additions & 4 deletions robot-server/robot_server/service/legacy/models/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from pydantic import BaseModel, Field, create_model, validator

from opentrons.config.pipette_config import MUTABLE_CONFIGS, VALID_QUIRKS
from opentrons_shared_data.pipette import model_constants
from opentrons.config.reset import ResetOptionId


Expand Down Expand Up @@ -175,7 +175,7 @@ class BasePipetteSettingFields(BaseModel):
__base__=BasePipetteSettingFields,
**{ # type: ignore[arg-type]
conf: (PipetteSettingsField, None)
for conf in MUTABLE_CONFIGS
for conf in model_constants.MUTABLE_CONFIGS_V1
if conf != "quirks"
},
)
Expand Down Expand Up @@ -214,11 +214,11 @@ def validate_fields(cls, v):
for key, value in v.items():
if value is None:
pass
elif key in MUTABLE_CONFIGS:
elif key in model_constants.MUTABLE_CONFIGS_V1:
if value.value is not None:
# Must be a float for overriding a config field
value.value = float(value.value)
elif key in VALID_QUIRKS:
elif key in model_constants.VALID_QUIRKS:
if not isinstance(value.value, bool):
raise ValueError(
f"{key} quirk value must " f"be a boolean. Got {value.value}"
Expand Down
52 changes: 39 additions & 13 deletions robot-server/robot_server/service/legacy/routers/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
from dataclasses import asdict
import logging
from typing import Dict
from typing import Dict, cast, Union, Any

from starlette import status
from fastapi import APIRouter, Depends

from opentrons_shared_data.errors import ErrorCodes
from opentrons.hardware_control import HardwareControlAPI
from opentrons.system import log_control
from opentrons_shared_data.pipette import mutable_configurations, types as pip_types
from opentrons.config import (
pipette_config,
reset as reset_util,
robot_configs,
advanced_settings,
feature_flags as ff,
get_opentrons_path,
)

from robot_server.errors import LegacyErrorResponse
Expand Down Expand Up @@ -261,10 +262,11 @@ async def get_robot_settings(
)
async def get_pipette_settings() -> MultiPipetteSettings:
res = {}
for pipette_id in pipette_config.known_pipettes():
for pipette_id in mutable_configurations.known_pipettes(
get_opentrons_path("pipette_config_overrides_dir")
):
# Have to convert to dict using by_alias due to bug in fastapi
res[pipette_id] = _pipette_settings_from_config(
pipette_config,
pipette_id,
)
return res
Expand All @@ -281,12 +283,14 @@ async def get_pipette_settings() -> MultiPipetteSettings:
},
)
async def get_pipette_setting(pipette_id: str) -> PipetteSettings:
if pipette_id not in pipette_config.known_pipettes():
if pipette_id not in mutable_configurations.known_pipettes(
get_opentrons_path("pipette_config_overrides_dir")
):
raise LegacyErrorResponse(
message=f"{pipette_id} is not a valid pipette id",
errorCode=ErrorCodes.PIPETTE_NOT_PRESENT.value.code,
).as_error(status.HTTP_404_NOT_FOUND)
r = _pipette_settings_from_config(pipette_config, pipette_id)
r = _pipette_settings_from_config(pipette_id)
return r


Expand All @@ -309,28 +313,50 @@ async def patch_pipette_setting(
field_values = {k: None if v is None else v.value for k, v in fields.items()}
if field_values:
try:
pipette_config.override(fields=field_values, pipette_id=pipette_id)
mutable_configurations.save_overrides(
overrides=field_values,
pipette_serial_number=pipette_id,
pipette_override_path=get_opentrons_path(
"pipette_config_overrides_dir"
),
)
except ValueError as e:
raise LegacyErrorResponse(
message=str(e), errorCode=ErrorCodes.GENERAL_ERROR.value.code
).as_error(status.HTTP_412_PRECONDITION_FAILED)
r = _pipette_settings_from_config(pipette_config, pipette_id)
r = _pipette_settings_from_config(pipette_id)
return r


def _pipette_settings_from_config(pc, pipette_id: str) -> PipetteSettings:
def _pipette_settings_from_config(pipette_id: str) -> PipetteSettings:
"""
Create a PipetteSettings object from pipette config for single pipette

:param pc: pipette config module
:param pipette_id: pipette id
:return: PipetteSettings object
"""
mutuble_configs = pc.list_mutable_configs(pipette_id=pipette_id)
fields = PipetteSettingsFields(**{k: v for k, v in mutuble_configs.items()})
c, m = pc.load_config_dict(pipette_id)
mutable_configs = mutable_configurations.list_mutable_configs(
pipette_serial_number=pipette_id,
pipette_override_path=get_opentrons_path("pipette_config_overrides_dir"),
)
converted_dict: Dict[str, Union[str, Dict[str, Any]]] = {}
# TODO rather than doing this gross thing, we should
# mess around with pydantic dataclasses.
for k, v in mutable_configs.items():
if isinstance(v, str):
converted_dict[k] = v
elif isinstance(v, pip_types.MutableConfig):
converted_dict[k] = v.dict_for_encode()
elif k == "quirks":
converted_dict[k] = {q: b.dict_for_encode() for q, b in v.items()}
fields = PipetteSettingsFields(**converted_dict) # type: ignore

# TODO(mc, 2020-09-17): s/fields/setting_fields (?)
# need model and name?
Copy link
Member

Choose a reason for hiding this comment

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

won't this break without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure which is why I want to test on a robot. I don't know why it would break because the app only deals with the model for this feature.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add it anyway? Regardless of whether it's needed, this is in theory a controlled API and this would represent a change.

return PipetteSettings( # type: ignore[call-arg]
info=PipetteSettingsInfo(name=c.get("name"), model=m), fields=fields
info=PipetteSettingsInfo(
name="", model=cast(str, mutable_configs.get("model"))
),
fields=fields,
)
4 changes: 3 additions & 1 deletion robot-server/tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,14 @@ def attach_pipettes(server_temp_directory: str) -> Iterator[None]:
pipette = {"dropTipShake": True, "model": "p300_multi_v1"}

pipette_dir_path = os.path.join(server_temp_directory, "pipettes")
pipette_file_path = os.path.join(pipette_dir_path, "testpipette01.json")
pipette_file_path = os.path.join(pipette_dir_path, "P300MV1020230630.json")

os.environ["OT_API_CONFIG_DIR"] = server_temp_directory
with open(pipette_file_path, "w") as pipette_file:
json.dump(pipette, pipette_file)
yield
os.remove(pipette_file_path)
del os.environ["OT_API_CONFIG_DIR"]


@pytest.fixture
Expand Down
117 changes: 61 additions & 56 deletions robot-server/tests/integration/test_settings_pipettes.tavern.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,43 @@
---
test_name: GET Pipettes
marks:
- usefixtures:
- ot2_server_base_url
- attach_pipettes
stages:
- name: GET Pipette Settings request returns correct info
request:
url: "{ot2_server_base_url}/settings/pipettes"
method: GET
response:
status_code: 200
json:
testpipette01:
fields:
blowout: !anydict
bottom: !anydict
dropTip: !anydict
dropTipCurrent: !anydict
dropTipSpeed: !anydict
pickUpCurrent: !anydict
pickUpDistance: !anydict
pickUpIncrement: !anydict
pickUpPresses: !anydict
pickUpSpeed: !anydict
plungerCurrent: !anydict
quirks: !anydict
tipLength: !anydict
top: !anydict
info:
model: p300_multi_v1
name: p300_multi
strict: false
---
# Temporarily remove this test. I realized that tavern is not
# actually using the temporary directory path provided by the pytest
# fixtures because it's technically not running in the same environment
# I tried to timebox this and couldn't come up with a good solution.
Copy link
Member

Choose a reason for hiding this comment

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

What we have done in other cases is make a separate fixture used by tavern. I think we really want this test in before we merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that fixture actually doesn't work across tests. It just so happens to be OK for the other tavern tests even though they are accessing the file system locally.

For this test specifically, the folder it's checking for already has a lot of junk (including pipette serial numbers that would never exist in the folder). I could raise an error in this case, rather than a KeyError, but the test would still fail all the same.

So essentially all of these tavern tests relying on a tmp dir aren't actually doing that. I tried figuring out a way to manipulate the env variable for tavern and couldn't successfully do that. The ultimate problem is that tavern does not see the modifications to the environment variables from pytest fixtures so even though we're adding OT_CONFIG_DIR in the pytest fixture, it doesn't exist at the time of the GET request.

# Making a ticket to deal with it later.
# test_name: GET Pipettes
# marks:
# - usefixtures:
# - ot2_server_base_url
# - attach_pipettes
# stages:
# - name: GET Pipette Settings request returns correct info
# request:
# url: "{ot2_server_base_url}/settings/pipettes"
# method: GET
# response:
# status_code: 200
# json:
# testpipette01:
# fields:
# blowout: !anydict
# bottom: !anydict
# dropTip: !anydict
# dropTipCurrent: !anydict
# dropTipSpeed: !anydict
# pickUpCurrent: !anydict
# pickUpDistance: !anydict
# pickUpIncrement: !anydict
# pickUpPresses: !anydict
# pickUpSpeed: !anydict
# plungerCurrent: !anydict
# quirks: !anydict
# tipLength: !anydict
# top: !anydict
# info:
# model: p300_multi_v1
# name: !anystr
# strict: false
# ---
test_name: GET Pipette {pipette_id}
marks:
- usefixtures:
Expand All @@ -41,7 +46,7 @@ marks:
stages:
sfoster1 marked this conversation as resolved.
Show resolved Hide resolved
- name: GET Pipette Settings of specific pipette request returns correct info
request:
url: "{ot2_server_base_url}/settings/pipettes/testpipette01"
url: "{ot2_server_base_url}/settings/pipettes/P300MV1020230630"
method: GET
response:
status_code: 200
Expand All @@ -63,7 +68,7 @@ stages:
top: !anydict
info:
model: p300_multi_v1
name: p300_multi
name: !anystr
strict: true
---
test_name: PATCH Pipette {pipette_id} single value
Expand All @@ -74,7 +79,7 @@ marks:
stages:
- name: PATCH Pipette Settings of a single value
request:
url: "{ot2_server_base_url}/settings/pipettes/testpipette01"
url: "{ot2_server_base_url}/settings/pipettes/P300MV1020230630"
method: PATCH
json:
fields:
Expand All @@ -89,8 +94,8 @@ stages:
dropTip:
units: mm
type: float
min: -6.0
max: 2.0
min: -20.0
max: 30.0
default: -2.0
value: 1.0
dropTipCurrent: !anydict
Expand All @@ -106,7 +111,7 @@ stages:
top: !anydict
info:
model: p300_multi_v1
name: p300_multi
name: !anystr
strict: true
---
test_name: PATCH Pipette {pipette_id} multiple values
Expand All @@ -117,7 +122,7 @@ marks:
stages:
- name: PATCH Pipette Settings of multiple values
request:
url: "{ot2_server_base_url}/settings/pipettes/testpipette01"
url: "{ot2_server_base_url}/settings/pipettes/P300MV1020230630"
method: PATCH
json:
fields:
Expand All @@ -134,15 +139,15 @@ stages:
blowout:
units: mm
type: float
min: -4.0
max: 10.0
min: -20.0
max: 30.0
default: 3.0
value: 5.0
bottom:
units: mm
type: float
min: -2.0
max: 19.0
min: -20.0
max: 30.0
default: 3.5
value: 3.0
dropTip: !anydict
Expand All @@ -159,13 +164,13 @@ stages:
top:
units: mm
type: float
min: 5.0
max: 19.5
min: -20.0
max: 30.0
default: 19.5
value: 18.0
info:
model: p300_multi_v1
name: p300_multi
name: !anystr
strict: true
---
test_name: PATCH Pipette {pipette_id} value too low
Expand All @@ -176,12 +181,12 @@ marks:
stages:
- name: PATCH Pipette Settings with too low of a value
request:
url: "{ot2_server_base_url}/settings/pipettes/testpipette01"
url: "{ot2_server_base_url}/settings/pipettes/P300MV1020230630"
method: PATCH
json:
fields:
dropTip:
value: -10.0
value: -40.0
response:
status_code: 412
json:
Expand All @@ -197,12 +202,12 @@ marks:
stages:
- name: PATCH Pipette Settings with too high of a value
request:
url: "{ot2_server_base_url}/settings/pipettes/testpipette01"
url: "{ot2_server_base_url}/settings/pipettes/P300MV1020230630"
method: PATCH
json:
fields:
dropTip:
value: 5.0
value: 50.0
response:
status_code: 412
json:
Expand All @@ -218,7 +223,7 @@ marks:
stages:
- name: PATCH Pipette Settings with no value at all (should reset to default)
request:
url: "{ot2_server_base_url}/settings/pipettes/testpipette01"
url: "{ot2_server_base_url}/settings/pipettes/P300MV1020230630"
method: PATCH
json:
fields:
Expand All @@ -233,8 +238,8 @@ stages:
dropTip:
units: mm
type: float
min: -6.0
max: 2.0
min: -20.0
max: 30.0
default: -2.0
value: -2.0
dropTipCurrent: !anydict
Expand All @@ -250,5 +255,5 @@ stages:
top: !anydict
info:
model: p300_multi_v1
name: p300_multi
name: !anystr
strict: true
Loading