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

Pydantic validators raise a pydantic.Validation error instead of raising a tyro.Instantiation error #70

Open
sander76 opened this issue Sep 12, 2023 · 4 comments

Comments

@sander76
Copy link
Contributor

sander76 commented Sep 12, 2023

Pydantic has strong property validation capabilities that go beyond the capabilities of type Annotations.
Consider the following pydantic model which has a custom validator included:

class Model(BaseModel):
    a: int

    @field_validator("a")
    def below_ten(cls, v):
        if v > 10:
            raise ValueError("Must be below 10")

Tyro doesn't take them into account during model validation which is -I think- taking place somewhere here:

value = arg.lowered.instantiator(value)

Pydantic does the above validation at model instantiation, which is happening here:

return unwrapped_f(*positional_args, **kwargs), consumed_keywords # type: ignore

It would be great to leverage the validation rules inside tyro.

@sander76 sander76 changed the title Pydantic validators raise an error instead of raising tyro.Instantiation error Pydantic validators raise a pydantic.Validation error instead of raising a tyro.Instantiation error Sep 12, 2023
@brentyi
Copy link
Owner

brentyi commented Sep 12, 2023

Thanks for the suggestion @sander76!

This is mostly an aesthetics/user-friendliness thing, right? We get an error message either way, we just want it to be formatted as a tyro error instead of a pydantic one?

The easiest solution here would be to drop a try/except pair around the model instantiation line (return unwrapped_f(...)). We can catch pydantic.ValidationError, and then form a cleaner error message.

If there's a way to pull out the individual validator for a field we could also do it on the output of value = arg.lowered.instantiator(value), but this is more involved and I'm not sure it'd be worth it.

@sander76
Copy link
Contributor Author

Yes it is. I like the way tyro does the output. And I see the pydantic validators as a powerful way to validate the arguments.

@brentyi
Copy link
Owner

brentyi commented Sep 14, 2023

Okay, I like it! This seems like an easy way to add a lot of argument validation flexibility to tyro, which is nice. :)

No worries if not, but if you have time to take a stab at a (even partial) PR I'd appreciate it, otherwise I can experiment with these errors as soon as I catch up on some other responsibilities.

@sander76
Copy link
Contributor Author

I will come up with something! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants