refactor(calibration-storage): Use pydantic models for data validation, add OT-3 pydantic models #11520

Laura-Danielle · 2022-09-29T20:09:38Z

Overview

Closes RLIQ-118. The objective of this PR is to add pydantic models to support better validation on calibration data in general. Please check out this confluence doc for more information on the pydantic schemas and this confluence doc for more information on what the file system looks like in general. There are a lot of loose ends around calibration data that need to be tied up in separate PRs of which I've documented in this epic.

In-memory caching for calibration data will be in a follow-up PR. Mike's comments from here have been addressed in this new branch aside from file access asynchrony which will be addressed in a separate ticket. Apologies for the commit wonky-ness. I couldn't get the branches to rebase properly.

Changelog

Added two modules under calibration_storage to separate OT-2 and OT-3 calibration types
Separated instrument calibration data based on OT-2 and OT-3 (pipette(s) will be handled in a follow-up PR which changes the data schema of pipettes to support OT-3 pipette functionality).
Moved the dataclass objects of the calibration data models (which were only used in the hardware controller) to the hardware controller and converted from the pydantic schemas in the hardware controller once the data was being loaded in.
Switched to using only pydantic models in the robot-server
Added more test coverage on the calibration storage module in general
Modified tests where necessary

Review requests

Please test this on a robot, let me know if you have any changes you want to the actual data shape as well.

Test Plan

Please view this confluence document for a more detailed test plan and check list.

Run through all OT-2 calibration flows
Check that all calibration data endpoints (calibration/pipette_offset and calibation/tip_length) still work as expected
Check that all robot calibrations are still being properly loaded and applied in the hardware controller
Check that downgrading the software keeps all calibrations that were calibrated with the new software

Risk assessment

High. This touches a lot of flakey code and needs a lot of QA eyes. See test plan above.

codecov · 2022-09-29T20:13:33Z

Codecov Report

Merging #11520 (db8c636) into edge (2dd6aac) will decrease coverage by 10.09%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             edge   #11520       +/-   ##
===========================================
- Coverage   74.58%   64.49%   -10.10%     
===========================================
  Files        2074     1416      -658     
  Lines       57827    24790    -33037     
  Branches     6104     5990      -114     
===========================================
- Hits        43133    15988    -27145     
+ Misses      13265     7381     -5884     
+ Partials     1429     1421        -8

Flag	Coverage Δ
api	`?`
g-code-testing	`?`
hardware	`?`
hardware-testing	`∅ <ø> (∅)`
notify-server	`88.26% <ø> (ø)`
ot3-gravimetric-test	`?`
robot-server	`?`
shared-data	`?`
update-server	`?`
usb-bridge	`?`

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

Impacted Files	Coverage Δ
...c/components/steplist/MultiSelectToolbar/index.tsx	`86.79% <0.00%> (ø)`
...rc/opentrons/calibration_storage/file_operators.py
api/src/opentrons/calibration_storage/helpers.py
api/src/opentrons/calibration_storage/types.py
api/src/opentrons/config/reset.py
api/src/opentrons/hardware_control/__init__.py
api/src/opentrons/hardware_control/api.py
api/src/opentrons/hardware_control/ot3api.py
...ardware_control/protocols/instrument_configurer.py
...rc/opentrons/hardware_control/robot_calibration.py
... and 645 more

sfoster1

Some smaller things in code review, but one bigger one: I don't really like the use of "Schema" in pydantic model names. They're schemas, but they're also instantiated with data and passed around as data container objects. It's weird to have a function that modifies the data of a schema, it's a mismatch of kind: if you have a function that operates on a schema, i expect the schema to be different afterwards - but here, it's just the data in an instance. We should either use these objects only to verify data but not store it, or call them Models or something

shared-data/LICENSE

api/src/opentrons/calibration_storage/ot2/mark_bad_calibration.py

mcous

Many of the added tests assert on implementation details (private methods / fields) of the code under test, which is inappropriate and does not actually test that the public API works. Code and/or tests should structured such that behavior can be validated against public, used-in-production methods

mcous · 2022-10-18T16:40:01Z

api/src/opentrons/calibration_storage/file_operators.py

@@ -52,7 +72,7 @@ def read_cal_file(

 def save_to_file(
 filepath: StrPath,
- data: typing.Mapping[str, typing.Any],
+ data: typing.Union[BaseModel, typing.Dict[str, typing.Any], typing.Any],


That last typing.Any effectively removes any type constraints on this argument. Why is it needed?

Suggested change

data: typing.Union[BaseModel, typing.Dict[str, typing.Any], typing.Any],

data: typing.Union[BaseModel, typing.Dict[str, typing.Any]],

api/src/opentrons/calibration_storage/file_operators.py

mcous · 2022-10-18T16:47:37Z

api/src/opentrons/calibration_storage/ot2/deck_attitude.py

+ if file.name == "deck_calibration.json":
+ try:
+ return v1.DeckCalibrationSchema(**io.read_cal_file(file.path))
+ except (json.JSONDecodeError, ValidationError):


What does this mean? Should it be logged?

Sure I'll throw in a log. It effectively means the data is bad.

api/src/opentrons/calibration_storage/ot2/deck_attitude.py

api/tests/opentrons/calibration_storage/test_gripper_offset.py

api/tests/opentrons/calibration_storage/test_pipette_offset.py

api/tests/opentrons/calibration_storage/test_tip_length.py

api/src/opentrons/calibration_storage/file_operators.py

Laura-Danielle · 2022-10-27T16:55:59Z

I want to get this merged ASAP because it's blocking some other calibration work. Any other clean-ups requested (aside from anything found during testing) will be documented here and addressed after our 'feature freeze'.

…chemas for all calibration data We are now using pydantic models to store and modify calibration data. Most OT-3 calibration data no longer requires tips to calibrate with, so the schemas reflect this fact. re #RLIQ-118

…are stored We should really keep the dataclasses in hardware controller -- as that's the only place they are used. While here, also separated out the concept of 'robot' calibration and 'instrument' calibration.

…re controller

…ok-up tip length calibration

…ns to calibration storage

… storage module g

… calibration storage

…ble names 'get', 'delete' etc can be rather confusing when using the calibration_storage module. Instead, we should name each file based on the calibration data they handle.

sfoster1

Looks good to me! Would love to remove conditional imports at some point but not this PR.

sfoster1 · 2022-10-27T17:00:05Z

api/src/opentrons/calibration_storage/__init__.py

+)
+from .ot2.pipette_offset import get_all_pipette_offset_calibrations
+
+if config.feature_flags.enable_ot3_hardware_controller():


This is fine for now but can we add a todo to make this something that is not based on a feature flag and not conditionally imported at some point? I'd love for fewer decisions to get made during import, and having our code support both machines at runtime is an important thing for offline analysis and general sanity

I agree. If we do need to check a feature flag, it should be done at function run time, not import time. This global config singleton that needs to be initialized in order for imports to work is an antipattern in our codebase

If it's too much for this PR, that's fine, but I was imagining a lightweight facade in my earlier review:

def save_robot_deck_attitude(...) -> ...: if is_ot3: from .ot3 import deck_attitude as ot3_deck_attitude ot3_deck_attitude.save_robot_deck_attitude(...) else: ...

mcous

I'm ok to move forward with this PR and address additional items in a follow up PR for speed, but in order of importance, I'm still concerned about these issues:

Conditional imports/exports at the top level of the calibration_storage module should be avoided at all costs
Tests for calibration_storage are written with if/else, which seems to suggest that a test suite run will only check OT-2 or OT-3, rather than OT-2 and OT-3
File/directory reading/writing concerns still leak out of file_operators.py
Since file_operators.py has test coverage, tests for other calibration_storage components could mock out file_operators for speed and useful design feedback, but they do not
- Tests will be slower because there will be a lot of hitting the filesystem
- We have redundant test coverage of file operations

mcous · 2022-10-28T16:09:04Z

api/src/opentrons/calibration_storage/__init__.py

+)
+from .ot2.pipette_offset import get_all_pipette_offset_calibrations
+
+if config.feature_flags.enable_ot3_hardware_controller():


I agree. If we do need to check a feature flag, it should be done at function run time, not import time. This global config singleton that needs to be initialized in order for imports to work is an antipattern in our codebase

If it's too much for this PR, that's fine, but I was imagining a lightweight facade in my earlier review:

