Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(robot-server): do not save calibration offsets if an error occurs #13168

Merged

Conversation

Laura-Danielle
Copy link
Contributor

Overview

A fix for the behavior where pipette offsets get saved even if an error occurs during the calibration flow.

Test Plan

Follow steps in this ticket to reproduce.

Changelog

  • Save the offset inside the robot server function rather than the pipette function
  • Ensure that we're raising on edge detection failed errors as well

Review requests

Please test on a robot, let me know if anything seems off.

Risk assessment

Low. Should be fixing a small bug.

A fix for the behavior where pipette offsets get saved even if an error occurs
@Laura-Danielle Laura-Danielle requested a review from a team July 26, 2023 14:49
@Laura-Danielle Laura-Danielle requested a review from a team as a code owner July 26, 2023 14:49
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #13168 (0077b45) into internal-release_0.14.0 (57b7c9b) will increase coverage by 0.06%.
Report is 10 commits behind head on internal-release_0.14.0.
The diff coverage is n/a.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           internal-release_0.14.0   #13168      +/-   ##
===========================================================
+ Coverage                    72.53%   72.59%   +0.06%     
===========================================================
  Files                         2359     2372      +13     
  Lines                        64994    65416     +422     
  Branches                      7213     7209       -4     
===========================================================
+ Hits                         47143    47490     +347     
- Misses                       16105    16181      +76     
+ Partials                      1746     1745       -1     
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)

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

see 15 files with indirect coverage changes

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea but it's sort of a weird middle ground between a refactor and a new feature.

I think the simplest way to do this is to change ot3_calibration.calibrate_pipette to put the save_instrument_offset_call after the finally? (although we definitely want to restore the old one). But if we want a whole new different function like this, then we should have calibrate_pipette call the new function instead of just copy-pasting it; and also we should probably remove calibrate_pipette since I don't think anything else would use it now.

Let's do one of those two (either alter calibrate_pipette, or implement it via find_pipette_offset and/or delete it).

@ahiuchingau
Copy link
Contributor

Do you think we'll need to fix the gripper offset saving mechanism as well? I don't see why this behavior would only be happening to the pipettes and not the gripper as well? What am i missing?

@Laura-Danielle
Copy link
Contributor Author

Do you think we'll need to fix the gripper offset saving mechanism as well? I don't see why this behavior would only be happening to the pipettes and not the gripper as well? What am i missing?

@ahiuchingau the gripper already does the mechanism above. Previously, the pipette was saving calibration inside the try/except of the ot3_calibration.py function.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks good to me!

@Laura-Danielle Laura-Danielle merged commit 1388e25 into internal-release_0.14.0 Jul 28, 2023
@Laura-Danielle Laura-Danielle deleted the fix-do-not-save-offset-on-failure branch July 28, 2023 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants