Skip to content

Commit

Permalink
refactor: Migrate Python projects from Pydantic v1 to v2 (#14871)
Browse files Browse the repository at this point in the history
# Overview

This updates our robot-stack Python projects from Pydantic v1 to v2.

Closes PLAT-326 and GitHub issue #13983.

Affected projects that run on the robot:

 - api
 - robot-server
 - shared-data
 - hardware
 - server-utils
 - system-server
 - performance-metrics (re-lock only, depends on shared-data)

Affected projects that only run in CI and on our laptops:

 - g-code-testing
 - hardware-testing
 - abr-testing


# Test Plan

When the https://github.com/Opentrons/buildroot and
https://github.com/Opentrons/oe-core changes are ready:

* [x] OT-2
* [x] Make sure all servers still boot without errors (`systemctl
status`)
    * [ ] No unexpected mismatches reported by `pip check`
* I get "robot-server 8.2.0 has requirement pydantic==2.9.0, but you
have pydantic 2.9.2." I think we can fix this in a follow-up.
* [x] Try running a protocol or something and make sure the server
doesn't return an error or take an obscenely long time to respond.
* [x] Flex
* [x] Make sure all servers still boot on a Flex without errors
(`systemctl status`)
    * [x] No unexpected mismatches reported by `pip check`
* [x] Try running a protocol or something and make sure the server
doesn't return an error or take an obscenely long time to respond.


Beyond that, this PR touches a million little things in a million little
ways, so it's difficult to test. We should try to merge it early in the
release cycle to give us time to shake things out.

# Changelog

See https://docs.pydantic.dev/latest/migration/#migration-guide for
everything that has changed from Pydantic v1 to v2.

The basic methodology of this PR is:

* Update `setup.py`, `Pipfile`, and `Pipfile.lock` files to a new
Pydantic version, trying to follow Unfortunately, FastAPI is tightly
coupled to Pydantic, so we need to update it too. The FastAPI bump is
kept minimal.
* Run [bump-pydantic](https://github.com/pydantic/bump-pydantic) on all
projects. This automatically does a lot of the grunt work, but it does
need manual follow-up.
* Manually fix up lots of little things
* Do global find+replaces for some [trivial
renames](https://docs.pydantic.dev/latest/migration/#changes-to-pydanticbasemodel).
I moved some of this to [a separate PR,
#17123](#17123), because the
GitHub web UI was struggling with the big diff.


# Review requests

- Do the setup.py, Pipfile, and Pipfile.lock files look good? We're
trying to align on a definition of "good"
[here](https://opentrons.atlassian.net/wiki/spaces/RPDO/pages/4671602797/Python+dependency+management).
- Do all Pydantic migrations look correct? Reference:
https://docs.pydantic.dev/latest/migration/#migration-guide
- Do the rewritten validators look correct? Some of these needed manual
intervention.
- Do the `= None` additions look correct? `bump-pydantic` added these
automatically to match prior parse behavior—see
https://docs.pydantic.dev/latest/migration/#required-optional-and-nullable-fields.
As far as I can tell, these additions are always safe. But defaulting to
`None` may not be what we actually want, e.g. it may not match the
underlying JSON schema.
- This was a long-lived PR that changed hands several times, so there
are definitely vestigial things left over from earlier attempts. If
something looks unexplained or out of place to you, please speak up.

# Risk assessment

## Performance

This *will,* at least in the short term, make robot-server take much
longer to start up, and make the tests much slower. This is a known
Pydantic v2 problem (pydantic/pydantic#6768
etc.). Earlier testing on a Flex found it slowed from 46s to 1m54s
(2.5x). I don't think we'll be able to get it back down to Pydantic v1
times, but some proofs of concept suggest that we can mitigate it to
only ~1.6x slower. There are some ideas in EXEC-1060.

## Correctness

High-risk due to the breadth of changes: storage reads and writes, HTTP
requests and responses, communication between the
`opentrons.protocol_api` and `opentrons.protocol_engine`, ...

* [Pydantic's type coercion behavior has
changed](https://docs.pydantic.dev/latest/migration/#validator-behavior-changes),
trending in the direction of doing less type coercion. This is a good
direction, but it is basically impossible to audit affected one Python
protocol in the snapshot tests—see the inline review comments below.

---------

Co-authored-by: Max Marrone <[email protected]>
Co-authored-by: Seth Foster <[email protected]>
  • Loading branch information
3 people authored Dec 18, 2024
1 parent a2e4bdd commit 60398ea
Show file tree
Hide file tree
Showing 336 changed files with 28,385 additions and 52,676 deletions.
1,539 changes: 869 additions & 670 deletions abr-testing/Pipfile.lock

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -10789,9 +10789,9 @@
"pipetteId": "UUID",
"wellLocation": {
"offset": {
"x": 0,
"y": 0,
"z": 0
"x": 0.0,
"y": 0.0,
"z": 0.0
},
"origin": "default"
},
Expand Down Expand Up @@ -10921,9 +10921,9 @@
"pipetteId": "UUID",
"wellLocation": {
"offset": {
"x": 0,
"y": 0,
"z": 0
"x": 0.0,
"y": 0.0,
"z": 0.0
},
"origin": "default"
},
Expand Down Expand Up @@ -11121,9 +11121,9 @@
"pipetteId": "UUID",
"wellLocation": {
"offset": {
"x": 0,
"y": 0,
"z": 0
"x": 0.0,
"y": 0.0,
"z": 0.0
},
"origin": "default"
},
Expand Down Expand Up @@ -11253,9 +11253,9 @@
"pipetteId": "UUID",
"wellLocation": {
"offset": {
"x": 0,
"y": 0,
"z": 0
"x": 0.0,
"y": 0.0,
"z": 0.0
},
"origin": "default"
},
Expand Down
Loading

0 comments on commit 60398ea

Please sign in to comment.