`CoreMetadata` refactor with an emphasis on documentation and reducing complexity by sydney-runkle · Pull Request #10675 · pydantic/pydantic

sydney-runkle · 2024-10-21T17:26:53Z

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 the CoreMetadataHandler structure - it offered minimal type checking benefits, some overhead, and a good amount of unnecessary complexity.
Better documentation for the CoreMetadata 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 in json_schema.py
For basic json schema updates and json_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 the CoreMetadata structure to rust so that we can pull out some of the cast calls and just get natural type checking benefits there. One hiccup here is that replacing metadata: dict[str, Any] with metadata: 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 need pydantic_js_functions and pydantic_js_annotation_functions

…lue is overridden by introduced complexity and overhead

cloudflare-workers-and-pages · 2024-10-21T17:28:18Z

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

codspeed-hq · 2024-10-21T17:33:29Z

CodSpeed Performance Report

Merging #10675 will not alter performance

_{Comparing metadata-stuck-on-a-flight (2a08ddf) with main (19f7d41)}

Summary

✅ 44 untouched benchmarks

…py and migrating to json_schema.py

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

sydney-runkle · 2024-10-23T15:46:55Z

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


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

sydney-runkle · 2024-10-23T15:49:06Z

-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)
-
-


Also, shouldn't be in json_schema.py

sydney-runkle · 2024-10-23T15:51:13Z

+    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.


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

sydney-runkle · 2024-10-23T15:52:18Z

+        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}')
+        return self.resolve_ref_schema(schema_to_update)


Renamed and simplified. There's a function called this in GenerateJsonSchemaHandler, 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 · 2024-10-23T15:53:17Z

-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,
-    }


So, I think we should only be pulling the extra_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.

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?

…xtra

hyperlint-ai-deprecated

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

github-actions · 2024-10-23T19:13:55Z

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 by python-coverage-comment-action}

dmontagu

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.

sydney-runkle · 2024-10-24T01:22:22Z

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

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

Move the CoreMetadata structure to rust so that we can pull out some of the cast 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?

Viicos · 2024-10-24T15:08:01Z

-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,
-    }


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?

sydney-runkle · 2024-10-24T15:49:38Z

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 type metadata as CoreMetadata instead of dict[str, Any] so that we can avoid some of the casting here and get more type checking across the board.

sydney-runkle · 2024-10-25T17:47:57Z

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 type metadata: CoreMetadata, but extra values will be allowed both from the user side and when we're doing the odd mutation for discriminator mutation stuff.

sydney-runkle · 2024-10-25T17:48:57Z

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

Yes, I haven't removed the extra_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.

Viicos · 2024-10-28T14:39:43Z

Yes, I haven't removed the extra_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 the PEP 728 added feature and slowly deprecate pulling the info from the config (or support both, but disallow having both specified at the same time?)

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

Viicos

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

Viicos · 2024-10-28T20:34:57Z

 if TYPE_CHECKING:
-    from ..annotated_handlers import GetJsonSchemaHandler
+    pass


Can be removed

tamird · 2024-11-22T14:36:56Z

+# 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 \


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

Awesome, thanks!

If you wouldn't mind, could you open a PR against pydantic to remove these once your FastAPI PR is merged? Thanks!!

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

Thanks so much!!

sydney-runkle added 3 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-actions Bot added the relnotes-fix Used for bugfixes. label Oct 21, 2024

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 added 4 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