refactor(app, api): calibration check middleware #6781

Laura-Danielle · 2020-10-15T13:36:01Z

Overview

This PR ports calibration check over to the new architecture style/design of the other calibration flows. It also changes the name from Robot Calibration Check -> Robot Calibration Health Check. Closes #6725 and closes #6710.

Changelog

Remove all pipette specific states
Add in an active tiprack and active pipette to the flow for tracking purposes
Add in states that allow you to "transition" from the first pipette to the second pipette
Remove all intermediate calibration threshold feedback screens for health check in the app
Add a return tip modal to transition from the first to second pip or to exit
Port cal check state machine and screens over to the new "simple state machine" and shared screens
Change robot calibration check -> calibration health check
Remove analytics call-sites (but keep the functions around for potential future use)
Hold the comparisons in a dataclass rather than a dictionary, and sort them by pipette rank
Fix all tests related to the above
Load tipracks in one at a time to slot 8

Limitations and/or TODOs

The robot needs to send over the last error state along with a diagnosis (i.e. if comparing point one was the only one to fail send over pip offset re-do state)
The error states need to be overhauled to match the new system
We should load tipracks based on the pipette offset rather than pip attached (if applicable)
We should check if all the flows have a tip upon exit rather than extraneously moving the robot to the tiprack
We need to add in the ability to load a cal check block etc for tip length calibration checks
The flow needs to be updated to match the new screens, so more states will probably need to be added in

Review requests

Please test on a robot, and let me know if there are other places I should throw TODOs in. I think at this point the PR should just be merged and all remaining work delegated to other tickets.

Risk assessment

High. This is completely changing how the calibration check session worked previously, although we are not releasing immediately, we should be weary of any new bugs that might get introduced.

b-cooper

awesome! 🔑

b-cooper · 2020-10-16T16:07:07Z

app/src/components/CalibrationPanels/TipConfirmation.js

@@ -58,7 +65,13 @@ export function TipConfirmation(props: CalibrationPanelProps): React.Node {
    sendCommands({ command: moveCommandString })
  }

-  return (
+  const isBadTipPickUp =


We should move this cal check specific step/panel combination into it's own file in the CheckCalibration component directory. That way these generic panels don't have to serve every special case. This example is branching only on currentStep (within a check cal session), which is exactly what the parent component step/panel mapping does, it should just slot in there.

b-cooper · 2020-10-16T16:13:11Z

app/src/components/CheckCalibration/index.js


-const ROBOT_CALIBRATION_CHECK_SUBTITLE = 'Robot calibration heatlh check'
+const ROBOT_CALIBRATION_CHECK_SUBTITLE = 'Calibration heatlh check'


Suggested change

const ROBOT_CALIBRATION_CHECK_SUBTITLE = 'Calibration heatlh check'

const ROBOT_CALIBRATION_CHECK_SUBTITLE = 'Calibration health check'

sfoster1

Lots of little change requests to keep in mind

sfoster1 · 2020-10-20T13:23:46Z

app/src/components/CalibrationPanels/SaveXYPoint.js

-
-    if (moveCommandString) {
-      commands = [...commands, { command: moveCommandString }]
+    let commands = null


does the stuff in 190:196 only apply to isHealthCheck === true? if so consider moving it into the if block on 185:187. Also consider making this a const-binding using an anonymous function or a logical expression like a ternary, eg

const commands = isHealthCheck ?( (finalCommand && activePipette?.rank ? ... : ...) : ...)

sfoster1 · 2020-10-20T13:25:24Z

app/src/components/CalibrationPanels/DeckSetup.js

@@ -26,9 +26,9 @@ import { CalibrationLabwareRender } from './CalibrationLabwareRender'
 import styles from './styles.css'

 const DECK_SETUP_WITH_BLOCK_PROMPT =
-  'Place full tip rack and Calibration Block on the deck within their designated slots as illustrated below.'
+  'Place full tip rack(s) and Calibration Block on the deck within their designated slots as illustrated below.'


it's a little thing, but in all flows except cal check this will never have multiple tip racks. consider either making separate values or making this a function that accepts the number of tipracks or something.

sfoster1 · 2020-10-20T13:27:02Z

app/src/components/CalibrationPanels/SaveZPoint.js

-      { command: Sessions.sharedCalCommands.MOVE_TO_POINT_ONE }
-    )
+  let continueCommands
+  if (sessionType === Sessions.SESSION_TYPE_CALIBRATION_HEALTH_CHECK) {


Consider using const-binding here with a ternary or something.

sfoster1 · 2020-10-20T13:28:07Z

app/src/components/CalibrationPanels/types.js

@@ -16,4 +18,8 @@ export type CalibrationPanelProps = {|
  sessionType: SessionType,
  calBlock?: CalibrationLabware | null,
  shouldPerformTipLength?: boolean | null,
+  checkBothPipettes?: boolean | null,


This is growing a whole lot of optional attributes. While we don't have to do it now, we should at the very least drop a todo to refactor this into a union type discriminated on the session type.

sfoster1 · 2020-10-20T13:32:03Z

app/src/components/CheckCalibration/ResultsSummary.js

-    const comparison = comparisonsByStep[step]
-    if (comparison && comparison.exceedsThreshold) {
-      return comparison
+  ].reduce((acc, step): CalibrationHealthCheckComparison | null => {


We needed this last-failed logic in the old system because of the overlapping of the calibration components; I don't think we need it anymore. Honestly since we're already going to have the marked-bad logic on the server side, it might be worth having the server just tell us which calibrations are a problem, and generating the troubleshooting screen based on that. In the interim, I think we definitely don't need this logic here.

sfoster1 · 2020-10-20T13:51:00Z

robot-server/robot_server/robot/calibration/models.py

@@ -20,7 +20,8 @@ class SessionCreateParams(BaseModel):
    )
    tipRackDefinition: Optional[dict] = Field(
        None,
-        description='The full labware definition of the tip rack to calibrate.'
+        description='The full tiprack definition(s)'


this is definitely set up to be a single tip rack definition, if we want it to be possibly-multiple we should probably make the type List[dict] and change the session creators

I'll change the description back. Basically was having a lot of issues w/ pydantic models -- I wanted to have an optional create param for check calibration but it was mixing up deck cal w/ check cal when I didn't specify the tipracks I wanted to use. Then I thought about adding it to this model, but realized it just wasn't worth it at this point.

sfoster1 · 2020-10-20T13:52:53Z

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

@@ -120,6 +121,10 @@ def __init__(self,
    def hardware(self) -> ThreadManager:
        return self._hardware

+    @property


good change but does it belong in this pr?

I made the change before your other PR that was put up. I'm going to be rebasing anyways so I'll probably just remove this to make it easier.

sfoster1 · 2020-10-20T13:53:36Z

robot-server/robot_server/service/session/router.py

@@ -80,7 +80,8 @@ async def delete_session_handler(
    session_obj = get_session(manager=session_manager,
                              session_id=sessionId,
                              api_router=router)
-
+    log.info("delete was called! ")


remember to remove/lower level

sfoster1 · 2020-10-20T13:53:49Z

robot-server/robot_server/service/session/router.py

@@ -80,7 +80,8 @@ async def delete_session_handler(
    session_obj = get_session(manager=session_manager,
                              session_id=sessionId,
                              api_router=router)
-
+    log.info("delete was called! ")
+    log.info(f"session object {session_obj}")


remember to remove/lower level

sfoster1 · 2020-10-20T13:54:29Z

robot-server/robot_server/service/session/session_types/check_session.py


    @classmethod
    async def create(cls,
                     configuration: SessionConfiguration,
                     instance_meta: SessionMetaData) -> BaseSession:
        """Create an instance"""
+        # (lc, 10-19-2020) For now, only pass in empty tipracks. We cannot
+        # have a session model with an optional tiprack for session
+        # create params right now because of the pydantic union problem.


rather than having it be optional, having it be a list that therefore may be empty is probably a good choice

codecov · 2020-10-21T14:26:51Z

Codecov Report

❗ No coverage uploaded for pull request base (chore_release-4.0.0-beta.0@297bf06). Click here to learn what that means.
The diff coverage is n/a.

@@                      Coverage Diff                      @@
##             chore_release-4.0.0-beta.0    #6781   +/-   ##
=============================================================
  Coverage                              ?   87.53%           
=============================================================
  Files                                 ?      101           
  Lines                                 ?     4411           
  Branches                              ?        0           
=============================================================
  Hits                                  ?     3861           
  Misses                                ?      550           
  Partials                              ?        0

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 297bf06...2ed0fde. Read the comment docs.

sfoster1

Code all looks good to me, will test some

sfoster1 · 2020-10-21T19:17:13Z

robot-server/robot_server/service/session/session_types/check_session.py

+        # off after
+        session_controls_lights =\
+            not configuration.hardware.get_lights()['rails']
+        # await configuration.hardware.cache_instruments()


sfoster1

lgtm!

…heck_middleware

When the user selects and saves the trash surface as their tip length calibration target, send an event to intercom so that support can follow up about fulfilling a calibration block. This also brings back the intercom event epics and bindings removed in #6781, without the calibration check session end bindings.

Laura-Danielle added app Affects the `app` project api Affects the `api` project WIP labels Oct 15, 2020

b-cooper approved these changes Oct 16, 2020

View reviewed changes

sfoster1 requested changes Oct 20, 2020

View reviewed changes

Laura-Danielle force-pushed the refactor_calibration_check_middleware branch from 8f225c6 to 91f20e5 Compare October 21, 2020 14:23

Laura-Danielle changed the base branch from edge to chore_release-4.0.0-beta.0 October 21, 2020 14:23

Laura-Danielle marked this pull request as ready for review October 21, 2020 14:24

Laura-Danielle requested review from a team as code owners October 21, 2020 14:24

Laura-Danielle requested review from Kadee80 and sfoster1 October 21, 2020 14:24

Laura-Danielle added ready for review and removed WIP labels Oct 21, 2020

sfoster1 reviewed Oct 21, 2020

View reviewed changes

Laura-Danielle added 13 commits October 21, 2020 22:04

Lay groundwork for robot server refactor

4e36799

Lay the ground work for app refactor

3f6eacf

Modify functions to match new behavior

666e584

app: use shared panels

73fe740

Robot server - fix linter errors

b6f00a9

App - fix lint errors

cec65d0

Fixup more linter errors and tests

2b66e4e

app - change around types

ae9dbd7

Add additional check command, lint fixes

70beb37

app - fix up session type called

f2aafea

app - more screen fixes

614b98f

robot-server - change around some states

71e705b

app - fixup flow errors

46155d6

Laura-Danielle and others added 14 commits October 21, 2020 22:05

Fixup: add return tip state and handle second pipette properly

302205a

app - remove analytics, fixup more screens

3afe558

Remove intercom analytics

e012706

handled reading length from instruments when instr is null

13eebef

app - send correct command to server in return tip

c192ed4

Add a new dummy transition

67e2cab

fix register initial point buffer for comparingHeight

4ddb985

read channels and max vol from pip config

2936ada

Robot server fix up all tests except tavern

0a74abb

Fixup: remaining tests and address comments on PR

0de1edf

Fixup - js linter errors

2a7c469

Fixup: comparison states and safety buffer

05f8926

Fixup: ensure the correct comparisons are being made for each step

f6aae2a

Fixup - rebase error

d958f33

Laura-Danielle force-pushed the refactor_calibration_check_middleware branch from fc44bcb to d958f33 Compare October 22, 2020 02:15

Laura-Danielle requested a review from sfoster1 October 22, 2020 13:07

sfoster1 approved these changes Oct 22, 2020

View reviewed changes

Merge branch 'chore_release-4.0.0-beta.0' into refactor_calibration_c…

2ed0fde

…heck_middleware

Laura-Danielle merged commit 25c9eb9 into chore_release-4.0.0-beta.0 Oct 22, 2020

Laura-Danielle deleted the refactor_calibration_check_middleware branch October 22, 2020 13:41

sfoster1 mentioned this pull request Nov 2, 2020

feat(app): Send intercom event on no-cal-block selected #6893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(app, api): calibration check middleware #6781

refactor(app, api): calibration check middleware #6781

Laura-Danielle commented Oct 15, 2020 •

edited

Loading

b-cooper left a comment

b-cooper Oct 16, 2020

b-cooper Oct 16, 2020

sfoster1 left a comment

sfoster1 Oct 20, 2020

sfoster1 Oct 20, 2020

sfoster1 Oct 20, 2020

sfoster1 Oct 20, 2020

sfoster1 Oct 20, 2020

sfoster1 Oct 20, 2020

Laura-Danielle Oct 20, 2020

sfoster1 Oct 20, 2020

Laura-Danielle Oct 20, 2020

sfoster1 Oct 20, 2020

sfoster1 Oct 20, 2020

sfoster1 Oct 20, 2020

codecov bot commented Oct 21, 2020 •

edited

Loading

sfoster1 left a comment

sfoster1 Oct 21, 2020

Laura-Danielle Oct 22, 2020

sfoster1 left a comment


		const ROBOT_CALIBRATION_CHECK_SUBTITLE = 'Robot calibration heatlh check'
		const ROBOT_CALIBRATION_CHECK_SUBTITLE = 'Calibration heatlh check'

refactor(app, api): calibration check middleware #6781

refactor(app, api): calibration check middleware #6781

Conversation

Laura-Danielle commented Oct 15, 2020 • edited Loading

Overview

Changelog

Limitations and/or TODOs

Review requests

Risk assessment

b-cooper left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 21, 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

sfoster1 left a comment

Choose a reason for hiding this comment

Laura-Danielle commented Oct 15, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading