feat(api): Mark calibrations as bad when determined they exceed threshold #6918

Laura-Danielle · 2020-11-04T20:32:21Z

Overview

Closes #6730. Now, when a user completes calibration check, the app will be able to detect calibrations that were marked bad.

Changelog

Add a method in calibration storage to mark a file with a bad status
Add a method in the user flow to mark a calibration as bad if it exceeds threshold.
Refactor the valid calibration check
Add a test on the user flow to check the new behavior
Fix up an enum check

Review requests

Any other tests you would like? Test on a robot, force your calibrations to be above the threshold and you should see
that the app picks up bad calibrations afterwards.

It will only get resolved once you re-do whichever calibration was marked bad.

Risk assessment

Low.

codecov · 2020-11-04T20:32:28Z

Codecov Report

Merging #6918 (b686d83) into chore_release-4.0.0-beta.0 (8cba814) will decrease coverage by 0.13%.
The diff coverage is 90.76%.

@@                      Coverage Diff                       @@
##           chore_release-4.0.0-beta.0    #6918      +/-   ##
==============================================================
- Coverage                       73.20%   73.07%   -0.14%     
==============================================================
  Files                             263      244      -19     
  Lines                           21034    20145     -889     
==============================================================
- Hits                            15398    14720     -678     
+ Misses                           5636     5425     -211

Impacted Files	Coverage Δ
...r/robot_server/robot/calibration/deck/user_flow.py	`84.06% <ø> (+0.54%)`	⬆️
...rver/robot/calibration/pipette_offset/user_flow.py	`79.16% <ø> (ø)`
.../robot_server/robot/calibration/check/user_flow.py	`72.70% <89.83%> (+1.69%)`	⬆️
...r/robot_server/robot/calibration/helper_classes.py	`77.14% <100.00%> (+1.75%)`	⬆️
...obot-server/robot_server/robot/calibration/util.py	`94.56% <100.00%> (+0.05%)`	⬆️
opentrons/calibration_storage/modify.py	`76.00% <0.00%> (-3.05%)`	⬇️
opentrons/legacy_api/containers/__init__.py	`46.21% <0.00%> (-0.51%)`	⬇️
opentrons/protocol_api/instrument_context.py	`89.77% <0.00%> (ø)`
notify-server/notify_server/network/connection.py
update-server/otupdate/buildroot/file_actions.py
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cba814...b686d83. Read the comment docs.

sfoster1

I think we can make this a little bit better by not going too overboard with using the same function for marking everything bad, since the caller calls different things regardless. Either something like a data transformation only function; or a separate set of functions for each kind of calibration data or something I think would make the mark_bad function itself (or whatever replaces it) a bit nicer to read.

sfoster1 · 2020-11-04T22:07:35Z

api/src/opentrons/calibration_storage/modify.py

+        calibration: typing.Union[
+            local_types.DeckCalibration,
+            local_types.PipetteOffsetCalibration,
+            typing.Tuple[float, str, 'Labware']],


It's a bit odd that this is a tuple and the other stuff isn't - can we get a nice type for tip length calibration in line with the other kinds?

Also, rather than having a union, I think we should probably type this as a series of overload defs, but that's a personal preference

In fact, since whatever calls this is going to know what kind of calibration it has, it may make a lot more sense to have mark_bad be a function that transforms a calibration type rather than a function that transforms and saves it. Since the calibration types are all dataclasses, that means that we can stararg expand them. Something like:

@typing.override def mark_bad(calibration: local_types.DeckCalibration) -> local_types.DeckCalibration: ... @typing.override def mark_bad(calibration: local_types.PipetteOffsetCalibration) -> local_types.PipetteOffsetCalibration: ... @typing.override def mark_bad(calibration: local_types.TipLengthCalibration) -> local_types.TipLengthCalibration: ... def mark_bad(calibration): caldict = calibration.as_dict() caldict['status'] = local_types.CalibrationStatus(markedBad=True, ...) return type(calibration)(**caldict)

and then the callers do

bad_cal = mark_bad(calibration) save_deck_calibration(bad_cal)

sfoster1 · 2020-11-04T22:09:17Z

api/src/opentrons/calibration_storage/modify.py

        pip_id: str, mount: Mount,
-        tiprack_hash: str, tiprack_uri: str):
+        tiprack_hash: str, tiprack_uri: str,
+        cal_status: typing.Optional[local_types.CalibrationStatus] = None):


don't need both Optional and = None, mypy will figure it out

sfoster1 · 2020-11-04T22:09:34Z

api/src/opentrons/calibration_storage/modify.py

