opentrons
opentrons copied to clipboard
refactor(robot-server): Safely unpickle renamed types
Overview
This PR mitigates a source of fragility in robot-server's database. It lets robot-server unpickle objects created by prior versions of itself, even if the object's Python class has been moved or renamed in the meantime. Formerly, if the class were moved or renamed, it would cause "class not found" errors.
This work goes towards RSS-98. See that ticket for more background.
Changelog
- Hard-code a list of all Python types that
robot-serverhas been pickling in its database. This list is intended to be exhaustive. I generated it with the help of a script that recursively scans our Pydantic models and outputs every field type that it encounters: dump_leaf_types.py - When unpickling a type from that list, map it to its modern equivalent. The mapping is hard-coded, but it should be easy for us to adapt as we move and rename things in the future.
- Apply this mapping to all pickle columns in
robot-server's database:analysis.completed_analysis,run.state_summary, andrun.commands.
Review requests
- Does the way I've written the mapping look maintainable? I'm a little worried about a future find+replace accidentally changing an
original_namestring without us noticing. Does anyone have ideas to make it more foolproof, or make it stick out more in code review? - I didn't add unit tests for the custom unpickler because it's already covered by integration tests such as these. But I could add them without too much trouble. Do we want that?
- Do we want to check my
dump_leaf_types.pyscript in to this repo somewhere?
Risk assessment
Alone, this PR is low-risk. It's well-covered by existing integration tests, and it's written to fall back to prior behavior in case it encounters an unexpected type.
Next steps
This PR isn't sufficient to close RSS-98 because it remains easy for us to accidentally pickle something not included in the _legacy_ot_types list. As discussed in that ticket, we want to migrate entirely off of pickles and use JSON instead. There's a # TODO for this in the code.
Codecov Report
Merging #11569 (71b0b73) into edge (71b0b73) will not change coverage. The diff coverage is
n/a.
:exclamation: Current head 71b0b73 differs from pull request most recent head 1a5707d. Consider uploading reports for the commit 1a5707d to get more accurate results
@@ Coverage Diff @@
## edge #11569 +/- ##
=======================================
Coverage 75.01% 75.01%
=======================================
Files 2046 2046
Lines 57160 57160
Branches 5909 5909
=======================================
Hits 42880 42880
Misses 12883 12883
Partials 1397 1397
| Flag | Coverage Δ | |
|---|---|---|
| app | 75.52% <0.00%> (ø) |
|
| hardware-testing | ∅ <0.00%> (∅) |
|
| notify-server | 88.26% <0.00%> (ø) |
|
| protocol-designer | 45.85% <0.00%> (ø) |
|
| shared-data | 84.96% <0.00%> (ø) |
|
| step-generation | 88.39% <0.00%> (ø) |
Flags with carried forward coverage won't be shown. Click here to find out more.