-
Notifications
You must be signed in to change notification settings - Fork 68
Vb/support pydantic2 plt 145 #1412
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
Conversation
84e4c3b to
04de631
Compare
04de631 to
779f692
Compare
444d092 to
f592269
Compare
labelbox/pydantic_compat.py
Outdated
|
|
||
| # Check if the version is 1 | ||
| if pydantic_version.startswith("1"): | ||
| pydantic_v1_module_name = "pydantic" if sub_module_path is None else f"pydantic.{sub_module_path}" |
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 think this is more legible and DRY if we don't have nested boolean statements here, e.g.
If sub_module_path:
pydantic_v1_module_name = f"pydantic.{submodulepath}"
else if pydantic_version.startswith("1"):
pydantic_v1_module_name = ...
else:
...
labelbox/pydantic_compat.py
Outdated
| if pydantic_version.startswith("1"): | ||
| pydantic_v1_module_name = "pydantic" if sub_module_path is None else f"pydantic.{sub_module_path}" | ||
| else: # use pydantic 2 v1 thunk | ||
| pydantic_v1_module_name = "pydantic.v1" if sub_module_path is None else f"pydantic.{sub_module_path}" |
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 would assume we want pydantic.v1 in both cases. If so, why not?
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.
Well what if users are still using pydantic v1? Currently we need to support this case so we will do from pydantic instead of from pydantic.v1
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 thought that case is captured by the if condition?
I'm referring to the f"pydantic.{sub_module_path}" in the else condition which assumes the usage of Pydantic 2.x.
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.
Good point
mrobers1982
left a comment
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.
Good start!
Story: https://labelbox.atlassian.net/browse/PLT-145
Please review for background and implementation approach: https://labelbox.atlassian.net/wiki/spaces/~63d753c8724a5c79c7b69a36/pages/2369257507/SDK+Pydantic2+Pydantic1+Compatibility
This PR introduces several crucial updates aimed at enhancing our sdk compatibility with Pydantic2. The changes are as follows: