Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.2k
Support parametrization of class-scoped type variables when using@validate_call
on Pydantic models methods#10367
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
01c4dc4
to95f0d2c
Comparecodspeed-hqbot commentedSep 10, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#10367 willnot alter performanceComparing Summary
|
9d16504
todd014f4
Comparedd014f4
to352b7a7
CompareI've enabled auto merge on#10100 :) |
Could you by chance separate this fix out into a separate PR? Makes things easier to review and easier to revert, if necessary. |
cc@dmontagu, perhaps you'd want to take a first pass review on this one? |
Certainly, check#10380. |
87d62f4
to23f7aa6
Compare23f7aa6
tobb683a1
Comparegithub-actionsbot commentedSep 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Coverage reportClick to see where and how coverage changed
This report was generated bypython-coverage-comment-action |
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 seems like a very smart implementation, and I think it's a good piece of functionality to get working, but I'd really prefer if there was any way we could achieve it without usingexec
and class generation codegen; something just seems very fragile and/or security-risky about that.
kc0506 commentedSep 12, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I have completely removed the hacky
@dmontagu btw, what do you think about this workaround? From the current design, it seems we want to return a normal function instead of a wrapper class by |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
validate_call
with generic function@validate_call
on Pydantic models methodsCo-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
function.__qualname__ = qualname.replace(original_postfix, f'{model.__name__}.{name}') | ||
def update_generic_validate_calls(model: type[BaseModel]) -> None: |
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.
You will have to add atry..finally
block in case any exception happens, as otherwise the original function will have its attributes mutated.
Also, I'll have to think about it more, but are we certain that every callable that can be used withvalidate_call
can have the__qualname__
/__annotations__
etc arguments mutated?
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.
Ah, I think I forgot to consider callables with various type in many places. Later I should add more tests withpartial
,wraps
etc.
Also,#10461 should probably be dealt with first to better recognize what types of functions we may have here.
We are going to close this for now, as you mentioned it is difficult to properly support it and there isn't enough upvotes on the issue to be considered worthwhile. Thanks again for taking a stab at this, we can revisit if we ever want to implement it. |
Uh oh!
There was an error while loading.Please reload this page.
Change Summary
The primary change of this PR is to allow code as below to work properly:
Limitations
I classify generic functions into three cases:
1. The TypeVar is "free", or attached to a function:
2. The TypeVar is attached to a class that is not a subclass of
BaseModel
:3. The TypeVar is attached to a class thatis a subclass of
BaseModel
:For the case 1 and 2, there is (AFAIK) unfortunately no way to access the value of
T
at runtime without custom__class_getitem__
. See theissue anddiscussion. Therefore there is no way to "trigger" the schema rebuilding process.For case 3, the value of
T
is always determined and can be accessed at runtime. Hence, this PR currently only supports such case.Update: I realize that in case 1, we can override the
__getitem__
such that the user can do something likef[int](...)
to manually trigger rebuilding. The only problem is thatFunctionType
is not allowed to be subclassed, and instance-level method override does not work for dunder methods, so we must return an object whose type is notFunctionType
anymore. This seems to conflict with current design.As for case 2, perhaps we could expose another API, e.g.
enable_generic_validate_call
, so that when a class is decorated by it, we override its__class_getitem__
and do the similar stuff asBaseModel
.Related issue number
#7796 (partially fixed)
Checklist