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 the pydantic from v1 to V2 #3942

Closed
wants to merge 15 commits into from

Conversation

shuchu
Copy link
Collaborator

@shuchu shuchu commented Feb 8, 2024

What this PR does / why we need it:
Based on the requirements from the Feast users, we want to upgrade the Pydantic version to v2 (2.6) for the performance of Feast.

Here are a few important package version changes:
pydantic >= 2.0.0
great_expectations>=0.15.41 (the upper bound is removed. Its required by the pydantic v2.)

Since it changes many files in the codebase, I humbly ask our reviewers to review this PR carefully.

Which issue(s) this PR fixes:
Fixes #3778

@sudohainguyen
Copy link
Collaborator

Seems a lot to do at a time, can we leave some of them for another PR such as great expectations?

Let's target pydantic only first

@shuchu
Copy link
Collaborator Author

shuchu commented Feb 8, 2024

The upgradation of "fastapi" and "great-expectation" are required by Pydantic v2. There was a discussion over here: #3778

Also the "mypy", if I don't upgrade it, I can not past the lint check.

It is a little bit struggling as you can see. :)

@shuchu
Copy link
Collaborator Author

shuchu commented Feb 8, 2024

by the way, I can downgrade the fastAPI and great-expectation versions by using the "from pydantic.v1 import" migration strategy. If you think this is better.

@shuchu
Copy link
Collaborator Author

shuchu commented Feb 8, 2024

I just found some issues of fastAPI. let me try to change the version back (for fastAPI and greate-expectation). The unit test didn't show me any errors. Only the vscode gives me some hints of errors.

@sudohainguyen
Copy link
Collaborator

sudohainguyen commented Feb 8, 2024

Great, we can get rid of GX later cause it needs some extra work for the API changes.

Regarding fastapi we should expect to be resolved in this issue #3938

Copy link

@picnic-sven picnic-sven left a comment

Choose a reason for hiding this comment

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

Exciting PR 🚀 !

# via feast (setup.py)
pydantic==1.10.13
pydantic==2.6.1

Choose a reason for hiding this comment

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

Is there a specific reason for requiring 2.6.1? Otherwise, would you consider using something like 2.0.0 to allow for better compatibility for downstream users of this package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't specify a version in the setup.py, so it loads the latest one :p. let me find a "safe" version around 2.0.0. @sudohainguyen Harry, what's your opinion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the txt files are just for CI workflows 😄
installation from end users should be fine since we set the boundaries in setup.py

Choose a reason for hiding this comment

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

Apologies, the excitement got the better of me. Thanks for the clear up 💪

@tokoko
Copy link
Collaborator

tokoko commented Feb 8, 2024

@shuchu Can't we upgrade mypy first without touching anything else?

@sudohainguyen
Copy link
Collaborator

@tokoko 😆 just had a thought about it too, worth a try I believe

@shuchu shuchu closed this Feb 8, 2024
@shuchu shuchu reopened this Feb 8, 2024
@shuchu
Copy link
Collaborator Author

shuchu commented Feb 8, 2024

@shuchu Can't we upgrade mypy first without touching anything else?

Okay, let me try. Do you have a specific mypy version requirement? I'm using ~=1.1.1 in this PR.

@tokoko
Copy link
Collaborator

tokoko commented Feb 8, 2024

Okay, let me try. Do you have a specific mypy version requirement? I'm using ~=1.1.1 in this PR.

No, just suggesting a way to split the PR up into smaller PRs.

@sudohainguyen
Copy link
Collaborator

hey @shuchu in #3938 the author has done the mypy upgrade part
mind having a look? once the PR merged, this PR can move forward with ease

@shuchu
Copy link
Collaborator Author

shuchu commented Feb 9, 2024

Seems a great relief to me :) Thank you for letting me know, Harry! @sudohainguyen

@sudohainguyen
Copy link
Collaborator

Seems a great relief to me :) Thank you for letting me know, Harry! @sudohainguyen

mypy upgrade is done, would you mind pushing this one?

@shuchu
Copy link
Collaborator Author

shuchu commented Feb 11, 2024

Seems a great relief to me :) Thank you for letting me know, Harry! @sudohainguyen

mypy upgrade is done, would you mind pushing this one?

Sure, let me merge and adjust the great-expectation version. :)

@sudohainguyen sudohainguyen added the dependencies Pull requests that update a dependency file label Feb 11, 2024
@shuchu
Copy link
Collaborator Author

shuchu commented Feb 11, 2024

it's ready for reviews. :) @sudohainguyen

@sudohainguyen
Copy link
Collaborator

looks conflicts were resolved incorrectly, think we should re-do on a new PR 😢

@shuchu
Copy link
Collaborator Author

shuchu commented Feb 12, 2024

looks conflicts were resolved incorrectly, think we should re-do on a new PR 😢

I see the problems. let me redo it.

@shuchu
Copy link
Collaborator Author

shuchu commented Feb 12, 2024

@sudohainguyen check the new PR. :) #3948

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pydantic 2.0 support
7 participants