-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Support both pydantic 1.x and 2.x #37055
Conversation
Signed-off-by: Alan Guo <aguo@anyscale.com>
d3212fa
to
71b30ba
Compare
Signed-off-by: Alan Guo <aguo@anyscale.com>
Is this required for ray serve? Btw, maybe we need a guard against ray serve (or at least warning)? cc @edoakes @sihanwang41 |
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.
nit comments
except ImportError: | ||
# pydantic is not available in the dashboard. | ||
# We will use the dataclass from the standard library. | ||
from dataclasses import dataclass | ||
|
||
JobDetails = object | ||
is_pydantic_2 = False |
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.
why don't we just call it pydantic_1_available
? This seems to be more clear with code below
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.
that naming doesn't quite work, because when pydantic is not installed at all, also want init=True
. Setting the variable pydantic_1_available = True
is weird.
) | ||
|
||
if ModelField is not None: |
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.
Q: Can't we just check if pydantic is 1 or 2 here instead?
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.
not sure why that's better. I thought it's better to check existance of features/variables over doing a version check.
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.
just the logic seems pretty complicated (1 vs 2 is just one line?) I am okay with this approach too.
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.
in this case, do you think it is possible to add a simple unit test?
General question: does this mean that ray serve will be broken until fastapi uses pydantic v2? |
Btw, this is a release blocker right? Are we tracking the issue @alanwguo ? |
We pinned pydantic to <2 so this is not a release blocker. But I am still targetting to get this merged for 2.6 |
yes, but only if user is using pydanatic 2.0 |
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 perfer to have an unit test for serialization_addon.py logic (it seems pretty complicated to me). maybe you can do it using runtime _env (runtime_env with pydantic 1 vs 2). Other than that it lgtm
) | ||
|
||
if ModelField is not None: |
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.
just the logic seems pretty complicated (1 vs 2 is just one line?) I am okay with this approach too.
) | ||
|
||
if ModelField is not None: |
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.
in this case, do you think it is possible to add a simple unit test?
Signed-off-by: Alan Guo <aguo@anyscale.com>
Added a unit test! Verified the test fails if i comment out my |
Lint failure + can you remove the pin from the PR? |
I don't know if we should remove the pin so quickly. There are still issues with Serve and we can't pickle pydantic models with v 2.x. @pcmoritz thoughts? |
Signed-off-by: Alan Guo <aguo@anyscale.com>
Lmk when this is ready to be merged! |
I think it's ready... it's hard to tell with all these test failures, but I looked through them and they look unrelated Different tests seem to fail between each run. |
@alanwguo before we merge, is it possible to run CI with pydantic version > 2.0? And then let's merge this by today? |
@alanwguo looks like fastapi support pydantic |
@edoakes , thanks for bringing that to my attention! I don't think I'm the right person to get serve working with pydantic 2.0, |
01e5a94
to
cacc796
Compare
cacc796
to
414d04d
Compare
The failing tests seem new to me, so I am rerunning it just in case. Highly likely it is not releated |
Looks ready to merge? |
These changes fixes a couple of things when ray and pydantic 2.0 is installed: ray can start up now ray jobs works now gets rid of some deprecation warnings when using old pydantic field names. Some things are still broken: ray serve doesn't work because it depends on fastapi and fastapi does not support pydantic 2.0 yet: Prepare for Pydantic V2 release fastapi/fastapi#6051 Pickling pydantic 2 models is not possible, although users can still pickle pydantic 1.0 models via the compatibility pydantic.v1 import.
These changes fixes a couple of things when ray and pydantic 2.0 is installed: ray can start up now ray jobs works now gets rid of some deprecation warnings when using old pydantic field names. Some things are still broken: ray serve doesn't work because it depends on fastapi and fastapi does not support pydantic 2.0 yet: Prepare for Pydantic V2 release fastapi/fastapi#6051 Pickling pydantic 2 models is not possible, although users can still pickle pydantic 1.0 models via the compatibility pydantic.v1 import.
These changes fixes a couple of things when ray and pydantic 2.0 is installed: ray can start up now ray jobs works now gets rid of some deprecation warnings when using old pydantic field names. Some things are still broken: ray serve doesn't work because it depends on fastapi and fastapi does not support pydantic 2.0 yet: Prepare for Pydantic V2 release fastapi/fastapi#6051 Pickling pydantic 2 models is not possible, although users can still pickle pydantic 1.0 models via the compatibility pydantic.v1 import. Signed-off-by: NripeshN <nn2012@hw.ac.uk>
These changes fixes a couple of things when ray and pydantic 2.0 is installed: ray can start up now ray jobs works now gets rid of some deprecation warnings when using old pydantic field names. Some things are still broken: ray serve doesn't work because it depends on fastapi and fastapi does not support pydantic 2.0 yet: Prepare for Pydantic V2 release fastapi/fastapi#6051 Pickling pydantic 2 models is not possible, although users can still pickle pydantic 1.0 models via the compatibility pydantic.v1 import. Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
Sorry for the ping, any plan to support pydantic 2.0 on ray serve? |
Found the answer, targeting for release 2.8, sorry for the bothering. |
These changes fixes a couple of things when ray and pydantic 2.0 is installed: ray can start up now ray jobs works now gets rid of some deprecation warnings when using old pydantic field names. Some things are still broken: ray serve doesn't work because it depends on fastapi and fastapi does not support pydantic 2.0 yet: Prepare for Pydantic V2 release fastapi/fastapi#6051 Pickling pydantic 2 models is not possible, although users can still pickle pydantic 1.0 models via the compatibility pydantic.v1 import. Signed-off-by: Victor <vctr.y.m@example.com>
Why are these changes needed?
These changes fixes a couple of things when ray and pydantic 2.0 is installed:
Some things are still broken:
pydantic.v1
import.Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.