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

Simplify handling of typing.Annotated in GenerateSchema #6887

Merged
merged 2 commits into from Jul 26, 2023

Conversation

dmontagu
Copy link
Contributor

Closes #6884

@adriangb It seems to me some of the cruft removed in this PR might have been left around from previous fiddling that has been rendered unnecessary. But if you see issues with this we can revert some of the other changes; I'll note that an alternative diff that also closes #6884 is just:

diff --git a/pydantic/_internal/_generate_schema.py b/pydantic/_internal/_generate_schema.py
index 4b4289ac..2b2de757 100644
--- a/pydantic/_internal/_generate_schema.py
+++ b/pydantic/_internal/_generate_schema.py
@@ -1222,7 +1222,7 @@ class GenerateSchema:
         item_type = self._get_first_arg_or_any(sequence_type)
 
         def json_schema_func(_schema: CoreSchemaOrField, handler: GetJsonSchemaHandler) -> JsonSchemaValue:
-            items_schema = self._generate_schema(item_type)
+            items_schema = self.generate_schema(item_type)
             return handler(core_schema.list_schema(items_schema))
 
         metadata = build_metadata_dict(js_functions=[json_schema_func])

But looking into the code, it seemed to me this was a "safer" solution. But I think you know this code best at this point.

@@ -391,6 +392,17 @@ def collect_definitions(self, schema: CoreSchema) -> CoreSchema:
list(self.defs.definitions.values()),
)

def _add_js_function(self, metadata_schema: CoreSchema, js_function: Callable[..., Any]) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function just moved, no changes were made

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jul 26, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8c6d630
Status: ✅  Deploy successful!
Preview URL: https://2f488098.pydantic-docs2.pages.dev
Branch Preview URL: https://simplify-annotated-core-sche.pydantic-docs2.pages.dev

View logs

if js_function not in pydantic_js_functions:
pydantic_js_functions.append(js_function)

def _generate_schema_for_type(
Copy link
Contributor Author

@dmontagu dmontagu Jul 26, 2023

Choose a reason for hiding this comment

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

I eliminated this method since the only place it was called was in generate_schema, and with the removal of the handling of _annotated_schema there, generate_schema was just immediately calling this function with the exact same signature and arguments passing through without modification.

Comment on lines -1545 to -1546
if isinstance(obj, type(Annotated[int, 123])):
schema = transform_inner_schema(self._annotated_schema(obj))
Copy link
Contributor Author

@dmontagu dmontagu Jul 26, 2023

Choose a reason for hiding this comment

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

This branch no longer seemed necessary now that _generate_schema handles Annotated properly (and Annotated doesn't hit any of the other property extractions as far as I can tell). So I just eliminated the branch here, that's all that's happening in this chunk.

@@ -87,6 +87,7 @@
from ._schema_generation_shared import GetJsonSchemaFunction

_SUPPORTS_TYPEDDICT = sys.version_info >= (3, 12)
_AnnotatedType = type(Annotated[int, 123])
Copy link
Contributor Author

@dmontagu dmontagu Jul 26, 2023

Choose a reason for hiding this comment

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

Repeated calls to type and Annotated.__class_getitem__ while building schemas seemed unnecessary/inefficient, but I didn't profile this or anything. Probably negligible overhead but just ... rubbed me the wrong way reading isinstance(obj, type(Annotated[int, 123])) in a frequently-executed loop (at least during model creation) lol. Can revert this change if preferred.

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@dmontagu dmontagu merged commit 14f27b5 into main Jul 26, 2023
48 checks passed
@dmontagu dmontagu deleted the simplify-annotated-core-schema branch July 26, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[V2] List or Sequence of discriminated unions fails to produce the json schema
2 participants