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

Add support for unpacked TypedDict to type hint variadic keyword arguments in ArgumentsValidator #1451

Merged
merged 6 commits into from
Sep 20, 2024

Conversation

Viicos
Copy link
Member

@Viicos Viicos commented Sep 13, 2024

Change Summary

Part of pydantic/pydantic#9619.

Related issue number

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

src/validators/var_kwargs.rs Outdated Show resolved Hide resolved
Copy link

codspeed-hq bot commented Sep 16, 2024

CodSpeed Performance Report

Merging #1451 will not alter performance

Comparing kwargs-td (57467bd) with main (8c1a0da)

Summary

✅ 155 untouched benchmarks

None => None,
};

if var_kwargs_mode == VarKwargsMode::UnpackedTypedDict && var_kwargs_validator.is_none() {
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 wondering if we should also check that var_kwargs_validator is a TypedDictValidator?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to, given that you have the conditional checks in pydantic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Making it a TypedDictValdiator statically (at build time) would allow for efficiency gains (by avoiding the dispatch via CombinedValidator).

But I wonder, are there cases where it can be wrapped in a function-after validator (e.g. model_validator)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Making it a TypedDictValdiator statically (at build time) would allow for efficiency gains (by avoiding the dispatch via CombinedValidator).

The thing is (as described here) var_kwargs_validator is used for both uniform and unpacked-typed-dict modes.

But I wonder, are there cases where it can be wrapped in a function-after validator (e.g. model_validator)?

only config can be attached to typed dicts iirc, and it still results in a typed dict schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could have

#[derive(Debug, PartialEq)]
enum VarKwargsMode {
    Uniform(CombinedValidator),
    UnpackedTypedDict(TypedDictValidator),
}

... and replace the FromStr implementation with a bespoke function like from_string_and_validator or similar.

Copy link
Member

Choose a reason for hiding this comment

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

Will leave this as an optimization for later, going to approve for now 👍

Copy link
Member

Choose a reason for hiding this comment

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

See #1457

src/validators/arguments.rs Outdated Show resolved Hide resolved
@Viicos Viicos changed the title Add support for TypedDict for **kwargs Add support for unpacked TypedDict to type hint variadic keyword arguments with @validate_call Sep 17, 2024
@Viicos Viicos changed the title Add support for unpacked TypedDict to type hint variadic keyword arguments with @validate_call Add support for unpacked TypedDict to type hint variadic keyword arguments in the ArgumentsValidator Sep 17, 2024
@Viicos Viicos changed the title Add support for unpacked TypedDict to type hint variadic keyword arguments in the ArgumentsValidator Add support for unpacked TypedDict to type hint variadic keyword arguments in ArgumentsValidator Sep 17, 2024
@Viicos Viicos marked this pull request as ready for review September 17, 2024 13:06
Copy link
Member

@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.

Looking great thus far! A couple of rust syntax suggestions + potentially one constant change.

python/pydantic_core/core_schema.py Show resolved Hide resolved
Comment on lines 3422 to 3424
var_kwargs_mode: The validation mode to use for variadic keyword arguments. If `'single'`, every value of the
keyword arguments will be validated against the `var_kwargs_schema` schema. If `'unpacked-typed-dict'`,
the `schema` argument must be a [`typed_dict_schema`][pydantic_core.core_schema.typed_dict_schema]
Copy link
Member

Choose a reason for hiding this comment

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

What if instead of single we did uniform? Don't want to bikeshed here too much, but I was a bit confused by single on its own.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I'm not too happy with name either. uniform seems better, although there might be an even more explicit name, I'll think about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

uniform seems good to me 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about whether to handle uniform via the "extras" of the typed dict schema (if such a thing exists) and have simpler code here (i.e. no need for mode, just always require typed dict schema). Maybe a little breaking, though?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a extras_schema key in the TypedDictSchema, but it is not used currently. For Pydantic models, we support:

class Model(BaseModel):
    __pydantic_extra__: dict[str, int]

But we don't for typed dicts. I believe it's best to wait for https://peps.python.org/pep-0728/, this will probably completely change how we handle extra keys for typed dicts currently.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. We should probably start validating extra keys as well, just a side note :).

None => None,
};

if var_kwargs_mode == VarKwargsMode::UnpackedTypedDict && var_kwargs_validator.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to, given that you have the conditional checks in pydantic.

src/validators/arguments.rs Outdated Show resolved Hide resolved
src/validators/arguments.rs Outdated Show resolved Hide resolved
src/validators/arguments.rs Show resolved Hide resolved
src/validators/arguments.rs Show resolved Hide resolved
Co-authored-by: Sydney Runkle <[email protected]>
Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Overall LGTM with some brief thoughts...

Comment on lines 3422 to 3424
var_kwargs_mode: The validation mode to use for variadic keyword arguments. If `'single'`, every value of the
keyword arguments will be validated against the `var_kwargs_schema` schema. If `'unpacked-typed-dict'`,
the `schema` argument must be a [`typed_dict_schema`][pydantic_core.core_schema.typed_dict_schema]
Copy link
Contributor

Choose a reason for hiding this comment

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

uniform seems good to me 👍

src/validators/arguments.rs Show resolved Hide resolved
None => None,
};

if var_kwargs_mode == VarKwargsMode::UnpackedTypedDict && var_kwargs_validator.is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Making it a TypedDictValdiator statically (at build time) would allow for efficiency gains (by avoiding the dispatch via CombinedValidator).

But I wonder, are there cases where it can be wrapped in a function-after validator (e.g. model_validator)?

src/validators/arguments.rs Outdated Show resolved Hide resolved
src/validators/arguments.rs Outdated Show resolved Hide resolved
Comment on lines 3422 to 3424
var_kwargs_mode: The validation mode to use for variadic keyword arguments. If `'single'`, every value of the
keyword arguments will be validated against the `var_kwargs_schema` schema. If `'unpacked-typed-dict'`,
the `schema` argument must be a [`typed_dict_schema`][pydantic_core.core_schema.typed_dict_schema]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about whether to handle uniform via the "extras" of the typed dict schema (if such a thing exists) and have simpler code here (i.e. no need for mode, just always require typed dict schema). Maybe a little breaking, though?

Copy link
Member

@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.

Great work @Viicos, looking forward to having this feature!

python/pydantic_core/core_schema.py Outdated Show resolved Hide resolved
None => None,
};

if var_kwargs_mode == VarKwargsMode::UnpackedTypedDict && var_kwargs_validator.is_none() {
Copy link
Member

Choose a reason for hiding this comment

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

Will leave this as an optimization for later, going to approve for now 👍

@sydney-runkle sydney-runkle enabled auto-merge (squash) September 20, 2024 16:30
@sydney-runkle sydney-runkle merged commit f3f436e into main Sep 20, 2024
28 of 29 checks passed
@sydney-runkle sydney-runkle deleted the kwargs-td branch September 20, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants