Skip to content
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

Draft
wants to merge 75 commits into
base: edge
Choose a base branch
from

Conversation

ahiuchingau
Copy link
Contributor

@ahiuchingau ahiuchingau commented Apr 11, 2024

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:

  • api
  • robot-server
  • shared-data
  • g-code-testing
  • hardware
  • server-utils
  • system-server
  • hardware-testing (re-lock only, depends on api)
  • abr-testing (re-lock only, depends on api)
  • performance-metrics (re-lock only, depends on shared-data)

buildroot changes in [todo]

Review requests

Risk assessment

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a 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:

Comment on lines +12 to +16
DatetimeType = typing.Annotated[
datetime,
PlainSerializer(lambda x: x.isoformat(), when_used="json"),
]

Copy link
Contributor

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?

Comment on lines +15 to +18
DatetimeType = typing.Annotated[
datetime,
PlainSerializer(lambda x: x.isoformat(), when_used="json"),
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

api/src/opentrons/protocol_engine/clients/sync_client.py Outdated Show resolved Hide resolved
# 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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Typo: Adatper instead of Adapter
  2. 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(
Copy link
Contributor

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?

Comment on lines +19 to +24
class Vec3f(BaseModel, Generic[NumberType]):
"""A 3D Vector."""

x: NumberType
y: NumberType
z: NumberType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The name Vec3f comes from "vector of 3 floats." If this is generic across other kinds of NumberType, let's rename it to just Vec3.
  2. 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.
Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a 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!

Copy link
Contributor

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/robot_server/log.txt Outdated Show resolved Hide resolved
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",
Copy link
Contributor

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?

Copy link

@ecederstrand ecederstrand May 24, 2024

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

Copy link
Contributor

@SyntaxColoring SyntaxColoring May 24, 2024

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

ahiuchingau and others added 2 commits April 19, 2024 13:09
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
@SyntaxColoring SyntaxColoring deleted the chore_update-pydantic-v2 branch May 28, 2024 17:14
@SyntaxColoring
Copy link
Contributor

An update on where this stands at the time of writing:

  • The big problem to solve is that there's a pretty severe hit to robot-server startup time. On Flex, it goes from ~46s to ~1m54s, and it's probably worse on OT-2.
    • The Pydantic people are aware of the problem, but it doesn't look like there will be an imminent fix.
    • defer_build might help, but some quick experiments didn't show it making a huge difference. I wouldn't expect it to shave more than 20 seconds, and that's being optimistic.
    • There is a large FastAPI component to the problem, having to do with the way it nests routers within each other. We might be able to work around it by flattening our routers. I don't know exactly what that would look like yet. A proof of concept would be a good next step.
  • There are still various tests that are failing for one reason or another. So far, I haven't seen anything that fundamentally concerns me. Just a lot of little things to work through.
  • edge is a moving target and there are always going to be more merge conflicts. One thing that would help is ignoring the trivial rename DeprecationWarnings for now—maybe even remove the fixes that we have so far from this PR—and fix them in incremental follow-ups.

@sfoster1
Copy link
Member

sfoster1 commented Sep 3, 2024

Per pydantic/pydantic#6748 (comment) now may be the time to evaluate the latest pydantic2 release

Probably needed for pydantic updates
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.23%. Comparing base (094be6e) to head (cdbf235).
Report is 19 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@             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     
Flag Coverage Δ
hardware 55.82% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...are/opentrons_hardware/drivers/can_bus/settings.py 95.77% <100.00%> (ø)

... and 113 files with indirect coverage changes

@strangemonad
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants