-
Notifications
You must be signed in to change notification settings - Fork 233
feat: hybrid pydantic support for both v1 and v2 #1652
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
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
Signed-off-by: samsja <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
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.
Okay first pass done, I have few questions and will probably make a more thorough pass tomorrow!
from pydantic.validators import bytes_validator | ||
|
||
else: | ||
from pydantic.v1.validators import bytes_validator |
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.
Am I understanding correctly that we are falling back to the bytes validator of v1 even if pydantic v2 is used? What is the reason behind this?
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.
?
if is_pydantic_v2: | ||
raise NotImplementedError( | ||
'This method is not supported in Pydantic 2.0. Please use Pydantic 1.8.2 or lower.' | ||
) | ||
|
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 is this?
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.
the method we used has been removed from pydantic v2, will have to find a fix later
@@ -36,6 +45,13 @@ | |||
|
|||
from docarray.array.doc_vec.column_storage import ColumnStorageView | |||
|
|||
if is_pydantic_v2: | |||
|
|||
IncEx: typing_extensions.TypeAlias = ( |
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.
What does IncEx
stand for?
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.
this is the named used in pydantic function signature. Not even sure what this means
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.
okay
json_encoders = {AbstractTensor: lambda x: x} | ||
if is_pydantic_v2: | ||
|
||
class Config: |
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.
Should we still have this Config inner class, it is not recommended anymore in v2, right?
Also, where did the orjson stuff and the extra protobuf setting go?
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 question,
basically now in pydantic v2 this way is "deprecated". But at the same time, it is still working. Therefore if we keep it as a class it means we don't have to repeat the config for pydantic v2. Might be easier to maintain ...
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.
Also, where did the orjson stuff and the extra protobuf setting go?
This is handle differently with pydantic v2 and therefore not needed
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.
hmm, using deprecated stuff is not great. Does it raise a warning?
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.
it does indeed raise a warning ...
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
Co-authored-by: Johannes Messner <[email protected]> Signed-off-by: samsja <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
1 similar comment
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
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.
the only thing that feels icky to me is using the deprecated Config
inner class
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
1 similar comment
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
I will go with @JohannesMessner proposal and switch to ConfigDict instead of class |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
12a1dc6
to
d7a7a49
Compare
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
📝 Docs are deployed on https://ft-feat-full-pydantic-v2-support--jina-docs.netlify.app 🎉 |
Context
this PR allow support for pydantic v2 and v1 at the same time
Note: it is still very early progress