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

chore(api): disable legacy calibration accessors on flex #13513

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Sep 11, 2023

We had a lot of spammy logs in the api log about missing or malformed pipette and tip length calibration.

The missing calibrations should not be logged at info level - these are polled and this is a level state, so it just ends up being spammy. The malformed calibrations though should be, and it's weird that we were getting them.

The culprit is that <7.0.0 apps poll routes to get pipette offset and tip length because that's what you do on the ot2, and the implementations of those routes hardcode the ot2 calibration models - reasonable, since we completely replaced them for the flex. So you'd only get those logs if an old app was polling the robot.

To fix that, I made the routes shortcircuit with a 403 forbidden / not allowed on hardware error. This won't affect the flex because the app doesn't check that route on flex.

These messages about missing calibrations are completely meaningless on
the flex and are incredibly spammy; pull them.

They might have meaning on the OT-2 but honestly we have a
_lot_ of ui surrounding them so whatever.
@sfoster1 sfoster1 requested a review from a team as a code owner September 11, 2023 21:02
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #13513 (e29442b) into chore_release-7.0.0 (124f184) will decrease coverage by 0.01%.
Report is 13 commits behind head on chore_release-7.0.0.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.0   #13513      +/-   ##
=======================================================
- Coverage                71.36%   71.35%   -0.01%     
=======================================================
  Files                     2418     2418              
  Lines                    67937    67933       -4     
  Branches                  7886     7886              
=======================================================
- Hits                     48480    48476       -4     
  Misses                   17616    17616              
  Partials                  1841     1841              
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.

Files Changed Coverage Δ
...protocol_engine/resources/labware_data_provider.py 100.00% <ø> (ø)
robot-server/robot_server/hardware.py 81.15% <ø> (-0.27%) ⬇️
...rver/robot_server/service/pipette_offset/router.py 93.33% <ø> (-0.22%) ⬇️
...t-server/robot_server/service/tip_length/router.py 96.66% <ø> (-0.21%) ⬇️

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

🥫

log.warning(
log.debug(
Copy link
Contributor

Choose a reason for hiding this comment

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

I might leave this as a warning, unless this is actually more false-positive than true-positive in practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is, unfortunately, a frequent occurrence when a 6.3.1 app is viewing a flex because it's polling the old pipette offset route. which i might want to just disable in the server or something

Comment on lines -46 to 50
log.warning(
log.debug(
f"Tip length calibration is malformed for {tiprack} on {pipette_id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I might leave this as a warning, unless this is actually more false-positive than true-positive in practice.

Copy link
Member Author

Choose a reason for hiding this comment

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

this just never should ever print ever so i can make it a warning

Make the legacy routes for tip_length and pipette_offset 503 on flex and
not try and get the offsets using the old calibration accessors, since
they are hardcoded to use the ot2 versions of calibration which do not
apply to the flex and dumped errors in the logs all the time.

the api side change supports testing.
@sfoster1 sfoster1 requested a review from a team as a code owner September 12, 2023 17:11
@sfoster1 sfoster1 changed the title chore(api): remove spammy log messages chore(api): disable legacy calibration accessors on flex Sep 12, 2023
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Agreed with blocking off these routes if they aren't how calibration works on the Flex. Thanks!

Comment on lines 50 to +53
async def get_all_pipette_offset_calibrations(
pipette_id: Optional[str] = None, mount: Optional[pip_models.MountType] = None
pipette_id: Optional[str] = None,
mount: Optional[pip_models.MountType] = None,
_: API = Depends(get_ot2_hardware),
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure I understand correctly: on a Flex, even retrieving pipette calibrations through these endpoints was doing the wrong thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This code was hardcoded to select the ot2 pipette offset loaders.

@sfoster1 sfoster1 merged commit a1cde96 into chore_release-7.0.0 Sep 12, 2023
16 of 25 checks passed
@sfoster1 sfoster1 deleted the remove-api-log-spam branch September 12, 2023 19:42
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