opentrons icon indicating copy to clipboard operation
opentrons copied to clipboard

refactor(robot-server): Safely unpickle renamed types

Open SyntaxColoring opened this issue 3 years ago • 1 comments

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-server has 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, and run.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_name string 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.py script 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.

SyntaxColoring avatar Oct 13 '22 17:10 SyntaxColoring

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

Impacted file tree graph

@@           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.

codecov[bot] avatar Oct 13 '22 17:10 codecov[bot]