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

Laura-Danielle · 2023-06-23T13:40:25Z

Overview

Support mutable configs and smoothie configs with the new v2 pipette schema.

Test Plan

Throw this on an OT-2 robot and mess around with the pipette config settings to make sure everything still renders correctly.

Remove config changes
Add a new config change
click and un-click quirks.

Changelog

Add quirks to schema
Add mutable config helpers to shared data
Utilize the new mutable configs in the robot server endpoint

The next step after this PR is to port the OT-2 to the new configs and support loading the new configs in the protocol engine.

Review requests

Please check it over and let me know what thoughts you have.

Risk assessment

Medium. Touches core functionality on the OT-2

codecov · 2023-06-23T13:44:20Z

Codecov Report

Merging #12966 (ce448c4) into edge (d02dc6f) will increase coverage by 0.18%.
The diff coverage is 94.09%.

@@            Coverage Diff             @@
##             edge   #12966      +/-   ##
==========================================
+ Coverage   72.70%   72.88%   +0.18%     
==========================================
  Files        2367     2380      +13     
  Lines       64631    66113    +1482     
  Branches     7261     7753     +492     
==========================================
+ Hits        46987    48188    +1201     
- Misses      15955    16102     +147     
- Partials     1689     1823     +134

Flag	Coverage Δ
app	`71.51% <ø> (+0.17%)`	⬆️
protocol-designer	`45.11% <ø> (+1.02%)`	⬆️
shared-data	`78.32% <94.09%> (+1.93%)`	⬆️
step-generation	`87.24% <ø> (+2.96%)`	⬆️

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

Impacted Files	Coverage Δ
...ver/robot_server/service/legacy/models/settings.py	`98.76% <ø> (-0.05%)`	⬇️
...er/robot_server/service/legacy/routers/settings.py	`100.00% <ø> (ø)`
...ared_data/pipette/pipette_load_name_conversions.py	`85.05% <84.61%> (+1.05%)`	⬆️
...rons_shared_data/pipette/mutable_configurations.py	`91.30% <91.30%> (ø)`
...rons_shared_data/pipette/file_operation_helpers.py	`95.65% <95.65%> (ø)`
...data/python/opentrons_shared_data/pipette/types.py	`98.11% <98.11%> (ø)`
.../python/opentrons_shared_data/pipette/load_data.py	`97.91% <100.00%> (+1.76%)`	⬆️
...n/opentrons_shared_data/pipette/model_constants.py	`100.00% <100.00%> (ø)`
...pentrons_shared_data/pipette/pipette_definition.py	`97.08% <100.00%> (+0.18%)`	⬆️

... and 81 files with indirect coverage changes

sfoster1

I don't think I agree with putting the quirks in code like this. It means if we ever want to add some new model we have to add it in conceptually separate places. If we can drop it in configs then we're back to the same system and I think we're fine.

sfoster1

This overall looks, well, like something we have to do, but I really want to avoid duplicating that IS_ROBOT bool - I think that will eternally bite us if we duplicate it.

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

sfoster1 · 2023-06-30T14:03:39Z

robot-server/robot_server/service/legacy/routers/settings.py


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


won't this break without it?

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.

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

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

sfoster1 · 2023-06-30T14:09:24Z

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

+    overrides: TypeOverrides,
+) -> OverrideType:
+    """Helper function to add new overrides to the existing ones"""
+    # FIXME remove the validation here for the file save and rely


i guess we can do this by passing down a full pydantic model at this point?

sfoster1

Okaym, I really like the changes to get the base directory passed in and decided elsewhere, the lack of duplication is great. I think the last thing I'd really want to see is fixes to the HTTP API and the tavern tests. Those tests are for detecting regressions, and I think that this PR introduces changes both of value and of kind (not having names) that we really need to investigate and probably fix.

sfoster1 · 2023-07-06T16:52:28Z

robot-server/robot_server/service/legacy/routers/settings.py


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


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

sfoster1 · 2023-07-06T16:53:09Z

robot-server/tests/integration/test_settings_pipettes.tavern.yaml

+# 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.


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.

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.

robot-server/tests/integration/test_settings_pipettes.tavern.yaml

sfoster1

Awesome! Looks really good

sanni-t

Tested on an OT-2 and confirmed that pipette settings & quirks changes from app are propagated correctly to the settings files on the robot and the app reads file contents correctly. Also verified that all changes are correctly reset to default with 'Reset All'.

sfoster1 requested changes Jun 27, 2023

View reviewed changes

Laura-Danielle marked this pull request as ready for review June 29, 2023 21:55

Laura-Danielle requested a review from a team as a code owner June 29, 2023 21:55

Laura-Danielle requested a review from sfoster1 June 29, 2023 21:55

Laura-Danielle added 6 commits June 30, 2023 10:21

add quirks and mount configs to v2 pipette configurations

7c14e9c

support mutable configs

40f8e77

add support in the routers

8ec1815

update func comments

3931940

add quirks to schema

ab7d26d

fixup from rebase

1e4ccd9

Laura-Danielle force-pushed the RLIQ-345-mutable-configs branch from 416d8bc to 1e4ccd9 Compare June 30, 2023 07:29

fix tavern tests

876da2b

Laura-Danielle requested a review from a team as a code owner June 30, 2023 09:09

sfoster1 requested changes Jun 30, 2023

View reviewed changes

Laura-Danielle added 4 commits July 3, 2023 17:10

pass in the path to the pipette override directory

52ce36f

fixup robot server tests

5e662f9

fix model validation robot server

8220bf7

remove test

bcb297e

sfoster1 requested changes Jul 6, 2023

View reviewed changes

Laura-Danielle added 2 commits July 7, 2023 17:13

fix failing test

3562b46

add pieptte name back in

627227c

sfoster1 approved these changes Jul 7, 2023

View reviewed changes

make sure there is a quirks key

7392756

sanni-t approved these changes Jul 10, 2023

View reviewed changes

fix dumb linter

ce448c4

Laura-Danielle merged commit 9e7c35c into edge Jul 11, 2023

Laura-Danielle deleted the RLIQ-345-mutable-configs branch July 11, 2023 12:25

sanni-t mentioned this pull request Jul 11, 2023

feat(api, app, shared-data, pd): use new flex pipette names in backend & clients #13082

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Laura-Danielle commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading

sfoster1 left a comment

sfoster1 left a comment

sfoster1 Jun 30, 2023

Laura-Danielle Jul 3, 2023

sfoster1 Jul 6, 2023

sfoster1 Jun 30, 2023

sfoster1 left a comment

sfoster1 Jul 6, 2023

sfoster1 Jul 6, 2023

Laura-Danielle Jul 6, 2023

sfoster1 left a comment

sanni-t left a comment


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

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

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

Conversation

Laura-Danielle commented Jun 23, 2023 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Jun 23, 2023 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 Jun 30, 2023

Choose a reason for hiding this comment

Laura-Danielle Jul 3, 2023

Choose a reason for hiding this comment

sfoster1 Jul 6, 2023

Choose a reason for hiding this comment

sfoster1 Jun 30, 2023

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 Jul 6, 2023

Choose a reason for hiding this comment

sfoster1 Jul 6, 2023

Choose a reason for hiding this comment

Laura-Danielle Jul 6, 2023

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

Laura-Danielle commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading