NotificationsYou must be signed in to change notification settings
Fork2.2k
Star24.6k

Do not reuse validators and serializers during model rebuild#11429

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Viicos merged 1 commit intomainfrommodel-rebuild-val-ser

Feb 12, 2025

Merged

Do not reuse validators and serializers during model rebuild#11429

Viicos merged 1 commit intomainfrommodel-rebuild-val-ser

Feb 12, 2025

Conversation

Copy link

Member

Viicos commentedFeb 11, 2025

As we look forcls.__pydantic_validator/serializer__ in pydantic-core, we don't want to mistakenly reuse them.

Change Summary

Related issue number

Checklist

The pull request title is a good summary of the changes - it will be used in the changelog
Unit tests for the changes exist
Tests pass on CI
Documentation reflects the changes where applicable
My PR is ready to review,please add a comment including the phrase "please review" to assign reviewers

github-actionsbot added the relnotes-fixUsed for bugfixes. label

Feb 11, 2025

Copy link

cloudflare-workers-and-pagesbot commentedFeb 11, 2025•
edited
Loading

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`059a706`
Status:	✅ Deploy successful!
Preview URL:	https://414fc1ce.pydantic-docs.pages.dev
Branch Preview URL:	https://model-rebuild-val-ser.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hqbot commentedFeb 11, 2025•
edited
Loading

CodSpeed Performance Report

Merging#11429 willdegrade performances by 7.16%

_{Comparingmodel-rebuild-val-ser (059a706) withmain (a1f12c7)}

Summary

❌ 9 regressions
✅ 37 untouched benchmarks

⚠️Please fix the performance issues oracknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`test_complex_model_schema_generation`	1.4 ms	1.5 ms	-6.02%
❌	`test_recursive_model_schema_generation`	795.3 µs	845 µs	-5.87%
❌	`test_simple_model_schema_generation`	576.6 µs	614.2 µs	-6.13%
❌	`test_tagged_union_with_callable_discriminator_schema_generation`	995 µs	1,056.5 µs	-5.83%
❌	`test_tagged_union_with_str_discriminator_schema_generation`	995.4 µs	1,072.2 µs	-7.16%
❌	`test_deeply_nested_recursive_model_schema_generation`	1 ms	1.1 ms	-5.52%
❌	`test_generic_recursive_model_schema_generation`	699.7 µs	742.2 µs	-5.72%
❌	`test_recursive_discriminated_union_with_base_model`	1.3 ms	1.4 ms	-6.75%
❌	`test_simple_recursive_model_schema_generation`	580.3 µs	620.9 µs	-6.53%

Viicos force-pushed themodel-rebuild-val-ser branch from0e62bfd to638708fCompare

February 11, 2025 16:38

Copy link

Contributor

github-actionsbot commentedFeb 11, 2025

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
dataclasses.py
main.py
Project Total

_{This report was generated bypython-coverage-comment-action}

sydney-runkle reviewed

Feb 12, 2025

View reviewed changes

Copy link

Contributor

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.

I approve of these logical change, but I'm equally perplexed by the benchmark changes.

I think we should investigate before merging, but obviously this should make its way into v2.11.

pydantic/dataclasses.py OutdatedShow resolvedHide resolved

pydantic/main.py OutdatedShow resolvedHide resolved

Do not reuse validators and serializers during model rebuild

059a706

As we look for `cls.__pydantic_validator/serializer__` inpydantic-core, we don't want to mistakenly reuse them.

Viicos force-pushed themodel-rebuild-val-ser branch from638708f to059a706Compare

February 12, 2025 10:01

sydney-runkle mentioned this pull request

Feb 12, 2025

Collect model fields when rebuilding a model#11388

Merged

5 tasks

Copy link

MemberAuthor

Viicos commentedFeb 12, 2025

So I'm still not sure why the performance difference comes from pydantic-core:

But I now realize it was just improved by the reusable validators/serializers in pydantic-core (see#11402), which doesn't make sense for benchmarks such astest_simple_model_schema_generation, which doesn't use any nested models:

pydantic/tests/benchmarks/test_model_schema_generation.py

Lines 36 to 42 ina1f12c7

	deftest_simple_model_schema_generation(benchmark)->None:
	classSimpleModel(DeferredModel):
	field1:str
	field2:int
	field3:float

	benchmark(rebuild_model,SimpleModel)

Copy link

Contributor

sydney-runkle commentedFeb 12, 2025

Yeah, this regression is a bit mysterious. Though, it seems like for cases with nested models, we still see all of the perf wins that we wanted to from previous improvements.

sydney-runkle approved these changes

Feb 12, 2025

View reviewed changes

Copy link

Contributor

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.

This fix is necessary, and I believe we've diagnosed the codspeed changes as not blocking here.@Viicos if you can acknowledge them in codspeed, happy to merge this.