Fix typing of model_validator #6514
Conversation
Deploying with
|
| Latest commit: |
96de51b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d695b048.pydantic-docs2.pages.dev |
| Branch Preview URL: | https://fix-typing.pydantic-docs2.pages.dev |
|
@Kludex want to try and apply the same thing to the other protocols? We might be able to simplify quite a bit |
|
please review |
| ModelAfterValidatorWithoutInfo = Callable[[_ModelType], _ModelType] | ||
| """A `@model_validator` decorated function signature. This is used when `mode='after'` and the function does not | ||
| have info argument. | ||
| """ |
There was a problem hiding this comment.
If I understand correctly, mypy didn't recognize the the previously annotated Protocol object as callable? (Even though it had call method? By switching to explicitly annotating with Callable, the annotations are correct?
There was a problem hiding this comment.
I like that we're annotating callable functions as callable though. (Though Protocol is supposed to solve this for us, no?)
There was a problem hiding this comment.
In theory you can use either, but it's a little bit weird because here we want to require the self argument, in particular because it helps distinguish from the other alternative signatures. But my hack to make that work does not seem to have been compatible with mypy so this is a simpler albeit less pedantic version that hopefully will be.
There was a problem hiding this comment.
here we want to require the
selfargument, in particular because it helps distinguish from the other alternative signatures
I don't know if I'm understanding your thought correctly.
But if those validators work with other signatures as well (e.g. with classmethod signatures) it would be misleading imo. The implementation as far as I saw supports static-, class- and instance-signatures and even decorators. Therefore, I don't think you have to enforce an instance-signature here (i.e. with self argument).
Actually, even the documentation suggests a classmethod-signature for model_validator with mode="after". Does the solution of this PR really work with classmethod-signatures (and maybe with a @classmethod decorator) though, did you test it? If not, is anything wrong with doing it similar to field_validator? It seems to work there.
There was a problem hiding this comment.
Actually, even the documentation suggests a classmethod-signature for model_validator with mode="after"
This is precisely what we wanted to avoid, the docs are wrong.
There was a problem hiding this comment.
Ok, I tried around by myself but I really couldn't get a Protocol typing for instance methods to work together with the decorator. At least with mypy. Very sad, maybe it's possible in the future.
Btw, why do you even want to avoid classmethod signatures? Is there anywhere a discussion about this?
There was a problem hiding this comment.
Btw, why do you even want to avoid classmethod signatures? Is there anywhere a discussion about this?
You're getting an instance of the model. Of course (cls: Type['Model'], v: 'Model') and add an @classmethod works but why do that instead of (self) which works great with type checkers, etc.
There was a problem hiding this comment.
Just ran into this issue today and found the issue and this PR and thought why not telling my thoughts on it :)
I really like the idea of Protocol. But annotating callables with it with an explicit self-argument doesn't really seem to work with mypy. However, pyright seems to get it to work. I found this little discussion (more like an issue) regarding this topic if anyone is interested:
python/typing#1040
The PR looks good, but I had a question as well.
Thanks for the good work, appreciate it :)
| ModelAfterValidatorWithoutInfo = Callable[[_ModelType], _ModelType] | ||
| """A `@model_validator` decorated function signature. This is used when `mode='after'` and the function does not | ||
| have info argument. | ||
| """ |
There was a problem hiding this comment.
here we want to require the
selfargument, in particular because it helps distinguish from the other alternative signatures
I don't know if I'm understanding your thought correctly.
But if those validators work with other signatures as well (e.g. with classmethod signatures) it would be misleading imo. The implementation as far as I saw supports static-, class- and instance-signatures and even decorators. Therefore, I don't think you have to enforce an instance-signature here (i.e. with self argument).
Actually, even the documentation suggests a classmethod-signature for model_validator with mode="after". Does the solution of this PR really work with classmethod-signatures (and maybe with a @classmethod decorator) though, did you test it? If not, is anything wrong with doing it similar to field_validator? It seems to work there.
|
|
||
| _AnyModelWrapValidator = Union[ModelWrapValidator, ModelWrapValidatorWithoutInfo] | ||
| _AnyModeBeforeValidator = Union[ModelBeforeValidator, ModelBeforeValidatorWithoutInfo] | ||
| _AnyModeAfterValidator = Union[ModelAfterValidator, ModelAfterValidatorWithoutInfo] | ||
| _AnyModelAfterValidator = Union[ModelAfterValidator[_ModelType], ModelAfterValidatorWithoutInfo[_ModelType]] |
There was a problem hiding this comment.
Does this reparametrization do anything? It looks like the same typevar is used in the original definition of this thing. Is this for pseudo-documentation reasons or does it type check differently? Should it make use of a TypeAlias?
There was a problem hiding this comment.
It's because generic aliases require the type var reparametrization
|
Hi @adriangb, Since this commit is already merged and addresses the typing issue, I don't want to silence it in our codebase. |
|
I believe that either today, or tomorrow (likely). |
Fixes #6277. Closes python/mypy#15620
skip change file check
Selected Reviewer: @dmontagu