-
Notifications
You must be signed in to change notification settings - Fork 18k
core: Cleanup Pydantic models and handle deprecation warnings #30799
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
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
7529366
to
5fcb2bb
Compare
14905a4
to
6b9bb2c
Compare
@@ -183,6 +122,9 @@ def pre_init(func: Callable) -> Any: | |||
with warnings.catch_warnings(): | |||
warnings.filterwarnings(action="ignore", category=PydanticDeprecationWarning) | |||
|
|||
# Ideally we would use @model_validator(mode="before") but this would change the | |||
# order of the validators. See https://github.com/pydantic/pydantic/discussions/7434. | |||
# So we keep root_validator for backward compatibility. |
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.
One possibility would be to deprecate pre_init
and create a new decorator, for instance before_init
, that uses model_validator
.
WDYT ?
6b9bb2c
to
d4b2366
Compare
I would love to drop v1 support (would clean up the codebase a lot). Need to chat with the team about this depending on download #s, etc. |
Note: LangChain already dropped support for Pydantic v1. There remains some models using |
d4b2366
to
3c2f2ba
Compare
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.
Looks pretty good! Main thing is that let's avoid renaming out top level dict()
into model_dump()
. It's our own interface, I don't want to make a breaking change here, unless there's a compelling reason (i.e., bug)
@@ -1307,7 +1307,7 @@ def _llm_type(self) -> str: | |||
"""Return type of chat model.""" | |||
|
|||
@override | |||
def dict(self, **kwargs: Any) -> dict: | |||
def model_dump(self, **kwargs: Any) -> dict: |
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.
dict
is a langchain defined rather than pydantic defined method in this case.
Users likely have custom serialization code that will break if they to use dict
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.
Are you sure ? When I look at the dict()
inheritance, I find pydantic.BaseModel.dict()
.
So to be correct when we replace usage of the deprecated BaseChatModel.dict()
by BaseChatModel.model_dump()
, we have to do the override in model_dump
rather than in dict
.
In Pydantic v2, dict
is deprecated and just calls model_dump
which is the replacement.
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 don't think the goal is to mirror the Pydantic API here. I'm OK with this shadowing and overriding the dict in Pydantic.
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'm not sure to understand: do you want me to rollback the change here ?
BTW, I don't know if I was clear enough but overriding model_dump
isn't a breaking change for users that are calling dict
as dict
is just an alias for model_dump
. It would be breaking only for users that were calling model_dump
.
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's going to happen once pydantic 3 is released? dict()
gets removed from Pydantic. I don't think it means that we have to force langchain users to use model_dump()
, instead i'd prefer that we have users keep on using dict()
. I don't want users to assume that pydantic's underlying API is usable for arbitrary langchain abstractions
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 see. But dict
is an unfortunate name for a method as it conflicts with the builtin dict
(this is probably why Pydantic renamed it). And it also conflicts with the Pydantic dict
giving us a warning. It would be nice to rename it in LangChain too with a LangChain deprecation cycle (we could use asdict
which looks familiar from dataclasses
or model_dump
which would tie to Pydantic again for good or bad). WDYT ?
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's going to happen once pydantic 3 is released?
About that we could override both dict
and model_dump
. So even if it disappears from Pydantic, we still have it in LangChain.
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 changed to use asdict
. LMK if you prefer to use model_dump
(with dict
still overridden and LangChain's deprecated).
@@ -331,7 +331,7 @@ def _prompt_type(self) -> str: | |||
"""Return the prompt type key.""" | |||
raise NotImplementedError | |||
|
|||
def dict(self, **kwargs: Any) -> dict: | |||
def model_dump(self, **kwargs: Any) -> dict: |
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 should stay as dict
for now
21a75f8
to
47382b2
Compare
CodSpeed WallTime Performance ReportMerging #30799 will degrade performances by 56.46%Comparing
|
Benchmark | BASE |
HEAD |
Change | |
---|---|---|---|---|
❌ | test_import_time[InMemoryRateLimiter] |
70 ms | 160.7 ms | -56.46% |
@override | ||
def dict(self, **kwargs: Any) -> dict: | ||
return self.asdict() | ||
|
||
def asdict(self) -> typing.Dict[str, Any]: # noqa: UP006 |
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.
Once dict
is removed, we can use the builtin dict
instead of typing.Dict
and remove the noqa
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.
Could we keep dict()
as is for now and not introduce asdict()
? I'd like to avoid any interface changes as part of this PR.
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.
OK, done. I'll open a separate PR for the dict
conflict issue.
@override | ||
def dict(self, **kwargs: Any) -> dict: | ||
return self.asdict() | ||
|
||
def asdict(self) -> typing.Dict[str, Any]: # noqa: UP006 |
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.
LMK if you prefer an independent asdict
or a tied to Pydantic model_dump
.
CodSpeed Instrumentation Performance ReportMerging #30799 will not alter performanceComparing Summary
|
18f3c7b
to
91b407a
Compare
@eyurtsev I rolled-back the changes on the |
Uh oh!
There was an error while loading. Please reload this page.