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

introduce "bound validators" for int, str, float #675

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Contributor

This attempts to optimize validators for collections (currently just list and dict in this draft, could be extended to all of them) by making an extra "binding" step before validating a collection. The bound validator has the final strictness determined form both schema and extra.

The idea is that this should help avoid branches for laxness / strictness on each step of the collection validation. Seems to offer a modest speedup.

This design may fall apart if there's more ways to set strictness than by the schema and the extra (e.g. at runtime, maybe some validator chooses its strictness based on a function call).

Pushed this as a draft now because it would be interesting to gather feedback before pushing a load more additional complexity.

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 20, 2023

CodSpeed Performance Report

Merging #675 will improve performances by 55.52%

Comparing dh/bound-validators (8e60f2a) with main (5c696e5)

Summary

🔥 11 improvements
✅ 115 untouched benchmarks

Benchmarks breakdown

Benchmark main dh/bound-validators Change
🔥 test_complete_core_lax 1,177.2 µs 944.9 µs +24.59%
🔥 test_complete_core_strict 1,178.5 µs 948.5 µs +24.25%
🔥 test_complete_core_json 2.8 ms 2.5 ms +12.52%
🔥 test_core_python_fs 288.9 µs 185.8 µs +55.52%
🔥 test_list_of_ints_core_py 2.2 ms 1.7 ms +28.3%
🔥 test_list_of_ints_core_json 5.8 ms 5 ms +14.8%
🔥 test_set_of_ints_core_json_duplicates 4.7 ms 4.2 ms +12.04%
🔥 test_dict_of_ints_core 4.7 ms 4.1 ms +14.14%
🔥 test_dict_of_any_core 3.3 ms 2.9 ms +14.53%
🔥 test_definition_in_tree 4.5 ms 3.1 ms +45.82%
🔥 test_core_root_model 97.8 µs 75.6 µs +29.29%

@dmontagu
Copy link
Collaborator

@davidhewitt I think the extra is probably the only source of setting the strictness; the only way I'm aware of doing runtime manipulation of strictness from pydantic is the strict argument to model_validate (and similar for TypeAdapter). At any rate I think it's a reasonable restriction that you can't change strict except in those two places, unless you are willing to use a python-implemented field/model validator, at which point I think this is a non-issue.

I wouldn't say I'm confident in the above though, maybe a question for @samuelcolvin or @adriangb

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Overall this looks great, feel free to merge or add more usage here if you like.

@samuelcolvin
Copy link
Member

yes, your guesses about strict are right I think.

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #675 (c3dce30) into main (5c696e5) will increase coverage by 0.10%.
The diff coverage is 94.87%.

❗ Current head c3dce30 differs from pull request most recent head 8e60f2a. Consider uploading reports for the commit 8e60f2a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #675      +/-   ##
==========================================
+ Coverage   93.63%   93.73%   +0.10%     
==========================================
  Files          99       99              
  Lines       13895    13877      -18     
  Branches       25       25              
==========================================
- Hits        13010    13008       -2     
+ Misses        879      863      -16     
  Partials        6        6              
Impacted Files Coverage Δ
src/validators/float.rs 90.85% <93.33%> (+0.41%) ⬆️
src/validators/int.rs 98.57% <93.33%> (-1.43%) ⬇️
src/validators/string.rs 94.44% <93.33%> (-0.26%) ⬇️
src/input/return_enums.rs 85.71% <100.00%> (+0.95%) ⬆️
src/validators/dict.rs 86.60% <100.00%> (ø)
src/validators/mod.rs 98.10% <100.00%> (+0.33%) ⬆️

... and 29 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c696e5...8e60f2a. Read the comment docs.

@davidhewitt
Copy link
Contributor Author

I would love to come back to this idea one day but I think first will focus on more bugfixes / the PyO3 changes and see where performance sits after that.

@davidhewitt davidhewitt closed this Nov 6, 2023
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

Successfully merging this pull request may close these issues.

3 participants