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

`CoreMetadata` refactor with an emphasis on documentation and reducing complexity#10675

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

sydney-runkle merged 20 commits intomainfrommetadata-stuck-on-a-flight

Oct 28, 2024

Merged

`CoreMetadata` refactor with an emphasis on documentation and reducing complexity#10675

sydney-runkle merged 20 commits intomainfrommetadata-stuck-on-a-flight

Oct 28, 2024

Conversation

Copy link

Contributor

sydney-runkle commentedOct 21, 2024•
edited
Loading

I would recommend reviewing this commit by commit, though looking over the whole thing afterwards gives some helpful perspective as well.

Changes in this PR:

Remove theCoreMetadataHandler structure - it offered minimal type checking benefits, some overhead, and a good amount of unnecessary complexity.
Better documentation for theCoreMetadata dict so that it's clear what we're adding to it and why.
Remove as much of the json schema (callable) generation logic from_generate_schema.py and_known_annotated_metadata.py and consolidate it injson_schema.py
For basic json schema updates andjson_schema_extra updates, no longer build a callable ahead of time.
As a nice benefit, seeing some 2-5% improvements in schema build times, which is good considering this is just JSON schema related changes.

TODOs, in the future:

Minimize amount of callable wrapper logic we need for JSON schema generation - requires a major refactor, unfortunately.
Move theCoreMetadata structure to rust so that we can pull out some of thecast calls and just get natural type checking benefits there. One hiccup here is that replacingmetadata: dict[str, Any] withmetadata: CoreMetadata, doesn't allow for arbitrary injection (say, in a custom schema), so I'm not sure what we want to do about that. It also then raises errors when we do our temporary injects for say, discriminator application, but we shouldn't be doing that anyway.
Standardize whether or not we simplify refs for callable json schema application so we don't needpydantic_js_functions andpydantic_js_annotation_functions

sydney-runkle added3 commits

October 21, 2024 15:21

first step, removing CoreMetadataHandler construct - type checking va…

fbc5c9c

…lue is overridden by introduced complexity and overhead

remove reliance on build_metadata_dict

bd181f1

standardizing dict updates and json schema extra

f9c346d

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

Oct 21, 2024

Copy link

cloudflare-workers-and-pagesbot commentedOct 21, 2024•
edited
Loading

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`2a08ddf`
Status:	✅ Deploy successful!
Preview URL:	https://1b3fed65.pydantic-docs.pages.dev
Branch Preview URL:	https://metadata-stuck-on-a-flight.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hqbot commentedOct 21, 2024•
edited
Loading

CodSpeed Performance Report

Merging#10675 willnot alter performance

_{Comparingmetadata-stuck-on-a-flight (2a08ddf) withmain (19f7d41)}

Summary

✅ 44 untouched benchmarks

sydney-runkle changed the title~~CoreMetadata refactor as a consequence of a very long flight~~CoreMetadata refactor with an emphasis on documentation and reducing complexity

Oct 22, 2024

sydney-runkle added4 commits

October 22, 2024 17:04

removing as much json schema logic as possible from _generate_schema.…

a1442ff

…py and migrating to json_schema.py

minor docs fixes + revert enum change to fix tests

f93552c

Add some casting + standardize update methods so that CoreMetadata of…

9d8f4a4

…fers some typing support - to be improved with migration to pydantic-core

remove deprecated internal function

e3b7426

sydney-runkle commented

Oct 23, 2024

View reviewed changes

docs/concepts/json_schema.md OutdatedShow resolvedHide resolved

sydney-runkle commented

Oct 23, 2024

View reviewed changes

pydantic/_internal/_core_metadata.py Outdated

Comment on lines 51 to 52

		pydantic_js_prefer_positional_arguments: bool \| None = None,
		pydantic_js_input_core_schema: CoreSchema \| None = None,

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

This is probably overkill (these params specifically). Happy to remove. The others make more sense given that they need special update logic.

sydney-runkle commented

Oct 23, 2024

View reviewed changes

pydantic/_internal/_generate_schema.pyShow resolvedHide resolved

sydney-runkle commented

Oct 23, 2024

View reviewed changes

pydantic/_internal/_generate_schema.pyShow resolvedHide resolved

sydney-runkle commented

Oct 23, 2024

View reviewed changes

pydantic/_internal/_generate_schema.py

Comment on lines -2474 to -2510

		def get_json_schema_update_func(
		json_schema_update: JsonSchemaValue, json_schema_extra: JsonDict \| typing.Callable[[JsonDict], None] \| None
		) -> GetJsonSchemaFunction:
		def json_schema_update_func(
		core_schema_or_field: CoreSchemaOrField, handler: GetJsonSchemaHandler
		) -> JsonSchemaValue:
		json_schema = {handler(core_schema_or_field), json_schema_update}
		add_json_schema_extra(json_schema, json_schema_extra)
		return json_schema

		return json_schema_update_func


		def add_json_schema_extra(
		json_schema: JsonSchemaValue, json_schema_extra: JsonDict \| typing.Callable[[JsonDict], None] \| None
		):
		if isinstance(json_schema_extra, dict):
		json_schema.update(to_jsonable_python(json_schema_extra))
		elif callable(json_schema_extra):
		json_schema_extra(json_schema)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Also, shouldn't be injson_schema.py

sydney-runkle commented

Oct 23, 2024

View reviewed changes

pydantic/_internal/_known_annotated_metadata.pyShow resolvedHide resolved

sydney-runkle commented

Oct 23, 2024

View reviewed changes

pydantic/json_schema.pyShow resolvedHide resolved

sydney-runkle commented

Oct 23, 2024

View reviewed changes

pydantic/json_schema.py

Comment on lines +1484 to +1494

		def _update_class_schema(self, json_schema: JsonSchemaValue, cls: type[Any], config: ConfigDict) -> None:
		"""Update json_schema with the following, extracted from `config` and `cls`:

		* title
		* description
		* additional properties
		* json_schema_extra
		* deprecated

		Done in place, hence there's no return value as the original json_schema is mutated.
		No ref resolving is involved here, as that's not appropriate for simple updates.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

This now has lots of the logic that was mistakenly in_generate_schema.py

sydney-runkle commented

Oct 23, 2024

View reviewed changes

pydantic/json_schema.py

		schema_to_update =self.get_schema_from_definitions(JsonRef(ref))
		if schema_to_update is None:
		raise RuntimeError(f'Cannot update undefined schema for $ref={ref}')
		returnself.resolve_ref_schema(schema_to_update)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Renamed and simplified. There's a function called this inGenerateJsonSchemaHandler, and ideally, we'd get rid of that eventually, so I think this helps draw some parallels to make that simplification easier in the future.

sydney-runkle commented

Oct 23, 2024

View reviewed changes

tests/test_json_schema.py

Comment on lines -2616 to -2630

		def test_typeddict_with_extra_behavior_allow():
		class Model:
		@classmethod
		def __get_pydantic_core_schema__(cls, source_type: Any, handler: GetCoreSchemaHandler) -> CoreSchema:
		return core_schema.typed_dict_schema(
		{'a': core_schema.typed_dict_field(core_schema.str_schema())},
		extra_behavior='allow',
		)

		assert TypeAdapter(Model).json_schema() == {
		'type': 'object',
		'properties': {'a': {'title': 'A', 'type': 'string'}},
		'required': ['a'],
		'additionalProperties': True,
		}

Copy link

ContributorAuthor

sydney-runkleOct 23, 2024

Choose a reason for hiding this comment

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

So, I think we should only be pulling theextra_behavior from the config here. If folks disagree, I can pull from the schema as well, though not sure why we have this setting there - seems redundant.

Copy link

Member

ViicosOct 24, 2024

Choose a reason for hiding this comment

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

extra items are going to be supported soon for typed dicts in Python. Is Pydantic support for it going to be easy to implement with these changes?

adding warnings for lack of mixed composure support for json_schema_e…

a1ebd03

…xtra

hyperlint-aibot reviewed

Oct 23, 2024

View reviewed changes

Copy link

Contributor

hyperlint-aibot left a comment

Choose a reason for hiding this comment

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

1 files reviewed, 1 total issue(s) found.

docs/concepts/json_schema.mdShow resolvedHide resolved

typing fix for 3.13

41bc424

Copy link

Contributor

github-actionsbot commentedOct 23, 2024•
edited
Loading

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
pydantic
fields.py
functional_validators.py
json_schema.py
pydantic/_internal
_core_metadata.py					68,87,95
_core_utils.py
_generate_schema.py
_known_annotated_metadata.py					270
Project Total

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

sydney-runkle added2 commits

October 23, 2024 15:13

fix circular reference issue

7e479fe

add list of failing tests

aea1c00

sydney-runkle added the relnotes-changeUsed for changes to existing functionality which don't have a better categorization. label

Oct 23, 2024

maybe this is the way you skip that test?

d8429f1

dmontagu reviewed

Oct 23, 2024

View reviewed changes

pydantic/_internal/_core_metadata.py OutdatedShow resolvedHide resolved

dmontagu reviewed

Oct 23, 2024

View reviewed changes

pydantic/fields.py OutdatedShow resolvedHide resolved

dmontagu approved these changes

Oct 23, 2024

View reviewed changes

Copy link

Contributor

dmontagu left a comment

Choose a reason for hiding this comment

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

Generally looking good to me other than the couple comments I made above about the warning message; not sure if you plan to address any of the TODO comments you added before merging, but I'm okay even if they aren't addressed.

Copy link

ContributorAuthor

sydney-runkle commentedOct 24, 2024

Wasn't planning on addressing the TODOs before merging - I feel like this is big enough already, and the callable stuff would represent a pretty significant structural change.

I will work on getting the fastapi CI step to green, though.

Viicos requested changes

Oct 24, 2024

View reviewed changes

Copy link

Member

Viicos left a comment

Choose a reason for hiding this comment

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

Changes look reasonable and indeed having less stuff in theGenerateSchema class is much better.

Move theCoreMetadata structure to rust so that we can pull out some of thecast calls and just get natural type checking benefits there.

Will we get any benefits to move things to Rust here? Will this introduce an extra level of indirection, making things harder to understand?

pydantic/json_schema.py OutdatedShow resolvedHide resolved

pydantic/_internal/_core_metadata.pyShow resolvedHide resolved

pydantic/_internal/_core_metadata.py OutdatedShow resolvedHide resolved

pydantic/_internal/_known_annotated_metadata.pyShow resolvedHide resolved

pydantic/fields.py OutdatedShow resolvedHide resolved

tests/test_fastapi.sh OutdatedShow resolvedHide resolved

tests/test_json_schema.py

Comment on lines -2616 to -2630

		def test_typeddict_with_extra_behavior_allow():
		class Model:
		@classmethod
		def __get_pydantic_core_schema__(cls, source_type: Any, handler: GetCoreSchemaHandler) -> CoreSchema:
		return core_schema.typed_dict_schema(
		{'a': core_schema.typed_dict_field(core_schema.str_schema())},
		extra_behavior='allow',
		)

		assert TypeAdapter(Model).json_schema() == {
		'type': 'object',
		'properties': {'a': {'title': 'A', 'type': 'string'}},
		'required': ['a'],
		'additionalProperties': True,
		}

Copy link

Member

ViicosOct 24, 2024

Choose a reason for hiding this comment

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

extra items are going to be supported soon for typed dicts in Python. Is Pydantic support for it going to be easy to implement with these changes?

pydantic/json_schema.pyShow resolvedHide resolved

Copy link

ContributorAuthor

sydney-runkle commentedOct 24, 2024

Will we get any benefits to move things to Rust here? Will this introduce an extra level of indirection, making things harder to understand?

My only thought here was then we could then typemetadata asCoreMetadata instead ofdict[str, Any] so that we can avoid some of the casting here and get more type checking across the board.

PR feedback

88f9247

Copy link

ContributorAuthor

sydney-runkle commentedOct 25, 2024

extra items are going to be supported soon for typed dicts in Python

This is where moving this to rust would be super helpful, then we can typemetadata: CoreMetadata, but extra values will be allowed both from the user side and when we're doing the odd mutation for discriminator mutation stuff.

Copy link

ContributorAuthor

sydney-runkle commentedOct 25, 2024

Is Pydantic support for it going to be easy to implement with these changes?

Yes, I haven't removed theextra_behavior attribute, I just think we should pull that info from config so that we don't have two ways to set it. Can have a different chat about this, I don't think the change here is super meaningful.

sydney-runkleand others added2 commits

October 25, 2024 13:51

root description fix

b572456

Merge branch 'main' into metadata-stuck-on-a-flight

7a17254

Copy link

Member

Viicos commentedOct 28, 2024

Yes, I haven't removed theextra_behavior attribute, I just think we should pull that info from config so that we don't have two ways to set it. Can have a different chat about this, I don't think the change here is super meaningful.

I would rather support thePEP 728 added feature and slowly deprecate pulling the info from the config (or support both, but disallow having both specified at the same time?)

sydney-runkle added4 commits

October 28, 2024 11:31

pr feedback re typing

49e75eb

Merge branch 'metadata-stuck-on-a-flight' ofhttps://github.com/pydan…

8d09bc7

…tic/pydantic into metadata-stuck-on-a-flight

import fix

d66ae3b

deselect multiple

5f8b5ba

sydney-runkle requested a review fromViicos

October 28, 2024 16:26

fix test skip formatting

2a08ddf

Viicos approved these changes

Oct 28, 2024

View reviewed changes

Copy link

Member

Viicos left a comment

Choose a reason for hiding this comment

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

I'm also fine with removingextra_behavior for now, and re-add it in some way with PEP 728 coming soon

pydantic/_internal/_known_annotated_metadata.py

Comment on lines 14 to +15

		if TYPE_CHECKING:
		from ..annotated_handlers import GetJsonSchemaHandler
		pass

Copy link

Member

ViicosOct 28, 2024

Choose a reason for hiding this comment

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

Can be removed

sydney-runkle merged commit80353c2 intomain

Oct 28, 2024

56 checks passed

sydney-runkle deleted the metadata-stuck-on-a-flight branch

October 28, 2024 21:29

samuelcolvin mentioned this pull request

Nov 7, 2024

Change JSON schema (and maybe config) default forextra fordataclasses andTypedDicts#10785

Open

1 task

tamird reviewed

Nov 22, 2024

View reviewed changes

tests/test_fastapi.sh

		# Remove the first one once that test is fixed, see https://github.com/pydantic/pydantic/pull/10029
		# the remaining tests all failing bc we now correctly add a `'deprecated': True` attribute to the JSON schema,
		# So it's the FastAPI tests that need to be updated here
		./scripts/test.sh -vv \

Copy link

Contributor

tamirdNov 22, 2024•
edited
Loading

Choose a reason for hiding this comment

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

FYI I think the first one is already passing and the others should be fixed byhttps://github.com/fastapi/fastapi/pull/12971/files.

Copy link

ContributorAuthor

sydney-runkleNov 22, 2024

Choose a reason for hiding this comment

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

Awesome, thanks!

If you wouldn't mind, could you open a PR againstpydantic to remove these once yourFastAPI PR is merged? Thanks!!

Copy link

Contributor

tamirdNov 22, 2024

Choose a reason for hiding this comment

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

I'd be happy to! Judging by my past experience with FastAPI I wouldn't expect that to happen any time soon 🙃

Copy link

ContributorAuthor

sydney-runkleNov 22, 2024

Choose a reason for hiding this comment

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

Thanks so much!!

Copy link

Contributor

tamirdNov 22, 2024

Choose a reason for hiding this comment

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

#10948

Labels

relnotes-change

Used for changes to existing functionality which don't have a better categorization.

relnotes-fix

Used for bugfixes.

4 participants

Movatterモバイル変換

Uh oh!

CoreMetadata refactor with an emphasis on documentation and reducing complexity#10675

CoreMetadata refactor with an emphasis on documentation and reducing complexity#10675

Uh oh!

Conversation

sydney-runkle commentedOct 21, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

cloudflare-workers-and-pagesbot commentedOct 21, 2024• 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 commentedOct 21, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Merging#10675 willnot alter performance

Summary

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

hyperlint-aibot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actionsbot commentedOct 23, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Coverage report

Uh oh!

Uh oh!

Uh oh!

dmontagu left a comment

Choose a reason for hiding this comment

Uh oh!

sydney-runkle commentedOct 24, 2024

Uh oh!

Viicos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sydney-runkle commentedOct 24, 2024

Uh oh!

sydney-runkle commentedOct 25, 2024

Uh oh!

sydney-runkle commentedOct 25, 2024

Uh oh!

Viicos commentedOct 28, 2024

Uh oh!

Viicos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamirdNov 22, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

`CoreMetadata` refactor with an emphasis on documentation and reducing complexity#10675

`CoreMetadata` refactor with an emphasis on documentation and reducing complexity#10675

sydney-runkle commentedOct 21, 2024•
edited
Loading

cloudflare-workers-and-pagesbot commentedOct 21, 2024•
edited
Loading

codspeed-hqbot commentedOct 21, 2024•
edited
Loading

github-actionsbot commentedOct 23, 2024•
edited
Loading

tamirdNov 22, 2024•
edited
Loading