only contains the relevant changes (the rest was split off in smaller PRs)
Implements the schema traversing logic in Python instead of inpydantic-core. Out of 3-4s, the rust implementation takes 20ms, while the Python one takes 230ms. I don't know yet if we should keep it in Python (it has advantages: type checking, easier debugging).
simplifies the schema traversing logic (simplified context class, etc)
simplifies schema inlining logic (no need to check for recursive refs)

Concerns (1 so far):

Consequence of the new schema traversing logic: we don't traverse schemas that have been unpacked from another model. With the following:

frompydanticimportBaseModelclassStandaloneModel(BaseModel): ...classM1(BaseModel):m:StandaloneModel

The core schema of M1 is:

M1 core schema

{│'type':'model',│'cls':<class'__main__.M1'>,│'schema': {│   │'type':'model-fields',│   │'fields': {│   │   │'m': {│   │   │   │'type':'model-field',│   │   │   │'schema': {│   │   │   │   │'type':'model',│   │   │   │   │'cls':<class'__main__.StandaloneModel'>,│   │   │   │   │'schema': {'type':'model-fields','fields': {},'model_name':'StandaloneModel','computed_fields': []},│   │   │   │   │'config': {'title':'StandaloneModel'},│   │   │   │   │'ref':'__main__.StandaloneModel:94970984129568',│   │   │   │   │'metadata': {'<stripped>'}│   │   │   │   },│   │   │   │'metadata': {}│   │   │   }│   │   },│   │'model_name':'M1',│   │'computed_fields': []│   },│'config': {'title':'M1'},│'ref':'__main__.M1:94970983986336',│'metadata': {'<stripped>'}}

As you can see,Model is inlined inside the core schema for fieldm.

Now if you do:

classM2(BaseModel):m1:M1m:StandaloneModel

StandaloneModel is used twice inM2 (inm andM1.m). However, the inlined core schema forM1.StandaloneModel is not transformed back into a'definition-ref' schema (and doing so would require a copy of the schema to avoid mutating the core schema ofM1 fromM2!):

M2 core schema

{│'type':'model',│'cls':<class'__main__.M2'>,│'schema': {│   │'type':'model-fields',│   │'fields': {│   │   │'m1': {│   │   │   │'type':'model-field',│   │   │   │'schema': {│   │   │   │   │'type':'model',│   │   │   │   │'cls':<class'__main__.M1'>,│   │   │   │   │'schema': {│   │   │   │   │   │'type':'model-fields',│   │   │   │   │   │'fields': {│   │   │   │   │   │   │'m': {│   │   │   │   │   │   │   │'type':'model-field',│   │   │   │   │   │   │   │'schema': {│   │   │   │   │   │   │   │   │'type':'model',│   │   │   │   │   │   │   │   │'cls':<class'__main__.StandaloneModel'>,│   │   │   │   │   │   │   │   │'schema': {'type':'model-fields','fields': {},'model_name':'StandaloneModel','computed_fields': []},│   │   │   │   │   │   │   │   │'config': {'title':'StandaloneModel'},│   │   │   │   │   │   │   │   │'ref':'__main__.StandaloneModel:94970985756064',│   │   │   │   │   │   │   │   │'metadata': {'<stripped>'}│   │   │   │   │   │   │   │   },│   │   │   │   │   │   │   │'metadata': {}│   │   │   │   │   │   │   }│   │   │   │   │   │   },│   │   │   │   │   │'model_name':'M1',│   │   │   │   │   │'computed_fields': []│   │   │   │   │   },│   │   │   │   │'config': {'title':'M1'},│   │   │   │   │'ref':'__main__.M1:94970985783632',│   │   │   │   │'metadata': {'<stripped>'}│   │   │   │   },│   │   │   │'metadata': {}│   │   │   },│   │   │'m': {│   │   │   │'type':'model-field',│   │   │   │'schema': {│   │   │   │   │'type':'model',│   │   │   │   │'cls':<class'__main__.StandaloneModel'>,│   │   │   │   │'schema': {'type':'model-fields','fields': {},'model_name':'StandaloneModel','computed_fields': []},│   │   │   │   │'config': {'title':'StandaloneModel'},│   │   │   │   │'ref':'__main__.StandaloneModel:94970985756064',│   │   │   │   │'metadata': {'<stripped>'}│   │   │   │   },│   │   │   │'metadata': {}│   │   │   }│   │   },│   │'model_name':'M2',│   │'computed_fields': []│   },│'config': {'title':'M2'},│'ref':'__main__.M2:94970985797664',│'metadata': {'<stripped>'}}

Onmain,StandaloneModel is properly stored in definitions, and'definition-ref' schemas are used:

M2 core schema on main

{│'type':'definitions',│'schema': {│   │'type':'model',│   │'cls':<class'__main__.M2'>,│   │'schema': {│   │   │'type':'model-fields',│   │   │'fields': {│   │   │   │'m1': {│   │   │   │   │'type':'model-field',│   │   │   │   │'schema': {│   │   │   │   │   │'type':'model',│   │   │   │   │   │'cls':<class'__main__.M1'>,│   │   │   │   │   │'schema': {│   │   │   │   │   │   │'type':'model-fields',│   │   │   │   │   │   │'fields': {│   │   │   │   │   │   │   │'m': {│   │   │   │   │   │   │   │   │'type':'model-field',│   │   │   │   │   │   │   │   │'schema': {'type':'definition-ref','schema_ref':'__main__.StandaloneModel:101823146688192'},│   │   │   │   │   │   │   │   │'metadata': {}│   │   │   │   │   │   │   │   }│   │   │   │   │   │   │   },│   │   │   │   │   │   │'model_name':'M1',│   │   │   │   │   │   │'computed_fields': []│   │   │   │   │   │   },│   │   │   │   │   │'config': {'title':'M1'},│   │   │   │   │   │'ref':'__main__.M1:101823149646624',│   │   │   │   │   │'metadata': {'<stripped>'}│   │   │   │   │   },│   │   │   │   │'metadata': {}│   │   │   │   },│   │   │   │'m': {│   │   │   │   │'type':'model-field',│   │   │   │   │'schema': {'type':'definition-ref','schema_ref':'__main__.StandaloneModel:101823146688192'},│   │   │   │   │'metadata': {}│   │   │   │   }│   │   │   },│   │   │'model_name':'M2',│   │   │'computed_fields': []│   │   },│   │'config': {'title':'M2'},│   │'ref':'__main__.M2:101823148327728',│   │'metadata': {'<stripped>'}│   },│'definitions': [│   │   {│   │   │'type':'model',│   │   │'cls':<class'__main__.StandaloneModel'>,│   │   │'schema': {'type':'model-fields','fields': {},'model_name':'StandaloneModel','computed_fields': []},│   │   │'config': {'title':'StandaloneModel'},│   │   │'ref':'__main__.StandaloneModel:101823146688192',│   │   │'metadata': {'<stripped>'}│   │   }│   ]}

Both approaches are valid, but it might be that this cause unexpected issues (JSON Schema differences, etc). Regarding memory consumption, it is slightly affected. Taking thek8s_v2.py file as an example (it is a good one because the "issue" described here happens several times in it), we don't see any increase/decrease in %RAM consumption, although profile over time differs:

PR	`main`

Refactor and optimize schema cleaning logic

30048f9

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

Jan 9, 2025

Copy link

cloudflare-workers-and-pagesbot commentedJan 9, 2025•
edited
Loading

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`76b7109`
Status:	✅ Deploy successful!
Preview URL:	https://2c6bd658.pydantic-docs.pages.dev
Branch Preview URL:	https://ref-schema-walking.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hqbot commentedJan 9, 2025•
edited
Loading

CodSpeed Performance Report

Merging#11244 willimprove performances by 33.86%

_{Comparingref-schema-walking (76b7109) withmain (4722283)}

Summary

⚡ 19 improvements
✅ 26 untouched benchmarks

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
⚡	`test_schema_build`	2.9 ms	2.5 ms	+15.75%
⚡	`test_fastapi_startup_perf`	200.5 ms	149.8 ms	+33.86%
⚡	`test_fastapi_startup_perf`	26.2 ms	21.1 ms	+24.27%
⚡	`test_complex_model_schema_generation`	1.8 ms	1.5 ms	+22.43%
⚡	`test_construct_dataclass_schema`	1.8 ms	1.4 ms	+32.26%
⚡	`test_lots_of_models_with_lots_of_fields`	2.7 s	2.3 s	+13.66%
⚡	`test_model_validators_serializers`	906.4 µs	758.1 µs	+19.56%
⚡	`test_nested_model_schema_generation`	1,136.4 µs	869.8 µs	+30.65%
⚡	`test_recursive_model_schema_generation`	1,016.4 µs	850.5 µs	+19.51%
⚡	`test_simple_model_schema_generation`	746.3 µs	618.7 µs	+20.63%
⚡	`test_simple_model_schema_lots_of_fields_generation`	28.5 ms	22.7 ms	+25.5%
⚡	`test_tagged_union_with_callable_discriminator_schema_generation`	1.5 ms	1.1 ms	+31.54%
⚡	`test_tagged_union_with_str_discriminator_schema_generation`	1.5 ms	1.2 ms	+32.62%
⚡	`test_deeply_nested_recursive_model_schema_generation`	1.3 ms	1.1 ms	+21.63%
⚡	`test_generic_recursive_model_schema_generation`	894.3 µs	740.1 µs	+20.83%
⚡	`test_nested_recursive_generic_model_schema_generation`	1.7 ms	1.4 ms	+21.57%
⚡	`test_nested_recursive_model_schema_generation`	1.8 ms	1.5 ms	+19.97%
⚡	`test_recursive_discriminated_union_with_base_model`	1.7 ms	1.4 ms	+18.88%
⚡	`test_simple_recursive_model_schema_generation`	774.5 µs	624.6 µs	+23.99%

Viicos added the third-party-testsAdd this label on a PR to trigger 3rd party tests label

Jan 9, 2025

Viicos closed this

Jan 9, 2025

Viicos reopened this

Jan 9, 2025

Viicos mentioned this pull request

Jan 13, 2025

Performance issues related to schema building#10297

Closed

Viicos force-pushed theref-schema-walking branch froma0cae6c to1fad4cfCompare

January 13, 2025 07:22

rmorshea mentioned this pull request

Jan 13, 2025

Custom type adapters#8279

Open

13 tasks

Remove invalid schemas collection

5baf9a1

Viicos force-pushed theref-schema-walking branch fromdacb6f7 to8631f3eCompare

January 16, 2025 12:33

Viicos added2 commits

January 17, 2025 12:04

Update type alias type test

0c776df

Simplify, cleanup logic

7c6d8f8

Viicos marked this pull request as ready for review

January 21, 2025 20:11

Viicos force-pushed theref-schema-walking branch 2 times, most recently from48dc1a7 to818269dCompare

January 21, 2025 20:30

Copy link

Contributor

sydney-runkle commentedJan 22, 2025

I think we can mark this as closing#10655

Copy link

Contributor

sydney-runkle commentedJan 22, 2025

To be deleted:
apply_discriminators
Walk logic in _core_utils

As in, in a new commit on this PR, you're just refraining for now?

Copy link

Contributor

sydney-runkle commentedJan 22, 2025

Consequence of the new schema traversing logic: we don't traverse schemas that have been unpacked from another model.

A few questions:

Is theM1 schema different onmain?
I don't think this is a huge issue for small cases like this, but imagine you had 10 refs toStandaloneModel orM1, then schemas would start to get quite verbose. So, two spinoff questions here - a) does this affect JSON schema gen? b) should we be concerned about increased storage cost for non-optimized validators/serializers built from these schemas?

I think it would be really good to loop in@adriangb here - he worked a lot with refs/defs in core schema gen, or so the blame says 😉.

Copy link

Contributor

sydney-runkle commentedJan 22, 2025•
edited
Loading

Alright, a few things before I dive into an in depth review:

