Skip to content

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

Merged
merged 113 commits into from
Sep 10, 2023

Conversation

samsja
Copy link
Member

@samsja samsja commented Jun 15, 2023

Context

this PR allow support for pydantic v2 and v1 at the same time

Note: it is still very early progress

@github-actions github-actions bot added size/m and removed size/s labels Jun 15, 2023
@samsja samsja changed the title feat: init commit on adding v2 support feat: hybrid pydantic support for both v1 and v2 Jun 15, 2023
Signed-off-by: samsja <[email protected]>
@github-actions github-actions bot added size/xl and removed size/l labels Jun 22, 2023
@github-actions
Copy link

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.

Copy link
Member

@JohannesMessner JohannesMessner left a 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
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

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

?

Comment on lines +53 to +57
if is_pydantic_v2:
raise NotImplementedError(
'This method is not supported in Pydantic 2.0. Please use Pydantic 1.8.2 or lower.'
)

Copy link
Member

Choose a reason for hiding this comment

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

Why is this?

Copy link
Member Author

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 = (
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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:
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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.

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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.

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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.

samsja and others added 2 commits September 7, 2023 13:30
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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.

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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
@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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.

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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.

Copy link
Member

@JohannesMessner JohannesMessner left a 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

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

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
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

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.

@samsja
Copy link
Member Author

samsja commented Sep 8, 2023

I will go with @JohannesMessner proposal and switch to ConfigDict instead of class

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

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.

@samsja samsja force-pushed the feat-full-pydantic-v2-support branch from 12a1dc6 to d7a7a49 Compare September 8, 2023 08:38
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

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.

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

📝 Docs are deployed on https://ft-feat-full-pydantic-v2-support--jina-docs.netlify.app 🎉

@samsja samsja merged commit 715252a into main Sep 10, 2023
@samsja samsja deleted the feat-full-pydantic-v2-support branch September 10, 2023 21:06
@JoanFM JoanFM mentioned this pull request Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants