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(app): Tolerate old tip length calibration records without a uri field #14622

Merged
merged 5 commits into from
Mar 11, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Mar 11, 2024

Overview

This fixes RESC-215.

Apparently, old tip length calibration records don't have a uri field. (It looks like uri was introduced in #7136, v4.1.0.) We used to safely drop these records via this except ValidationError block. But PR #14512 started accessing the uri field manually, accidentally bringing it outside the coverage of the except block and turning it into an HTTP 500 error.

Test Plan

To reproduce this manually, you can download the data/ directory from RESC-215 and run this in the api project directory:

OT_API_TIP_LENGTH_CALIBRATION_DIR=/path/to/downloaded/data/tip_lengths \
  pipenv run python -c \
  'from opentrons import calibration_storage; print(calibration_storage.get_all_tip_length_calibrations())'

On v7.1.1, it will work. On v7.2.0, it will raise an error. On this branch, it will work again.

I've also added a unit test.

Changelog

  • If uri is missing from an old tip length calibration record, silently ignore that record.
  • Fix a misplaced JSONDecodeError catch. (This turned out to not be the problem, but it's still probably a good idea to fix.)
  • Add exception info to a bunch of parsing warnings to make these problems easier to debug. (These warnings turned out to not cover this problem, but it's still probably a good idea to fix.)

Review requests

  • Is there a better way to handle these records than skipping over them entirely?

Risk assessment

Low.

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.57%. Comparing base (be34672) to head (8cd870b).
Report is 57 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #14622   +/-   ##
=======================================
  Coverage   67.57%   67.57%           
=======================================
  Files        2521     2521           
  Lines       72237    72237           
  Branches     9306     9306           
=======================================
  Hits        48816    48816           
  Misses      21226    21226           
  Partials     2195     2195           
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)

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

Files Coverage Δ
api/src/opentrons/config/robot_configs.py 77.41% <ø> (ø)

@SyntaxColoring SyntaxColoring marked this pull request as ready for review March 11, 2024 17:03
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner March 11, 2024 17:03
Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

Nice debugging and fixing max :) I also don't know when we added in URI because I thought we've had that component for a long time. Maybe I only added it in when doing calibration stuff for the flex, but it's been a while so I can't remember.

@SyntaxColoring SyntaxColoring changed the base branch from edge to chore_release-7.2.1 March 11, 2024 19:13
@SyntaxColoring SyntaxColoring merged commit 3dfbb6f into chore_release-7.2.1 Mar 11, 2024
22 checks passed
@SyntaxColoring SyntaxColoring deleted the tlc_not_migrating branch March 11, 2024 19:16
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.

None yet

2 participants