Skip to content

Conversation

Viicos
Copy link
Member

@Viicos Viicos commented May 21, 2024

Fixes #9463

Selected Reviewer: @sydney-runkle

@Viicos Viicos marked this pull request as ready for review May 21, 2024 20:16
@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label May 21, 2024
Copy link

codspeed-hq bot commented May 21, 2024

CodSpeed Performance Report

Merging #9478 will not alter performance

Comparing Viicos:inspect-default-arg (9317086) with main (777cff0)

Summary

✅ 13 untouched benchmarks

@Viicos Viicos force-pushed the inspect-default-arg branch from 95d40b1 to 41696d9 Compare May 21, 2024 20:21
return sum(1 for param in sig.parameters.values() if can_be_positional(param))
"""Get the number of positional (required) arguments of a signature.

This function should only be used to inspect signatures of validation and serialization functions.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too happy with how this function grew, hence this "warning" in the docstring. At some point, maybe this inspection of validators/serializers signature could be refactored, e.g.:

@dataclass
class FunctionInfo:
    func: Callable[..., Any]

    @property
    def requires_info_arg(self) -> bool: ...

    ...

@Viicos Viicos force-pushed the inspect-default-arg branch from 8bec84c to 607c139 Compare May 21, 2024 21:05
@Viicos
Copy link
Member Author

Viicos commented May 21, 2024

Please review

@sydney-runkle
Copy link
Contributor

@Viicos,

Could you provide some tests with concrete examples of validators / serializers with default values, so that we have some more documented context?

Thanks!

@sydney-runkle
Copy link
Contributor

@Viicos,

Thanks for adding those extra tests. I suppose I still have a bit of a gap in understanding in why we'd want to support these additional args given that you don't call validators or serializers directly.

Could you explain a bit more re the motivation for this change / addition? Thanks!

@Viicos
Copy link
Member Author

Viicos commented May 22, 2024

Could you explain a bit more re the motivation for this change / addition? Thanks!

While it is really uncommon to use such a function as a validator (or serializer):

def test_validator_with_default_values() -> None:
def validate_x(v: int, unrelated_arg: int = 1, other_unrelated_arg: int = 2) -> int:
assert v != -1
return v
class Model(BaseModel):
x: int
val_x = field_validator('x')(validate_x)
with pytest.raises(ValidationError):
Model(x=-1)

It is more common to do:

class Model(BaseModel): 
    x: str

    val_x = field_validator('x', mode='before')(str) 

Or to use str as a plain serializer, etc.

Pydantic uses inspect.signature to check the validity of the validator/serializer callable: number of arguments, whether it takes an info argument or not. On CPython, inspect.signature does not work for some builtins, such as str. As it was done in https://github.com/pydantic/pydantic/pull/9450/files, we assume no info argument is needed in this case, so it works well. On PyPy however, the signature of str is successfully parsed as <Signature (object=<no value>, encoding=<no value>, errors=<no value>)>.

As we can see, str takes up to three arguments. To make it work with validator/serializer callables that do not take an info argument, we need to discard arguments having a default value (apart from the first one which will be the actual value being validated/serialized). After all, there's no reason to define a callable that needs to have the info arg this way:

def my_validator(value: Any, info: ValidationInfo | None = None): ...

If you need the info arg, you should specify it as a required argument.

@@ -774,7 +778,25 @@ def get_function_return_type(


def count_positional_params(sig: Signature) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the behaviour of this function changed to have the required arguments property, should we rename it to avoid accidents in case anyone is trying to borrow our internals?

@davidhewitt
Copy link
Contributor

Overall this seems very reasonable to me, with just a desire to rename the helper function to help avoid confusion.

@Viicos
Copy link
Member Author

Viicos commented May 31, 2024

Function renamed

@sydney-runkle
Copy link
Contributor

@Viicos,

Once we get tests passing, we should be all good to merge!

@sydney-runkle
Copy link
Contributor

@Viicos,

Maybe a pull from main would fix those tests?

Viicos added 4 commits June 6, 2024 15:14
Such functions now require the info argument to be required (i.e. no
default value specified in the signature).
Unify handling of exceptions when trying to get the function's signature.
Refactor tests
@Viicos Viicos force-pushed the inspect-default-arg branch from ce87dd5 to 109596d Compare June 6, 2024 13:14
@sydney-runkle
Copy link
Contributor

Ah with a closer look, looks like the test failure is related to these changes.

@Viicos
Copy link
Member Author

Viicos commented Jun 6, 2024

Yep, fixed the recently added test

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @Viicos!

@sydney-runkle sydney-runkle merged commit 28ab4f3 into pydantic:main Jun 6, 2024
@Viicos Viicos deleted the inspect-default-arg branch June 6, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support more callables in PlainSerializer
3 participants