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

feat: Upgrade pydantic to v2 #13983

Closed
adrijanik opened this issue Nov 15, 2023 · 7 comments
Closed

feat: Upgrade pydantic to v2 #13983

adrijanik opened this issue Nov 15, 2023 · 7 comments
Labels
feature-request A request for a new feature or a change that isn't a bug. May require further triage or scoping.

Comments

@adrijanik
Copy link

adrijanik commented Nov 15, 2023

Overview

Hi team,

Do you have any plans to upgrade pydantic to v2 from v1.8.2? Is there maybe any branch that I could use if such transition is happening? It becomes a problem when you want to use opentrons library in a project where packages dependencies requires pydantic v2. I couldn't find any issue that address that.

Thanks!

Implementation details

I haven't tested that, but this could be a simple fix by using v1 apis from within v2 version, see below:
from pydantic.v1 import BaseModel

There is also a helper package bump-pydantic which may help with that. Here is also a migration guide.

Design

No response

Acceptance criteria

Lift the dependency from pydantic 1.8.2 to pydantic >=2.

@adrijanik adrijanik added the feature-request A request for a new feature or a change that isn't a bug. May require further triage or scoping. label Nov 15, 2023
@SyntaxColoring
Copy link
Contributor

Yeah, we definitely want to upgrade Pydantic. (Internal reference: RSS-227)

Unfortunately we haven't started that work yet, so there's no branch you could use. We don't have a timeline for it, since we're focusing on the new Opentrons Flex.

There is also a helper package bump-pydantic which may help with that. Here is also a migration guide.

This is helpful. Thank you!

@ChrisTwig
Copy link

Hello @SyntaxColoring is there any proposed plan for this?
You could even use the new namespace pydantic.v1

@SyntaxColoring
Copy link
Contributor

Sorry for the lack of updates here. Yes, we're working on this in PR #14871. It's not our first priority, though, so I can't promise it will be completed soon.

@silvioscience
Copy link

Hello @SyntaxColoring, any updates on this?

@SyntaxColoring
Copy link
Contributor

Latest update: #14871 (comment)

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

sfoster1 added a commit that referenced this issue Dec 18, 2024
# 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]>
@SyntaxColoring
Copy link
Contributor

Done in PR #14871.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A request for a new feature or a change that isn't a bug. May require further triage or scoping.
Projects
None yet
Development

No branches or pull requests

5 participants