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 validate_core_schema function and remove validation from SchemaValidator and SchemaSerializer constructors #982

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

davidhewitt
Copy link
Contributor

@davidhewitt davidhewitt commented Sep 21, 2023

Change Summary

Related to startup performance. The idea is that pydantic will have more control over when the CoreSchema gets validated, e.g. validating it once for both SchemaValidator and SchemaSerializer, deciding not to validate it at all based on an env var, etc.

Selected Reviewer: @samuelcolvin

@davidhewitt
Copy link
Contributor Author

please review

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #982 (d235d90) into main (4c84ed8) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #982   +/-   ##
=======================================
  Coverage   93.09%   93.09%           
=======================================
  Files         106      106           
  Lines       15830    15832    +2     
  Branches       26       26           
=======================================
+ Hits        14737    14739    +2     
  Misses       1086     1086           
  Partials        7        7           
Files Changed Coverage Δ
python/pydantic_core/__init__.py 92.59% <ø> (ø)
src/validators/string.rs 98.29% <ø> (ø)
src/lib.rs 100.00% <100.00%> (ø)
src/serializers/mod.rs 98.49% <100.00%> (-0.02%) ⬇️
src/validators/mod.rs 95.23% <100.00%> (+0.03%) ⬆️

... and 1 file 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 4c84ed8...d235d90. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 21, 2023

CodSpeed Performance Report

Merging #982 will degrade performances by 20.54%

Comparing dh/build-both (d235d90) with main (33a7cc0)

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 135 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main dh/build-both Change
test_decimal_from_string_core 55.4 µs 69.7 µs -20.54%
test_core_model_py 67.1 µs 58.2 µs +15.41%
test_model_exclude_unset_false 73.4 µs 63.7 µs +15.3%

@adriangb
Copy link
Member

I'd prefer a method that validates the self-schema, and then remove that from the SchemaValidator and SchemaSerialzier constructors.

@adriangb adriangb changed the title add private API to build SchemaValidator and SchemaSerializer together Add validate_core_schema function and remove validation from SchemaValidator and SchemaSerializer constructors Sep 21, 2023
@adriangb
Copy link
Member

Made the change proposed above. It's also now clear where in our tests we're passing in schemas that need to be coerced, I'd suggest we minimize that and eventually remove it in followup PRs. Ideally a CoreSchema doesn't need to be validated at all.

@davidhewitt
Copy link
Contributor Author

LGTM, I am author so cannot approve. Feel free to approve & merge.

@adriangb adriangb enabled auto-merge (squash) September 21, 2023 14:49
Comment on lines +843 to +844
This currently uses lax mode for validation (i.e. will coerce strings to dates and such)
but may use strict mode in the future.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be nice to add a strict flag to make this easier later ;)

@adriangb adriangb merged commit 916d909 into main Sep 21, 2023
28 of 30 checks passed
@adriangb adriangb deleted the dh/build-both branch September 21, 2023 14:50
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.

None yet

3 participants