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

Refactoring Redis OM Python to use pydantic 2.0 types and validators #603

Merged
merged 11 commits into from May 2, 2024

Conversation

slorello89
Copy link
Member

With #533 it became possible to use Redis OM Python in an environment where pydantic 2.0 was installed, however this leaned completely on the pydantic v1 shim, consequentially this meant that if you tried to use any of the pydantic 2.0 types or validators produce errors-a-plenty, see: #602, #601, #590, #572, #571, #546, and many more

Pydantic V1 -> Pydantic V2 was described as a "ground up rewrite" by the pydantic team, and it's apparent that it was, however they have also stated that while they will be shipping major versions every year going forward that future migrations should not be nearly as painful

the tact projects like FastAPI took was to use the shim in the inverse (import v1 types from their old places if your pydantic version is V1, use v2 types if pydantic version is v2), which this PR adapts us towards.

If you only use pydantic 1.x, there should be no breaking changes in here, the shim should include the correct fields for you and the code should be backwards compatible, however if you are using pydantic 2.0 in your app, there will be breaks going from redis om python 0.2.x -> 0.3.x. namely. In short, anything that comes up in the pydantic 2.0 migration guide is liable to be an issue for someone migrating. Some highlights:

  • The way the validators work in pydantic has changed, previously, in pydantic 1.0, if a field was declared as Optional, pydantic would infer that it should not produce a validation error if the field was not set, in pydantic 2.0, optionals must be explicitly set on the model, or must be provided a default value.
  • There will also be type-changes in the types of error's that are thrown during validation, namely moving from the pydantic 1.x ValidationError to the pydantic 2.x ValidationError.
  • Only Validators from 2.x will work

Would like some eyes from our end (namely @banker, @bsbodden, @tylerhutcherson). As well as feedback from anyone in the community who might have interest.

Copy link

@bsbodden bsbodden left a comment

Choose a reason for hiding this comment

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

This LGTM, I will build a basic app using Pydantic 2.0 and provide some feedback

This was linked to issues Apr 5, 2024
Copy link

@tylerhutcherson tylerhutcherson left a comment

Choose a reason for hiding this comment

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

Tons of work here @slorello89 ! Kudos for grinding through. When will we drop pydantic <= 1.x.x support? LangChain has also had to fight this battle.

from pydantic.version import VERSION as PYDANTIC_VERSION
from typing_extensions import Annotated, Literal, get_args, get_origin


PYDANTIC_V2 = PYDANTIC_VERSION.startswith("2.")

Choose a reason for hiding this comment

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

If it's helpful, I have seen some shim implementations that just use try except.. but this may not necessarily be better. Just offering some other ways to approach this:

https://github.com/langchain-ai/langchain/blob/master/libs/core/langchain_core/pydantic_v1/dataclasses.py

Copy link
Member Author

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 or less the way that FastAPI does it. a try/except might be a bit more inclusive. . . hmm thoughts @banker or @bsbodden ?

Copy link

Choose a reason for hiding this comment

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

The try/except approach makes me feel icky. Are we thinking of dropping Pydantic 1.0 support completely? It's seems a lot of project are doing that nowadays.

@slorello89
Copy link
Member Author

@tylerhutcherson - I think a reasonable cutoff is whenever pydantic removes the v1 shim to discontinue support for v1. I think that might be coming sooner rather than later(I think they are stopping bug fixes after June).

@epicwhale
Copy link

epicwhale commented Apr 21, 2024

Thanks for looking into this! This would be great to have soon, given pydantic recently released 2.7.0 few days ago :) would make this library a lot more useful in projects where I can't downgrade pydantic.

Copy link

@rbs333 rbs333 left a comment

Choose a reason for hiding this comment

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

Looked through made some take or leave style comments. Seems like there might be a couple dangling comments/lines

aredis_om/model/model.py Outdated Show resolved Hide resolved
@@ -1156,7 +1203,7 @@ def Field(
vector_options=vector_options,
**current_schema_extra,
)
field_info._validate()
# field_info._validate()
Copy link

Choose a reason for hiding this comment

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

Should this fire? Or are you doing validate elsewhere? Otherwise, no need for comment probably.

aredis_om/model/model.py Outdated Show resolved Hide resolved
aredis_om/model/model.py Outdated Show resolved Hide resolved
aredis_om/model/model.py Outdated Show resolved Hide resolved
Copy link

@rbs333 rbs333 left a comment

Choose a reason for hiding this comment

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

Seems like there might be a couple dangling comments. Other than that LGTM

bsbodden
bsbodden previously approved these changes May 1, 2024
Copy link

@bsbodden bsbodden left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Copy link

@bsbodden bsbodden left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakingchange enhancement New feature or request
Projects
None yet
5 participants