I think the performance gains here might be worth the slightly less optimized / brief schema gen, but maybe we could find a happy medium by adding one more refs / defs collection pass - I understand we want to avoid an abundance of redundancy with cleaning though. One idea is that we could keep sort of key/value store indicating references to a certain type + do a replacement pass if a threshold (maybe 2) is passed.
Looks like tests are failing (also 3.9 tests bc we can't use a match statement)
I think one important thing for reviewers here to realize is that we attempted to move this cleaning logic to rust to speed things up, but having the logic in Python isn't much slower, and is much easier to maintain.@Viicos, could you add a note like this to the description + potentially some benchmarks if you have them on hand?

sydney-runkle reviewed

Jan 22, 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.

Generally, seems promising. I've already voiced some concerns in independent comments that we can chat about.

I think it'd be helpful to see the deletion of those other methods for comparison purposes on this PR 👍

tests/test_type_alias_type.py

Comment on lines -390 to +391

		'$defs': {'MySeq_int_': {'items': {'type': 'integer'}, 'type': 'array'}},
		'properties': {'my_int_seq': {'$ref': '#/$defs/MySeq_int_'}},
		'$defs': {'MyIntSeq': {'items': {'type': 'integer'}, 'type': 'array'}},
		'properties': {'my_int_seq': {'$ref': '#/$defs/MyIntSeq'}},

Copy link

Contributor

sydney-runkleJan 22, 2025

Choose a reason for hiding this comment

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

A note about this change, discussed in detail previously:#10655 (comment)

pydantic/_internal/_generate_schema.py OutdatedShow resolvedHide resolved

pydantic/_internal/_schema_gather.py OutdatedShow resolvedHide resolved

Copy link

MemberAuthor

Viicos commentedJan 22, 2025

Is theM1 schema different onmain?

It it the same

2. I don't think this is a huge issue for small cases like this, but imagine you had 10 refs toStandaloneModel orM1, then schemas would start to get quite verbose. So, two spinoff questions here - a) does this affect JSON schema gen? b) should we be concerned about increased storage cost for non-optimized validators/serializers built from these schemas?

If you have more that 1 reference toStandaloneModel, then you will end up with a'definitions' schema, that will be unpacked when buildingM2 anyway. So mostly this is only an issue when something is referenced once (so that the reference is inlined) and then you reuse the whole schema (in this case, schema ofM1) somewhere else.

a) It does affect JSON Schema gen
b) I don't think it will. Actually, I'm going to check how memory behaves here. Onmain, we currently copy the whole schema once (even the unpacked ones), which isn't the case here.

Looks like tests are failing (also 3.9 tests bc we can't use a match statement)

Only failing tests are because of the match statement that I need to change.

could you add a note like this to the description + potentially some benchmarks if you have them on hand?

yes sorry the PR description isn't complete yet.

Viicos force-pushed theref-schema-walking branch from818269d toafa73b0Compare

January 22, 2025 15:35

Copy link

Member

adriangb commentedJan 22, 2025

Consequence of the new schema traversing logic: we don't traverse schemas that have been unpacked from another model.
A few questions:
Is theM1 schema different onmain?
I don't think this is a huge issue for small cases like this, but imagine you had 10 refs toStandaloneModel orM1, then schemas would start to get quite verbose. So, two spinoff questions here - a) does this affect JSON schema gen? b) should we be concerned about increased storage cost for non-optimized validators/serializers built from these schemas?
I think it would be really good to loop in@adriangb here - he worked a lot with refs/defs in core schema gen, or so the blame says 😉.

The issue is not just verbose schemas but also duplicate SchemaValidator's.
ImagineStandaloneModel ishuge. We'd now be creatingmultiple (possibly hundreds, unbounded) copies of it'sSchemaValidator instead of just 1.

Copy link

MemberAuthor

Viicos commentedJan 22, 2025

The issue is not just verbose schemas but also duplicate SchemaValidator's.
ImagineStandaloneModel ishuge. We'd now be creatingmultiple (possibly hundreds, unbounded) copies of it'sSchemaValidator instead of just 1.

I'll note that this is only an issue with the following setup:

classStandaloneModel(BaseModel): ...classM1(BaseModel):m:StandaloneModel# If StandaloneModel is referenced another time,# M1's core schema is transformed in a `'definitions'` schema,# and we don't have any duplication of StandaloneModel's cs.classM2(BaseModel):m:StandaloneModel# Same note as M1...classOuter(BaseModel):m:StandaloneModel# Same note as M1 and M2m1:M1# If M1 is referenced another time,# Outer's core schema is transformed in a `'definitions'` schema,# and we don't have any duplication of M1's csm2:M2# Same    ...mx:MX

Which is relatively uncommon. Boxy's work will help here as well in reusing validator instances. JSON Schema changes is still an issue though (still valid, but the structure changes and this generally create churn in user code), so I'm looking into a way to have the previous behavior).

Feedback

1a45426

Viicos force-pushed theref-schema-walking branch 2 times, most recently fromb258a3e tofe0db49Compare

January 22, 2025 17:08

Copy link

Contributor

sydney-runkle commentedJan 24, 2025

Thanks for the clarification@Viicos. I'm less concerned now about this defs/refs simplification now that I understand it's semi-limited.

That being said, I do think think we might want to revisit an extra pass here in the future if/when we can get significant memory usage improvements with Boxy's work (cc@davidhewitt).

Happy to take another pass over this - could you remove the existing walk / discriminator logic so I can compare?

Thanks for all of your work here and the detailed explanations of some complex patterns.

Viicos added2 commits

January 24, 2025 16:06

gather -> traverse

99c46b5

Cleanup

d51d0f1

Viicos force-pushed theref-schema-walking branch 2 times, most recently fromdbced49 to0321015Compare

January 24, 2025 16:14

Copy link

Contributor

github-actionsbot commentedJan 24, 2025•
edited
Loading

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
type_adapter.py					298
pydantic/_internal
_core_utils.py					113,119-145,166-178
_dataclasses.py					174
_generate_schema.py					2511
_model_construction.py
_schema_gather.py					101-103
Project Total

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

Add tests from MS branch, finalize implementation

fbd4bc2

Viicos commented

Jan 27, 2025

View reviewed changes

pydantic/_internal/_generate_schema.py

		return schema


		def _can_be_inlined(def_ref: core_schema.DefinitionReferenceSchema) -> bool:

Copy link

MemberAuthor

ViicosJan 27, 2025

Choose a reason for hiding this comment

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

Difference from MS PR: instead of checking for specific keys in the metadata schema, we just check that there are no metadata at all.

Copy link

Contributor

sydney-runkleJan 27, 2025

Choose a reason for hiding this comment

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

Why can't we inline if there's metadata, ex json schema related metadata?

Copy link

MemberAuthor

ViicosJan 28, 2025

Choose a reason for hiding this comment

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

fromtypingimportAnnotatedfrompydanticimportBaseModel,WithJsonSchematypeTest=intclassModel(BaseModel):f:Annotated[Test,WithJsonSchema({})]Model.__pydantic_core_schema__{│'type':'definitions',│'schema': {│   │'type':'model',│   │'cls':<class'__main__.Model'>,│   │'schema': {│   │   │'type':'model-fields',│   │   │'fields': {│   │   │   │'t1': {'type':'model-field','schema': {'type':'int'},'metadata': {}},│   │   │   │'t2': {│   │   │   │   │'type':'model-field',│   │   │   │   │'schema': {'type':'definition-ref','schema_ref':'__main__.Test:124259184101792','metadata': {'<stripped>'}},│   │   │   │   │'metadata': {}│   │   │   │   }│   │   │   },│   │   │'model_name':'Model',│   │   │'computed_fields': []│   │   },│   │'config': {'title':'Model'},│   │'ref':'__main__.Model:107106664271472',│   │'metadata': {'<stripped>'}│   },│'definitions': [{'type':'int','ref':'__main__.Test:124259184101792'}]}

If you inline the definition ref, you loose the JSON Schema metadata. It could still be inlined and the metadata moved to the referenced schema, but you'll need to make a copy of it and merge metadata somehow.

Copy link

Contributor

sydney-runkleJan 28, 2025

Choose a reason for hiding this comment

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

Can you add a note about this to the code?

Viicos commented

Jan 27, 2025

View reviewed changes

