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

refactor(api): remove feature-flag dependency from calibration_storage #13833

Merged
merged 12 commits into from
Oct 31, 2023

Conversation

fsinapi
Copy link
Contributor

@fsinapi fsinapi commented Oct 24, 2023

Overview

As part of the migration of hardware-related code out of the opentrons package and into the opentrons_hardware package, we need to move dependencies on the advanced settings module out of any hardware code to avoid having multiple processes access & possibly modify the same settings file simultaneously.

The calibration_storage module decides what implementation (OT2 vs OT3) to expose by checking the feature flag enable_ot3_hardware_controller. This poses an issue because the module is imported by a few files inside of hardware_control, so this PR takes the platform logic out of calibration_storage and offloads that logic to packages importing the module.

In most of the places that the opentrons package imports calibration_storage, the code is either for an OT2 or OT3 so it's easy to hard code the imports. In robot-server, we just utilize the feature-flag logic that had been done internally by calibration_storage.

Test Plan

Test calibration flows on ot2/flex and make sure stuff is still working correctly!!! Pushing from this branch should still detect any calibration files already on a robot, and new files should be fully compatible with a downgrade because the code should be the same.

  • Test on OT-2
  • Test on Flex

Changelog

Review requests

Some tests ended up getting duplicated into ot2 and ot3 versions. I tried to get clever about importlib to parameterize based on the robot model but, in the end, the ot2 and ot3 subpackages are two different chunks of (very similar) code and I personally think it's okay to test them separately. Any objections?

A lot of robot-server tests were already only using the OT-2 code so I didn't bother trying to restructure them to test the OT-3 files too. Does someone think it's important to expand the tests as a part of this PR?

Risk assessment

High if it accidentally imports OT2 code on a Flex (or vice versa), but should be pretty easy to verify it doesn't.

@fsinapi fsinapi requested review from sfoster1 and a team October 24, 2023 18:23
@fsinapi fsinapi self-assigned this Oct 24, 2023
@fsinapi fsinapi requested a review from a team as a code owner October 24, 2023 18:23
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #13833 (eb536b2) into edge (7d68d92) will decrease coverage by 0.01%.
Report is 1 commits behind head on edge.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13833      +/-   ##
==========================================
- Coverage   70.82%   70.82%   -0.01%     
==========================================
  Files        2498     2498              
  Lines       70181    70162      -19     
  Branches     8589     8589              
==========================================
- Hits        49709    49692      -17     
+ Misses      18383    18381       -2     
  Partials     2089     2089              
Flag Coverage Δ
app 68.86% <ø> (ø)
components 63.38% <ø> (ø)
g-code-testing 96.44% <ø> (ø)
labware-library 49.17% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 45.45% <ø> (ø)
react-api-client 65.82% <ø> (ø)
step-generation 84.95% <ø> (ø)

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

Files Coverage Δ
api/src/opentrons/config/reset.py 92.30% <ø> (+3.41%) ⬆️
robot-server/robot_server/hardware.py 80.88% <ø> (-0.28%) ⬇️
.../robot_server/robot/calibration/check/user_flow.py 88.17% <ø> (-0.08%) ⬇️
...r/robot_server/robot/calibration/deck/user_flow.py 96.23% <ø> (-0.03%) ⬇️
...rver/robot/calibration/pipette_offset/user_flow.py 84.70% <ø> (-0.06%) ⬇️
...obot-server/robot_server/robot/calibration/util.py 100.00% <ø> (ø)
...er/robot_server/service/legacy/routers/settings.py 100.00% <ø> (ø)

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.

This is good but I think we can take it slightly farther - once we can remove conditions from the ot2 calibration stuff (have it import from ot2 directly) and from reset I think we're done

- removed tip length cal utils on OT3 because they are not used ever
# Conflicts:
#	robot-server/tests/service/legacy/routers/test_settings.py
@fsinapi fsinapi requested a review from sfoster1 October 30, 2023 20:35
@fsinapi
Copy link
Contributor Author

fsinapi commented Oct 31, 2023

Tested on OT-2 & Flex. Tested that

  • Can go through calibration flows via the app
  • Old calibration data is still valid
  • Calibration data created with this branch is compatible with a downgrade to edge

Flex is having some bugs with creating maintenance runs outside of protocol setup, but that also seems present on edge so I think it's unrelated to these 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.

This looks awesome! Nice!

@fsinapi fsinapi merged commit 2a06a3a into edge Oct 31, 2023
46 of 47 checks passed
@fsinapi fsinapi deleted the RET-1382-Refactor-calibration-imports branch October 31, 2023 15:26
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.

2 participants