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

Unexpected behavior with pydantic.functional_validators.model_validator (union of Protocols for Callables) #6875

Closed
ITProKyle opened this issue Jan 2, 2024 · 8 comments
Labels
as designed Not a bug, working as intended bug Something isn't working

Comments

@ITProKyle
Copy link

ITProKyle commented Jan 2, 2024

Looking through the recent release, there were a few changes regarding the handling of protocols, callables, and overloads which would have impacted this.
Not sure if this is intentional behavior but it does not seem like it as I have not been able to find a combo that works.

Describe the bug

See code example below which includes the error messages in the docstrings.
Included is only a few things that I tried to satisfy pyright after my original signature (_original_before_model_validator), which passed for <1.1.344, started to fail type checking.
One attempt (_second_attempt) which i included fails on both (1.1.343 & 1.1.344), which is fine, just showing what i tried based on the error message provided.

Links to relevant pydantic source code:

Code or Screenshots

"""Example of pyright error."""
from typing import Any

from pydantic import BaseModel, model_validator  # tried 2.5.2 & 2.5.3 (two latest versions) - latest version included more protocols in the union


class ExampleModel(BaseModel):
    """Example model."""

    some_field: str

    @model_validator(mode="before")
    def _original_before_model_validator(
        cls, values: dict[str, object]
    ) -> dict[str, object]:
        """My original signature.

        Type "(cls: Self@ExampleModel, values: dict[str, object]) -> dict[str, object]" cannot be assigned to type "_AnyModeBeforeValidator"
            Type "(cls: Self@ExampleModel, values: dict[str, object]) -> dict[str, object]" cannot be assigned to type "(cls: Any, __value: Any, __info: ValidationInfo) -> Any"
                Function accepts too many positional parameters; expected 2 but received 3
                    Parameter name mismatch: "__value" versus "values"
            Type "(cls: Self@ExampleModel, values: dict[str, object]) -> dict[str, object]" cannot be assigned to type "(cls: Any, __value: Any) -> Any"
                Parameter name mismatch: "__value" versus "values" (reportGeneralTypeIssues)

        Same/similar error when explicitly including `@classmethod`.

        """
        return values

    @model_validator(mode="before")
    def _second_attempt(cls, __value: Any) -> Any:
        """Explicitly match the parameter name and type from one protocol.

        This is the only one that errors on 1.1.343 & 1.1.344.

        Type "(cls: Self@ExampleModel, __value: Any, /) -> Any" cannot be assigned to type "_AnyModeBeforeValidator"
            Type "(cls: Self@ExampleModel, __value: Any, /) -> Any" cannot be assigned to type "(cls: Any, __value: Any, __info: ValidationInfo) -> Any"
            Function accepts too many positional parameters; expected 2 but received 3
                Position-only parameter mismatch; parameter "cls" is not position-only
                Position-only parameter mismatch; parameter "__value" is not position-only
                Position-only parameter mismatch; expected 2 but received 0
            Type "(cls: Self@ExampleModel, __value: Any, /) -> Any" cannot be assigned to type "(cls: Any, __value: Any) -> Any"
                Position-only parameter mismatch; parameter "cls" is not position-only (reportGeneralTypeIssues)

        Same/similar error when explicitly including `@classmethod`.

        """
        return __value

    @model_validator(mode="before")
    def _third_attempt(cls, v: Any, info: Any) -> Any:
        """Mimic the sig of the first protocol but with different param names.

        Type "(cls: Self@ExampleModel, v: Any, info: Any) -> Any" cannot be assigned to type "_AnyModeBeforeValidator"
            Type "(cls: Self@ExampleModel, v: Any, info: Any) -> Any" cannot be assigned to type "(cls: Any, __value: Any, __info: ValidationInfo) -> Any"
                Parameter name mismatch: "__value" versus "v"
                Parameter name mismatch: "__info" versus "info"
            Type "(cls: Self@ExampleModel, v: Any, info: Any) -> Any" cannot be assigned to type "(cls: Any, __value: Any) -> Any"
                Parameter name mismatch: "__value" versus "v" (reportGeneralTypeIssues)

        Same/similar error when explicitly including `@classmethod`.

        """
        return v

VS Code extension or command-line

CLI - 1.1.344

@erictraut
Copy link
Collaborator

This is related to the way pydantic defines some of its callback protocols. In particular, look at ModelBeforeValidatorWithoutInfo in functional_validators.py. It is define as follows:

class ModelBeforeValidatorWithoutInfo(Protocol):
    def __call__(self, cls: Any, __value: Any) -> Any:
        ...

I think the intent of this signature is to specify that the __value parameter is supposed to be position-only (since it's named with a double underscore), but it doesn't make sense for a position-only parameter to follow a non-position-only parameter. In this case cls, can be specified by keyword. There are therefore two ways this signature could be legitimately interpreted:

  1. cls should be considered a position-only parameter because it is followed by a position-only parameter.
  2. __value should not be considered a position-only parameter because it follows a non-position-only parameter.

Pyright is using interpretation 2, which I think is well justified. However, the typing spec is ambiguous on this point, so it could be interpreted either way. I'll post a question to the typing forums to solicit opinions from the broader community about how this should be interpreted.

The best solution here is for the pydantic code to be unambiguous in its intent. If the intent is for cls to be position-only, then the protocol should be defined as:

class ModelBeforeValidatorWithoutInfo(Protocol):
    def __call__(self, __cls: Any, __value: Any) -> Any:
        ...

or

class ModelBeforeValidatorWithoutInfo(Protocol):
    def __call__(self, cls: Any, __value: Any, /) -> Any:
        ...

Both of these eliminate the error you're seeing.

I'll keep this issue open until we have some feedback from the typing community on how to interpret the spec.

@erictraut
Copy link
Collaborator

For reference, here's the thread in the typing forum: https://discuss.python.org/t/ambiguities-about-positional-only-parameters/42328.

@ITProKyle
Copy link
Author

Interesting. I've been using __<name> for ages and never knew its true purpose was for backward-compatible positional-only arguments. The way I have always understood and used it is that it allows for the use of any name for the argument which would be a side effect of becoming a positional-only argument. Digging through PEPs (and mypy's docs which I used early on in learning python typing) i don't see anywhere where I would have gotten that understanding years ago other than that just being the behavior of type checking. Thanks again for sharing your expertise in this area.

I wonder how many other places this may pop up since it just worked in the past. Depending on the amount of effort it would take, might be worthwhile having a setting for this due to the ambiguity? Something along the lines of reportImplicitPositionalArgument that would act as Ambiguity #1 from your discussion topic when disabled and act as it does now (strict) when enabled?

@sherbang
Copy link

sherbang commented Jan 3, 2024

So, the only answer to getting this to pass is for the definition to be changed upstream, or to ignore all reportGeneralTypeIssues on this line?

If there's no syntax that I can use to make my function match what pydantic expected, then it would be helpful to separate this from the reportGeneralTypeIssues rule. As a consumer of the library, I can't make this change happen upstream, I can only suggest.

More generally, it would be really nice to have more granularity in ignores. With so much packed into reportGeneralTypeIssues it's too easy to unintentionally mask additional errors on a line when ignoring one that can't be fixed.

@adriangb
Copy link

adriangb commented Jan 9, 2024

I'll try to chime in on intent as the original author of the code, although I wrote it a bit ago so may not recall of the details.

The intent was to force users to have the first parameter be called cls because these methods must be class methods. For backward compatibility reasons, and because it's very verbose to decorate every method with @classmethod and as far as I know type checkers still special case @classmethod and there is no way to say "this decorator converts an unbound method to a class method", we chose to make the @model_validator(mode='before') decorator (which via overloads maps to that protocol) require cls as the first argument to keep users from using self and treating a class as an instance.

I think it'd be fine to change it on the pydantic side.

@erictraut
Copy link
Collaborator

@adriangb, thanks for the additional context. There's a discussion about how a type checker should treat this case (where a parameter whose name starts with a double underscore appears after a parameter whose name does not). We seem to be converging on the consensus that it should be considered an error by a type checker and the resulting behavior should be considered undefined or indeterminate. If that's the final conclusion to the discussion, you'll need to make the change on the pydantic side.

@adriangb
Copy link

adriangb commented Jan 9, 2024

Makes sense. My only feedback would be that it’d be nice if libraries could express that a decorator acts as if it were @classmethod which would’ve removed the need for Pydantic to do something questionable in the first place.

@erictraut
Copy link
Collaborator

The thread in the typing forum has resulted in a clarification to the typing spec that indicates type checkers should consider the following code an error:

class ModelBeforeValidatorWithoutInfo(Protocol):
    def __call__(self, cls: Any, __value: Any) -> Any:
        ...

The pydantic library will need to be updated accordingly.

I've filed a new issue #7290 to track the addition of this new error.

@erictraut erictraut added as designed Not a bug, working as intended and removed needs decision labels Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants