-
Notifications
You must be signed in to change notification settings - Fork 62
Upgrade to use Pydantic 2.x.x #101
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
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
+ Coverage 76.95% 77.16% +0.21%
==========================================
Files 22 22
Lines 1380 1384 +4
==========================================
+ Hits 1062 1068 +6
+ Misses 318 316 -2 ☔ View full report in Codecov by Sentry. |
Spartee
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.
Undoubtly will cause some pain with the langchain integration. However, I think as long as there is no inheritance chain that utilizes pydantic v2 with langchain internals we should be fine. the langchain integration should just use langchain.pydantic_v1 for any instantiated classes. As well, any base classes we expect to use in that integration should also utilize langchain.pydantic_v1 if at all possible.
Also, as this is a library we expect to have people use as a dependency within their projects, I would expect to have a wider range of support for pydantic versions even if we have to test multiple versions (we shouldnt, but you never know).
nit: might think about breaking up some of these non-related changes just in case you need to roll this back if it causes major issues at some point down the road.
Up to you, otherwise looks good
CONTRIBUTING.md
Outdated
|
|
||
| Formatting and linting checks: | ||
| ```bash | ||
| make format |
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.
make check?
requirements.txt
Outdated
| coloredlogs | ||
| pydantic<2.0.0 | ||
| redis>=5.0.0 | ||
| pydantic==2.5.3 |
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.
may think about relaxing this a bit. Pinning to a patch version for this popular of a library is bound to cause some issues. more on this in the top level 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.
Will do. Good call
The base branch was changed.
9abcb36 to
183cb68
Compare
With the shift in the ecosystem to Pydantic 2.x.x, we need to get
redisvlto the same level.Referenced here: https://github.com/RedisVentures/redisvl/issues/86
Fortunately, there is not a ton of complex usage of Pydantic; which makes this easy! Changes include:
pydantic>=2.0.0,<3v1shim for now for safety