feat(robot-server): add pipette calibration data to instruments endpoint #12498

b-cooper · 2023-04-13T21:55:03Z

Overview

Supply offset, source and last modified date of pipette offset calibration data for flex pipettes
from the GET instruments endpoint

re RSS-167

Test Plan

Push to an OT3 and check that the attached pipette's calibration data is available in the /instruments endpoint.
Push to an OT2 with an attached pipette and check that there is no calibration data available but all other pipette data is available

Changelog

added get_instrument_offset method to OT3API and PipetteHandler
added pipette calibration offset field as optional to PipetteData in instruments response model

Review requests

We went back and forth on whether it's good to add pipette offset data to PipetteDict. We might end up doing that eventually, but that would require updating some code related to OT2 pipettes as well in order to either not provide any calibration data for them or provide the relevant data that's different from OT3 calibration data. After weighing the pros/cons/ unknowns of PipetteDict, we decided to fetch the data in a way that doesn't cause changes to any existing data and types. But let me know if that's not the case.
Needs testing on an OT3

Risk assessment

Low. I've tried to not change any hardware controller components that are in use right now so in the worst case, this affects just the instruments endpoint.

…ndpoint Supply offset, source and last modified date of pipette offset calibration data for flex pipettes from the GET instruments endpoint re RSS-167

codecov · 2023-04-13T22:05:34Z

Codecov Report

Merging #12498 (274fec7) into edge (32c898a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             edge   #12498   +/-   ##
=======================================
  Coverage   73.67%   73.67%           
=======================================
  Files        2252     2252           
  Lines       61976    61976           
  Branches     6517     6517           
=======================================
  Hits        45658    45658           
  Misses      14757    14757           
  Partials     1561     1561

Flag	Coverage Δ
notify-server	`89.13% <ø> (ø)`

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

Impacted Files	Coverage Δ
api/src/opentrons/hardware_control/ot3api.py	`79.41% <ø> (ø)`

sanni-t

Initial thoughts. Mostly looks good! I can update the tests on Monday.

api/src/opentrons/hardware_control/dev_types.py

robot-server/robot_server/instruments/instrument_models.py

api/src/opentrons/hardware_control/instruments/ot2/pipette_handler.py

…dler.py

sanni-t

I think it's the right approach to not include OT2 instruments' calibration data here so thanks for making that change!
After looking into it a bit more, I think it would be best to not change PipetteDict. The field we are adding is OT3 specific while PipetteDict is used by OT2 as well. If we try to include OT2's pipette offset, then the field ends up accepting different types (even though they are both called PipetteOffsetByPipetteMount, they are actually two different types). With this, the final thing that makes me reluctant to modifying PipetteDict is that it's known that there are plans to refactor it and I don't know if adding this field fits with those plans.

So that said, I think most of what this PR is doing in the server can stay the same, except, instead of trying to fetch calibration info from PipetteDict, I would tap into the OT3API and create a getter to fetch pipette calibration data from it. Then use this method in the instruments endpoint router.

sanni-t · 2023-04-19T17:19:41Z

api/src/opentrons/hardware_control/dev_types.py

@@ -74,6 +75,7 @@ class PipetteDict(InstrumentDict):
    model: PipetteModel
    back_compat_names: List[PipetteName]
    pipette_id: str
+    pipette_offset: Optional[PipetteOffsetByPipetteMount]


I am a bit surprised this isn't making OT2 tests to fail because this type is shared between OT3 and OT2 instruments and we aren't setting this field for OT2 instruments.

… ot3api & pipette handler

robot-server/robot_server/instruments/instrument_models.py

ahiuchingau

Just a doc string change request, but otherwise LGTM

api/src/opentrons/hardware_control/ot3api.py

Co-authored-by: Alise Au <[email protected]>

sanni-t

Three people worked on this PR and we all (+ @ahiuchingau) agree this looks good so we're good to merge!

api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py

…dler.py

robot-server/tests/instruments/test_router.py

TamarZanzouri

tested on OT-2 and OT-3. shows only on OT-3 as expected!

b-cooper added 2 commits April 13, 2023 15:29

add pipete cal data to instruments endpoint

977eec6

refactor(robot-server): add pipette calibration data to instruments e…

378ab37

…ndpoint Supply offset, source and last modified date of pipette offset calibration data for flex pipettes from the GET instruments endpoint re RSS-167

b-cooper force-pushed the robot-server_add-cal-to-pipette-data branch from ec4ce6c to 378ab37 Compare April 13, 2023 21:57

b-cooper requested review from TamarZanzouri and Laura-Danielle April 13, 2023 21:57

b-cooper requested a review from sanni-t April 14, 2023 14:55

sanni-t reviewed Apr 14, 2023

View reviewed changes

api/src/opentrons/hardware_control/dev_types.py Outdated Show resolved Hide resolved

robot-server/robot_server/instruments/instrument_models.py Outdated Show resolved Hide resolved

TamarZanzouri added 4 commits April 17, 2023 14:42

fixed linting. added OT-2 pipette offset and fixed failing test

3c5c994

changed test input and access

112b705

pr suggestion fixes

b5eb6d8

changed data class for test and fixed accessing

cd819d9

TamarZanzouri reviewed Apr 18, 2023

View reviewed changes

api/src/opentrons/hardware_control/instruments/ot2/pipette_handler.py Outdated Show resolved Hide resolved

Update api/src/opentrons/hardware_control/instruments/ot2/pipette_han…

389b5ee

…dler.py

TamarZanzouri marked this pull request as ready for review April 18, 2023 19:06

TamarZanzouri requested a review from a team as a code owner April 18, 2023 19:06

TamarZanzouri requested a review from sanni-t April 18, 2023 19:07

linter

c683e2b

TamarZanzouri changed the title ~~refactor(robot-server): add pipette calibration data to instruments endpoint~~ feature(robot-server): add pipette calibration data to instruments endpoint Apr 19, 2023

removed ot-2 pipette cal from result

b9bb35d

sanni-t reviewed Apr 19, 2023

View reviewed changes

linting

a79e982

sanni-t changed the title ~~feature(robot-server): add pipette calibration data to instruments endpoint~~ feat(robot-server): add pipette calibration data to instruments endpoint Apr 19, 2023

sanni-t added 2 commits April 19, 2023 16:42

removed offset data from pipette dict, added pipette offset getter to…

2688481

… ot3api & pipette handler

updated workflow to see if CI can skip ot3 tests now

667e6b1

sanni-t requested a review from a team as a code owner April 19, 2023 21:10

sanni-t reviewed Apr 19, 2023

View reviewed changes

robot-server/robot_server/instruments/instrument_models.py Outdated Show resolved Hide resolved

Update robot-server/robot_server/instruments/instrument_models.py

6f322d6

ahiuchingau approved these changes Apr 19, 2023

View reviewed changes

api/src/opentrons/hardware_control/ot3api.py Outdated Show resolved Hide resolved

Update api/src/opentrons/hardware_control/ot3api.py

d6e3f79

Co-authored-by: Alise Au <[email protected]>

sanni-t approved these changes Apr 19, 2023

View reviewed changes

TamarZanzouri reviewed Apr 20, 2023

View reviewed changes

api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py Outdated Show resolved Hide resolved

Update api/src/opentrons/hardware_control/instruments/ot3/pipette_han…

5def6fd

…dler.py

TamarZanzouri reviewed Apr 20, 2023

View reviewed changes

robot-server/tests/instruments/test_router.py Outdated Show resolved Hide resolved

removed comment

274fec7

TamarZanzouri approved these changes Apr 20, 2023

View reviewed changes

TamarZanzouri merged commit 2f46fe9 into edge Apr 20, 2023

b-cooper deleted the robot-server_add-cal-to-pipette-data branch April 20, 2023 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(robot-server): add pipette calibration data to instruments endpoint #12498

feat(robot-server): add pipette calibration data to instruments endpoint #12498

b-cooper commented Apr 13, 2023 •

edited by jira bot

Loading

codecov bot commented Apr 13, 2023 •

edited

Loading

sanni-t left a comment

sanni-t left a comment

sanni-t Apr 19, 2023

ahiuchingau left a comment

sanni-t left a comment

TamarZanzouri left a comment

feat(robot-server): add pipette calibration data to instruments endpoint #12498

feat(robot-server): add pipette calibration data to instruments endpoint #12498

Conversation

b-cooper commented Apr 13, 2023 • edited by jira bot Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Apr 13, 2023 • edited Loading

Codecov Report

sanni-t left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

sanni-t Apr 19, 2023

Choose a reason for hiding this comment

ahiuchingau left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

TamarZanzouri left a comment

Choose a reason for hiding this comment

b-cooper commented Apr 13, 2023 •

edited by jira bot

Loading

codecov bot commented Apr 13, 2023 •

edited

Loading