-
Notifications
You must be signed in to change notification settings - Fork 179
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
fix(robot-server): do not save calibration offsets if an error occurs #13168
Conversation
A fix for the behavior where pipette offsets get saved even if an error occurs
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this 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).
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 |
There was a problem hiding this 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!
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
Review requests
Please test on a robot, let me know if anything seems off.
Risk assessment
Low. Should be fixing a small bug.