tests/test_internal.py



		@pytest.mark.parametrize('deep_ref', [False,True])
		@pytest.mark.xfail(

Copy link

MemberAuthor

ViicosJan 27, 2025

Choose a reason for hiding this comment

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

Difference from MS PR due to the type refs changes (specifically the GPCS method change). The generated schemas are still valid, but in this case the output is different:

On this branch, the core schema for M1 is:

{│'type':'definitions',│'schema': {│   │'type':'model',│   │'cls':<class'__main__.M1'>,│   │'schema': {│   │   │'type':'model-fields',│   │   │'fields': {'a': {'type':'model-field','schema': {'type':'definition-ref','schema_ref':'__main__.M2:101691608428272'},'metadata': {}}},│   │   │'model_name':'M1',│   │   │'computed_fields': []│   │   },│   │'config': {'title':'M1'},│   │'ref':'__main__.M1:101691610240000',│   │'metadata': {'<stripped>'}│   },│'definitions': [│   │   {│   │   │'type':'model',│   │   │'cls':<class'__main__.M2'>,│   │   │'schema': {│   │   │   │'type':'model-fields',│   │   │   │'fields': {│   │   │   │   │'b': {│   │   │   │   │   │'type':'model-field',│   │   │   │   │   │'schema': {│   │   │   │   │   │   │'type':'model',│   │   │   │   │   │   │'cls':<class'__main__.M1'>,│   │   │   │   │   │   │'schema': {│   │   │   │   │   │   │   │'type':'model-fields',│   │   │   │   │   │   │   │'fields': {│   │   │   │   │   │   │   │   │'a': {│   │   │   │   │   │   │   │   │   │'type':'model-field',│   │   │   │   │   │   │   │   │   │'schema': {'type':'definition-ref','schema_ref':'__main__.M2:101691608428272'},│   │   │   │   │   │   │   │   │   │'metadata': {}│   │   │   │   │   │   │   │   │   }│   │   │   │   │   │   │   │   },│   │   │   │   │   │   │   │'model_name':'M1',│   │   │   │   │   │   │   │'computed_fields': []│   │   │   │   │   │   │   },│   │   │   │   │   │   │'config': {'title':'M1'},│   │   │   │   │   │   │'ref':'__main__.M1:101691610240000',│   │   │   │   │   │   │'metadata': {'<stripped>'}│   │   │   │   │   │   },│   │   │   │   │   │'metadata': {}│   │   │   │   │   }│   │   │   │   },│   │   │   │'model_name':'M2',│   │   │   │'computed_fields': []│   │   │   },│   │   │'config': {'title':'M2'},│   │   │'ref':'__main__.M2:101691608428272',│   │   │'metadata': {'<stripped>'}│   │   }│   ]}

As we can see, the core schema forM1 appears twice and could be inlined.

Viicos force-pushed theref-schema-walking branch 2 times, most recently fromc1fffce toe093879Compare

January 27, 2025 18:44

sydney-runkle reviewed

Jan 27, 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.

In general, I feel like_generate_schema.py could use a lot more documentation, given that schema gen + ref/def simplification is complicated by nature.

This constitutes a really significant improvement though, great work.

I like the new typed nature of the schema gathering results, etc. Easier to follow that logic as well! I'd like to take one more pass over_schema_gather.py, but generally, really happy with this improvement.

pydantic/_internal/_core_utils.py OutdatedShow resolvedHide resolved

tests/test_utils.pyShow resolvedHide resolved

tests/test_internal.py OutdatedShow resolvedHide resolved

pydantic/_internal/_generate_schema.py OutdatedShow resolvedHide resolved

pydantic/_internal/_generate_schema.py

		return schema


		def _can_be_inlined(def_ref: core_schema.DefinitionReferenceSchema) -> bool:

Copy link

Contributor

sydney-runkleJan 27, 2025

Choose a reason for hiding this comment

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

Why can't we inline if there's metadata, ex json schema related metadata?

pydantic/_internal/_generate_schema.py OutdatedShow resolvedHide resolved

pydantic/_internal/_generate_schema.pyShow resolvedHide resolved

Viicos force-pushed theref-schema-walking branch 2 times, most recently fromd74190f to7340799Compare

January 28, 2025 12:59

Feedback 2

c91eef1

Viicos force-pushed theref-schema-walking branch from7340799 toc91eef1Compare

January 28, 2025 13:03

sydney-runkle reviewed

Jan 28, 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.

Looking really good. Happy to approve pending consideration of my final nit picks :)

pydantic/_internal/_schema_gather.pyShow resolvedHide resolved

pydantic/_internal/_schema_gather.py OutdatedShow resolvedHide resolved

Feedback 3

76b7109

Viicos force-pushed theref-schema-walking branch fromf1db547 to76b7109Compare

January 29, 2025 12:00

Viicos mentioned this pull request

Jan 29, 2025

@model_validator causes recursion error /recursion_loop#11165

Closed

1 task

sydney-runkle approved these changes

Jan 29, 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.

Great work@Viicos, perf wins like this take a lot of deep thought and work.

@MarkusSintonen, thanks for the ideas and blueprint here! We're really excited about this perf boost.

Viicos merged commit8fe3aae intomain

Jan 29, 2025

78 checks passed

Viicos deleted the ref-schema-walking branch

January 29, 2025 17:55

Viicos added relnotes-performanceUsed for performance improvements. and removed relnotes-fixUsed for bugfixes. labels

Jan 29, 2025

Copy link

Contributor

MarkusSintonen commentedJan 30, 2025•
edited
Loading

Thanks@sydney-runkle /@Viicos!

Why the generictest_fastapi_startup_perf case shows much lower level of improvement? This one has "only"+33.86% but the previous PR had×2.2?

Generic models construction is the most painful issue as that has been so slow.

Copy link

MemberAuthor

Viicos commentedFeb 3, 2025

Why the generictest_fastapi_startup_perf case shows much lower level of improvement? This one has "only"+33.86% but the previous PR had×2.2?

It's not entirely clear to me why this benchmark had better results than the other ones. I compared both flamegraphs on this benchmark, on this PR and yours, and couldn't find any significant time difference being spent in schema gathering (as that's the only significant difference between the two PRs -- this one is implemented in Python and so is a bit slower, however this isn't significant).

Note that since then, we merged other performance improvements so the percentages aren't really comparable.

Viicos mentioned this pull request

Feb 3, 2025

Optimized traversal of schema tree for schema cleaning (GenerateSchema.clean_schema)pydantic/pydantic-core#1487

Closed

4 tasks

Labels

relnotes-performance

Used for performance improvements.

third-party-tests

Add this label on a PR to trigger 3rd party tests

4 participants

Movatterモバイル変換

Uh oh!

Refactor and optimize schema cleaning logic#11244

Refactor and optimize schema cleaning logic#11244

Uh oh!

Conversation

Viicos commentedJan 9, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Change Summary

Uh oh!

cloudflare-workers-and-pagesbot commentedJan 9, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Deploying pydantic-docs with Cloudflare Pages

Uh oh!

codspeed-hqbot commentedJan 9, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Merging#11244 willimprove performances by 33.86%

Summary

Benchmarks breakdown

Uh oh!

sydney-runkle commentedJan 22, 2025

Uh oh!

sydney-runkle commentedJan 22, 2025

Uh oh!

sydney-runkle commentedJan 22, 2025

Uh oh!

sydney-runkle commentedJan 22, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Viicos commentedJan 22, 2025

Uh oh!

adriangb commentedJan 22, 2025

Uh oh!

Viicos commentedJan 22, 2025

Uh oh!

sydney-runkle commentedJan 24, 2025

Uh oh!

github-actionsbot commentedJan 24, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Coverage report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Viicos commentedJan 9, 2025•
edited
Loading

cloudflare-workers-and-pagesbot commentedJan 9, 2025•
edited
Loading

codspeed-hqbot commentedJan 9, 2025•
edited
Loading

sydney-runkle commentedJan 22, 2025•
edited
Loading

github-actionsbot commentedJan 24, 2025•
edited
Loading

MarkusSintonen commentedJan 30, 2025•
edited
Loading