-
Notifications
You must be signed in to change notification settings - Fork 178
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
chore: Migrate Python projects from Pydantic v1 to v2 #14871
base: edge
Are you sure you want to change the base?
Conversation
e83d1b5
to
80a7000
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm about 60% through the changes so far. Mostly looks great, only a few comments and questions:
DatetimeType = typing.Annotated[ | ||
datetime, | ||
PlainSerializer(lambda x: x.isoformat(), when_used="json"), | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inferring that this is replacing the old json_encoders
config. Does something need to replace the old json_decoders
config, to match?
DatetimeType = typing.Annotated[ | ||
datetime, | ||
PlainSerializer(lambda x: x.isoformat(), when_used="json"), | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
# Each time a TypeAdapter is instantiated, it will construct a new validator and | ||
# serializer. To improve performance, TypeAdapters are instantiated once. | ||
# See https://docs.pydantic.dev/latest/concepts/performance/#typeadapter-instantiated-once | ||
CommandCreateAdatper: TypeAdapter[CommandCreate] = TypeAdapter(CommandCreate) # type: ignore[arg-type] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Typo:
Adatper
instead ofAdapter
- What is this
TypeAdapter
doing?
@@ -230,7 +230,7 @@ def handle_action(self, action: Action) -> None: # noqa: C901 | |||
# request > command mapping, figure out how to type precisely | |||
# (or wait for a future mypy version that can figure it out). | |||
# For now, unit tests cover mapping every request type | |||
queued_command = action.request._CommandCls.construct( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be .model_construct
, for performance reasons?
class Vec3f(BaseModel, Generic[NumberType]): | ||
"""A 3D Vector.""" | ||
|
||
x: NumberType | ||
y: NumberType | ||
z: NumberType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The name
Vec3f
comes from "vector of 3float
s." If this is generic across other kinds ofNumberType
, let's rename it to justVec3
. - Since we're really pushing this deep down into common shared code, I'm worried that someone will come along later and add an
= 0
default for something without realizing that it will affect a million things. Let's spell out that this is specifically for vectors where all elements are required, to hopefully prompt people to use a different type if that's not the behavior that they want.
class Vec3f(BaseModel, Generic[NumberType]): | |
"""A 3D Vector.""" | |
x: NumberType | |
y: NumberType | |
z: NumberType | |
class Vec3f(BaseModel, Generic[NumberType]): | |
"""A 3D vector where all elements are required.""" | |
x: NumberType | |
y: NumberType | |
z: NumberType |
assert loaded_model.channels == types.PipetteChannelType.NINETY_SIX_CHANNEL | ||
|
||
model_dict = loaded_model.model_dump() | ||
# each field should be the value of the enum class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm caught up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the other comment—for performance reasons, can we change the .construct()
s to .model_construct()
s instead of changing them to fully-validating __init__()
calls?
robot-server/setup.py
Outdated
"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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need to be pinned to a specific version like they were before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't pin to exact versions unless strictly necessary. See e.g. #11905
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ==
.
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
Merging for updates to robot-server tests.
An update on where this stands at the time of writing:
|
Per pydantic/pydantic#6748 (comment) now may be the time to evaluate the latest pydantic2 release |
sadly this includes getting rid of some nice typing that won't work until we have future peps
This carries a lot of speedups relative to 2.6; maybe it's even usable now.
57305be
to
8082363
Compare
Probably needed for pydantic updates
03a63b4
to
5a781f1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14871 +/- ##
===========================================
- Coverage 92.43% 60.23% -32.21%
===========================================
Files 77 191 +114
Lines 1283 10667 +9384
===========================================
+ Hits 1186 6425 +5239
- Misses 97 4242 +4145
Flags with carried forward coverage won't be shown. Click here to find out more.
|
please please please prioritize this. Pydantic should have fixed many import perf issues. It's a major pain that we can't easily interoperate between core models in parts of our codebase. |
Overview
Let's try this again
Closes PLAT-326.
Test Plan
Changelog
Update our robot software (and everything that depends on it) from Pydantic v1 to Pydantic v2:
buildroot changes in [todo]
Review requests
Risk assessment