def save_robot_deck_attitude(...) -> ...: if is_ot3: from .ot3 import deck_attitude as ot3_deck_attitude ot3_deck_attitude.save_robot_deck_attitude(...) else: ...

api/src/opentrons/calibration_storage/ot2/deck_attitude.py

api/src/opentrons/calibration_storage/ot2/pipette_offset.py

mcous · 2022-10-28T16:14:27Z

api/src/opentrons/calibration_storage/ot2/pipette_offset.py

+ """
+ pipette_calibration_dir = Path(config.get_opentrons_path("pipette_calibration_dir"))
+ pipette_calibration_list = []
+ for filepath in pipette_calibration_dir.glob("**/*.json"):


Would recommend moving to a io.get_json_files_from_directory or something similar for testability

mcous · 2022-10-28T16:15:59Z

api/src/opentrons/calibration_storage/ot2/tip_length.py

+ with open(custom_tiprack_path, "rb") as f:
+ return typing.cast(
+ "LabwareDefinition",
+ json.loads(f.read().decode("utf-8")),
+ )


Should this be going through io.read_json_file or similar?

api/src/opentrons/calibration_storage/ot3/deck_attitude.py

mcous · 2022-10-28T16:20:26Z

api/tests/opentrons/calibration_storage/test_deck_attitude.py

+@pytest.fixture(autouse=True)
+def reload_module(robot_model: "RobotModel") -> None:
+ importlib.reload(opentrons.calibration_storage)


This is a warning sign, is it due to all the conditional importing and re-exporting? An explicit facade would avoid this problem.

importlib.reload, in my experience, is not terribly reliable or intuitive

mcous · 2022-10-28T16:23:10Z

api/tests/opentrons/calibration_storage/test_deck_attitude.py

+ )
+
+ assert get_robot_deck_attitude() is None
+ if robot_model == "OT-3 Standard":


if/else in tests should almost always be avoided. Can this be parametrized, instead?

mcous · 2022-10-28T16:24:55Z

api/tests/opentrons/calibration_storage/test_deck_attitude.py

+
+
+@pytest.fixture
+def starting_calibration_data(


Should we write these tests as isolated unit tests and mock out file_operators instead, given that file_operators has unit tests?

Laura-Danielle · 2022-10-28T18:36:37Z

I'm ok to move forward with this PR and address additional items in a follow up PR for speed, but in order of importance, I'm still concerned about these issues:

Conditional imports/exports at the top level of the calibration_storage module should be avoided at all costs

Tests for calibration_storage are written with if/else, which seems to suggest that a test suite run will only check OT-2 or OT-3, rather than OT-2 and OT-3

File/directory reading/writing concerns still leak out of file_operators.py

Since file_operators.py has test coverage, tests for other calibration_storage components could mock out file_operators for speed and useful design feedback, but they do not

Tests will be slower because there will be a lot of hitting the filesystem

We have redundant test coverage of file operations

Will hopefully get to all of this next week!

Laura-Danielle requested review from a team as code owners September 29, 2022 20:09

Laura-Danielle mentioned this pull request Sep 29, 2022

refactor(calibration-storage): Cache robot calibrations and use pydantic models for data validation #11505

Closed

4 tasks

sfoster1 requested changes Oct 17, 2022

View reviewed changes

shared-data/LICENSE Outdated Show resolved Hide resolved

api/src/opentrons/calibration_storage/ot2/mark_bad_calibration.py Outdated Show resolved Hide resolved

laviera assigned mcous and sanni-t Oct 18, 2022

mcous suggested changes Oct 18, 2022

View reviewed changes

ahiuchingau mentioned this pull request Oct 26, 2022

feat(api): add support for EVT gripper calibration #11616

Merged

Laura-Danielle added 20 commits October 27, 2022 12:59

add schemas

928a214

cache calibrations in memory

b80ec27

make changes in functions to grab data

3b9797f

refactor(hardware-control): modify where dataclasses for calibration …

80b256e

…are stored We should really keep the dataclasses in hardware controller -- as that's the only place they are used. While here, also separated out the concept of 'robot' calibration and 'instrument' calibration.

refactor(hardware-control): modify tests to reflect changes to hardwa…

521f3f2

…re controller

refactor(protocol-api): do not use calibration storage directly to lo…

d4bf8d2

…ok-up tip length calibration

refactor(robot-server): make initial changes required for modificatio…

b32086c

…ns to calibration storage

refactor(config): switch delete file based on feature flag

64db039

feat(calibration-storage): add more verbose tests for the calibration…

6ecbde1

… storage module g

refactor(calibration-storage): follow-up changes from new tests

95359c2

refactor(api): Modify the api to match changes to calibration storage

25f047b

refactor(robot-server): Modify robot server to reflect changes to the…

92b6f82

… calibration storage

linter and formatting for api

f7a2556

linter and formatting for robot-server

e94db29

remove unnecessary cache

66b4738

refactor(calibration-storage): rename calibration files to more sensi…

543b1f6

…ble names 'get', 'delete' etc can be rather confusing when using the calibration_storage module. Instead, we should name each file based on the calibration data they handle.

Api linting and tests

2099e72

robot server formatting

07fcfb6

fixup reset test

7eccdac

Laura-Danielle added 13 commits October 27, 2022 12:59

refactor(robot-server): fix things up for robot server tests

3d3a2f4

fixup path for cal check test

fd9d8a6

Change naming of the calibration data to models from schemas

fb6549e

change mark bad typing

0a24285

Remove accidental license generation in shared data

a0959a5

remove unneccessary helper function for deck calibration

7cb8a6c

only use mkdir in file operators file

42b330f

key mount type by mount

893b442

make everything use Path

6221387

remove check on private function, modify how files are accessed

7b800ac

utilize a conditional import

d2c5183

linter fixups from imports

975c377

add file operator tests, remove TypeVars

785f8ee

sfoster1 approved these changes Oct 27, 2022

View reviewed changes

fix hardware testing imports

0a1c8f2

Laura-Danielle force-pushed the RLIQ-118-add-schemas-file-name-changes branch from ffe4bef to 0a1c8f2 Compare October 27, 2022 18:58

Laura-Danielle added 4 commits October 27, 2022 17:43

fix: deck calibration status endpoint

07eec8a

fix: ignore index file of tip length

ee31faf

fix: further hardware testing import fixes

f70fda4

fix: correctly parse the deck calibration data

61bfdd7

mcous approved these changes Oct 28, 2022

View reviewed changes

pr feedback

db8c636

Laura-Danielle merged commit eb13afd into edge Oct 28, 2022

Laura-Danielle deleted the RLIQ-118-add-schemas-file-name-changes branch October 28, 2022 19:30

mcous mentioned this pull request Feb 16, 2023

refactor(api): fix unreleased regression in tip length load #12174

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(calibration-storage): Use pydantic models for data validation, add OT-3 pydantic models #11520

refactor(calibration-storage): Use pydantic models for data validation, add OT-3 pydantic models #11520

Laura-Danielle commented Sep 29, 2022 •

edited

Loading

codecov bot commented Sep 29, 2022 •

edited

Loading

sfoster1 left a comment

mcous left a comment

mcous Oct 18, 2022

mcous Oct 18, 2022

Laura-Danielle Oct 21, 2022

Laura-Danielle commented Oct 27, 2022

sfoster1 left a comment

sfoster1 Oct 27, 2022

mcous Oct 28, 2022

mcous left a comment

mcous Oct 28, 2022

mcous Oct 28, 2022

mcous Oct 28, 2022

mcous Oct 28, 2022

mcous Oct 28, 2022

mcous Oct 28, 2022

Laura-Danielle commented Oct 28, 2022

	data: typing.Union[BaseModel, typing.Dict[str, typing.Any], typing.Any],
	data: typing.Union[BaseModel, typing.Dict[str, typing.Any]],

refactor(calibration-storage): Use pydantic models for data validation, add OT-3 pydantic models #11520

refactor(calibration-storage): Use pydantic models for data validation, add OT-3 pydantic models #11520

Conversation

Laura-Danielle commented Sep 29, 2022 • edited Loading

Overview

Changelog

Review requests

Test Plan

Risk assessment

codecov bot commented Sep 29, 2022 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

mcous 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

Laura-Danielle commented Oct 27, 2022

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

mcous 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

Laura-Danielle commented Oct 28, 2022

Laura-Danielle commented Sep 29, 2022 •

edited

Loading

codecov bot commented Sep 29, 2022 •

edited

Loading