refactor: Migrate Python projects from Pydantic v1 to v2 #14871
+28,382
−52,674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ahiuchingau
force-pushed
the
chore_update-pydantic-v2
branch
from
e83d1b5 to
80a7000
Compare
robot-server/setup.py
Outdated
Comment on lines
59
to
62
| "fastapi==0.99.1", | ||
| "fastapi>=0.100.0", | ||
| "python-dotenv==1.0.1", | ||
| "python-multipart==0.0.6", | ||
| "pydantic==1.10.12", | ||
| "pydantic>=2.0.0,<3", |
There was a problem hiding this comment.
Do these need to be pinned to a specific version like they were before?
There was a problem hiding this comment.
Please don't pin to exact versions unless strictly necessary. See e.g. #11905
There was a problem hiding this comment.
This is robot-server, which is not published on PyPI, so it should not affect what gets pulled in when you do pip install opentrons.
I'm a bit shaky on our packaging mechanisms, but I think this specific setup.py may not even affect what we install on robots. I'm sure we can make this less confusing somehow. But for the sake of not changing too many things in this PR, I think we'll keep the ==.
[Edit: Opened a discussion here for Opentrons people and added this to the review requests.]
ahiuchingau
force-pushed
the
chore_update-pydantic-v2
branch
from
949e25a to
2728cca
Compare
Conflicts: * api/Pipfile.lock (took edge, will need to re-lock) * api/src/opentrons/calibration_storage/deck_configuration.py * api/src/opentrons/cli/analyze.py * api/src/opentrons/protocol_api/core/engine/protocol.py * api/src/opentrons/protocol_engine/commands/calibration/calibrate_gripper.py * api/src/opentrons/protocol_engine/commands/calibration/calibrate_pipette.py * api/src/opentrons/protocol_engine/commands/command.py * api/src/opentrons/protocol_engine/commands/custom.py * api/src/opentrons/protocol_engine/types.py * api/src/opentrons/protocol_runner/legacy_command_mapper.py * api/tests/opentrons/cli/test_cli.py * api/tests/opentrons/protocol_engine/state/command_fixtures.py * api/tests/opentrons/protocol_engine/state/test_addressable_area_store.py * api/tests/opentrons/protocol_engine/state/test_geometry_view.py * api/tests/opentrons/protocol_runner/test_protocol_runner.py * robot-server/Pipfile * robot-server/Pipfile.lock (took edge, will need to re-lock) * robot-server/robot_server/deck_configuration/defaults.py * robot-server/robot_server/persistence/pydantic.py * shared-data/command/schemas/8.json (took edge, will need to regenerate) * shared-data/python/tests/deck/test_typechecks.py
There was a problem hiding this comment.
All of the diffs in analyses-snapshot-testing/ are the same. Some floats used to get serialized like 0 and now they're getting serialized like 0.0. I've already looked through these files, so I don't think you have to.
There is one exception: https://github.com/Opentrons/opentrons/pull/14871/files#r1884999286
These were added by earlier commits in this PR, and I think commit b5b28b6 supersedes them.
robot-server/robot_server/persistence/_migrations/_up_to_3_worker.py
Outdated
Show resolved
Hide resolved
This was previously deleted before this PR, in commit 50d3208, but it snuck back in. Maybe during merge conflict resolution or something.
This appears unnecessary now. When this comment was written, this PR was on Pydantic 2.6, and now we're on 2.9. Maybe that fixed it?
I think I was talking about wanting to move the list loop into pydantic-core, by parsing this through a TypeAdapter(list[Command]) instead of a TypeAdapter(Command). That might be worth trying, but not in this PR.
Integrate with the new mmFromEdge param in TouchTipParams.
Comment on lines
44
to
+56
| def dict(self, *args: Any, **kwargs: Any) -> Dict[str, Any]: | ||
| """Always exclude `None` when serializing to an object. | ||
|
|
||
| The OpenAPI spec marks `Optional` BaseModel fields as omittable, but | ||
| not nullable. This `dict` method override ensures that `null` is never | ||
| returned in a response, which would violate the spec. | ||
| With Pydantic v1, the OpenAPI spec described `Optional`(i.e., possibly | ||
| `None`-valued) fields as omittable, but not nullable. This did not match | ||
| Pydantic's actual serialization behavior, which serialized Python `None` to | ||
| JSON `null` by default. This method override fixed the mismatch by making | ||
| Pydantic omit the field from serialization instead. | ||
|
|
||
| With Pydantic v2, the OpenAPI spec does describe `Optional` fields as nullable, | ||
| matching Pydantic's serialization behavior. We therefore no longer need this | ||
| override to make them match. However, removing this override and changing | ||
| serialization behavior at this point would risk breaking things on the client. |
This was referenced Dec 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[PR originally by @ahiuchingau, current description by @SyntaxColoring]
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:
Affected projects that only run in CI and on our laptops:
Test Plan
When the https://github.com/Opentrons/buildroot and https://github.com/Opentrons/oe-core changes are ready:
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:
setup.py,Pipfile, andPipfile.lockfiles 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.Review requests
= Noneadditions look correct?bump-pydanticadded 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 toNonemay not be what we actually want, e.g. it may not match the underlying JSON schema.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_apiandopentrons.protocol_engine, ...