@@ -197,20 +203,25 @@ def save_robot_deck_attitude(
        transform: local_types.AttitudeMatrix,
        pip_id: typing.Optional[str],
        lw_hash: typing.Optional[str],
-        source: local_types.SourceType = None):
+        source: local_types.SourceType = None,
+        cal_status: typing.Optional[local_types.CalibrationStatus] = None):


Don't need both Optional and =None, mypy will figure it out

sfoster1 · 2020-11-04T22:09:53Z

api/src/opentrons/calibration_storage/modify.py

@@ -106,7 +107,9 @@ def save_labware_calibration(
 def create_tip_length_data(
        definition: 'LabwareDefinition',
        parent: str,
-        length: float) -> 'PipTipLengthCalibration':
+        length: float,
+        cal_status: typing.Optional[local_types.CalibrationStatus] = None


don't need both Optional and =None, mypy will figure it out

sfoster1 · 2020-11-04T22:16:14Z

robot-server/robot_server/robot/calibration/check/user_flow.py

+            not self._is_checking_both_mounts()
+        pipette_state = is_second_pipette or only_one_pipette
+        if self.current_state == State.comparingTip:
+            modify.mark_bad(


Yeah see here we're already calling things differently depending on what we know the types to be so I don't think we need a fully generic function, or at least a fully generic mark-and-save function.

…hold closes #6730

…nt of pipettes

sfoster1

Let's remove that commented assert but the code looks good to me

sfoster1 · 2020-11-09T18:01:26Z

api/src/opentrons/calibration_storage/modify.py

@@ -121,22 +124,25 @@ def create_tip_length_data(
    # assert labware._is_tiprack, \


This commented out assert is part of the comment block above. I want to keep it in as a reminder for what we had previously utilized in that method.

Laura-Danielle added api Affects the `api` project ready for review labels Nov 4, 2020

Laura-Danielle requested a review from a team November 4, 2020 20:32

Laura-Danielle requested a review from a team as a code owner November 4, 2020 20:32

Laura-Danielle requested review from ahiuchingau and removed request for a team November 4, 2020 20:32

sfoster1 requested changes Nov 4, 2020

View reviewed changes

Laura-Danielle force-pushed the cal_check_mark_bad branch from f158112 to 448093e Compare November 5, 2020 19:14

Laura-Danielle requested review from a team as code owners November 9, 2020 15:31

Laura-Danielle requested review from shlokamin and sfoster1 and removed request for a team November 9, 2020 15:31

Laura-Danielle added 8 commits November 9, 2020 12:05

feat(api): Mark calibrations as bad when determined they exceed thres…

a6c078a

…hold closes #6730

Reload robot cal on exit

d0e672b

Reduce amount of repetitive code

17ca7d5

Modify return types for tip length

25f647b

Fixup tip length comparison and movement to tiprack

c4f3b8c

Fixup tests

126db45

Correctly check for the deck calibration status depending on the amou…

8368d19

…nt of pipettes

fixup from rebase

c3d8612

Laura-Danielle force-pushed the cal_check_mark_bad branch from a987d99 to c3d8612 Compare November 9, 2020 17:12

sfoster1 approved these changes Nov 9, 2020

View reviewed changes

Save initial height comparison point correctly

b686d83

nusrat813 self-requested a review November 9, 2020 21:27

nusrat813 approved these changes Nov 9, 2020

View reviewed changes

Laura-Danielle merged commit ac3a866 into chore_release-4.0.0-beta.0 Nov 9, 2020

Laura-Danielle deleted the cal_check_mark_bad branch November 9, 2020 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Mark calibrations as bad when determined they exceed threshold #6918

feat(api): Mark calibrations as bad when determined they exceed threshold #6918

Laura-Danielle commented Nov 4, 2020

codecov bot commented Nov 4, 2020 •

edited

Loading

sfoster1 left a comment

sfoster1 Nov 4, 2020

sfoster1 Nov 4, 2020

sfoster1 Nov 4, 2020

sfoster1 Nov 4, 2020

sfoster1 Nov 4, 2020

sfoster1 Nov 4, 2020

sfoster1 Nov 4, 2020

sfoster1 left a comment

sfoster1 Nov 9, 2020

Laura-Danielle Nov 9, 2020

		@@ -121,22 +124,25 @@ def create_tip_length_data(
		# assert labware._is_tiprack, \

feat(api): Mark calibrations as bad when determined they exceed threshold #6918

feat(api): Mark calibrations as bad when determined they exceed threshold #6918

Conversation

Laura-Danielle commented Nov 4, 2020

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Nov 4, 2020 • edited Loading

Codecov Report

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

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

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

codecov bot commented Nov 4, 2020 •

edited

Loading