Only evaluate `FieldInfo` annotations if required during schema building #10769

Viicos · 2024-11-05T16:48:47Z

By adding a new evaluated attribute that can be set during model fields collection. Also refactor a bit the utility functions to eval such annotations.

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

Viicos · 2024-11-05T16:49:36Z

pydantic/_internal/_generate_schema.py

@@ -1215,11 +1215,15 @@ def _common_field_schema(  # C901
    ) -> _CommonField:
        # Update FieldInfo annotation if appropriate:
        FieldInfo = import_cached_field_info()
-        if has_instance_in_type(field_info.annotation, (ForwardRef, str)):


The interesting part is that we remove the call to has_instance_in_type here.

cloudflare-workers-and-pages · 2024-11-05T16:50:15Z

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`076da3c`
Status:	✅ Deploy successful!
Preview URL:	https://bf7a63be.pydantic-docs.pages.dev
Branch Preview URL:	https://fieldinfo-evaluated.pydantic-docs.pages.dev

View logs

pydantic/_internal/_generate_schema.py

Viicos · 2024-11-05T16:51:59Z

pydantic/_internal/_generate_schema.py

@@ -1338,12 +1342,13 @@ def _type_alias_type_schema(self, obj: TypeAliasType) -> CoreSchema:
                return maybe_schema

            origin: TypeAliasType = get_origin(obj) or obj
-
-            annotation = origin.__value__


Moved below, as accessing the __value__ attribute will trigger the evaluation of the annotation, and thus this can raise a NameError. The eval_type call is still necessary though, as you can have explicit string references inside a a type alias type.

Maybe I'm misunderstanding - was this moved below? Or do we not need this specific line anymore?

Basically, here is what can happen with type aliases:

type A = Int annotation = origin.__value__ # NameError, 'Int' is not defined type B = 'Int' annotation = origin.__value__ assert annotation == 'Int' eval_type(B, ..., ...) # NameError, 'Int' is not defined

So everything was moved in a single try..except block:

try: annotation = _typing_extra.eval_type(origin.__value__, *self._types_namespace) except ...: ...

codspeed-hq · 2024-11-05T16:54:59Z

CodSpeed Performance Report

Merging #10769 will improve performances by 8.54%

_{Comparing fieldinfo-evaluated (076da3c) with main (6e91707)}

Summary

⚡ 5 improvements
✅ 39 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`fieldinfo-evaluated`	Change
⚡	`test_complex_model_schema_generation`	2.8 ms	2.6 ms	+6.33%
⚡	`test_lots_of_models_with_lots_of_fields`	3.6 s	3.3 s	+8.54%
⚡	`test_simple_model_schema_lots_of_fields_generation`	41.8 ms	38.8 ms	+7.73%
⚡	`test_nested_recursive_generic_model_schema_generation`	2.3 ms	2.2 ms	+5.79%
⚡	`test_nested_recursive_model_schema_generation`	2.3 ms	2.2 ms	+6.21%

pydantic/_internal/_typing_extra.py

github-actions · 2024-11-05T17:13:26Z

Coverage report

This PR does not seem to contain any modification to coverable code.

sydney-runkle

Alrighty, left some initial comments on the implementation.

The tuple return doesn't feel like the cleanest implementation, but I can't come up with anything better off the top of my head. Let's chat first thing tomorrow morning about alternatives. I'm not opposed to this approach, but want to think about other options before we move forward 👍

pydantic/_internal/_typing_extra.py

sydney-runkle · 2024-11-07T00:36:57Z

pydantic/_internal/_typing_extra.py

-    '`eval_type_lenient` is deprecated, use `eval_type` with `lenient=True` instead.',
+    '`eval_type_lenient` is deprecated, use `try_eval_type` instead.',


Hmm, this feels like an anti-pattern - if we already deprecated this, there's probably a reason why we didn't want to have two functions. Even if we disagree with this deprecation now, seems like a lot of churn to basically reintroduce the function and redirect for the deprecated case...

I believe my suggestion above re always returning a second value, evaluated, even in the case of eval_type with lenient=False might help, and we can leave this be?

A few more follow ups:

This is a very internal function - can we just deprecate this?

If we were to bring back a second function, to reduce churn, can we call it eval_type_lenient?

I had to keep this function during the types namespaces refactor because fastapi makes use of it 🫤. I added the deprecated decorator so that type checking fails in the fastapi CI and they can use the correct function (or best, do not rely on our internals because I don't think the _typing_extra module is stable enough, we might apply other changes here in the future).

I believe my suggestion above re always returning a second value, evaluated, even in the case of eval_type with lenient=False might help, and we can leave this be?

Same issue as https://github.com/pydantic/pydantic/pull/10769/files#r1832947088, we can't have a single function because we want to catch the NameError to reraise PydanticUndefinedAnnotation

pydantic/_internal/_generate_schema.py

sydney-runkle · 2024-11-07T00:41:19Z

pydantic/_internal/_generate_schema.py

@@ -1338,12 +1342,13 @@ def _type_alias_type_schema(self, obj: TypeAliasType) -> CoreSchema:
                return maybe_schema

            origin: TypeAliasType = get_origin(obj) or obj
-
-            annotation = origin.__value__


Maybe I'm misunderstanding - was this moved below? Or do we not need this specific line anymore?

sydney-runkle · 2024-11-07T11:24:34Z

Wow @Viicos, nice work here. Didn't realize this would have that significant of an impact.

We can chat about potential other APIs for this, but we should definitely move forward with this change :).

Does this unblock the _typing_extra.py refactor PR?

sydney-runkle · 2024-11-07T11:25:28Z

This is great. I presume next we'll look at _apply_annotations, and we know there's a lot to gain there.

sydney-runkle · 2024-11-07T19:24:47Z

Another follow up - what if we returned something like this:

from python import TypedDict

class TypeEvaluationResult(TypedDict):
    evaluated: bool
    result: Any
    error: NameError | None

Or something like:

from python import TypedDict, TypeAlias

class TypeEvaluationSuccess(TypedDict):
    evaluated: Literal[True]
    result: Any

class TypeEvaluationFailure(TypedDict):
    evaluated: Literal[False]
    error: NameError

type TypeEvaluationResult = TypeEvaluationSuccess | TypeEvaluationFailure

Doesn't have to be either of those exactly, but I think you get the point :). Then, we could have one function and preserve relevant errors, but also intuitively handle attempted evals?

Maybe this isn't better, but I think it does help us avoid the overloads, though admittedly there's more conditional logic required after a function call, as the result can be many things.

Could even do a named tuple.

Viicos · 2024-11-08T13:35:47Z

Seems like both the overload and structured return value have different drawbacks: what is result if it failed to evaluate? How do you safely make use of result, even if you checked that error is None before ? (as type checkers will complain).

Maybe it's worth revisiting the API, if we want to make this module more stable and maybe expose it publicly, as it is used already by some other projects.

sydney-runkle · 2024-11-08T13:59:13Z

Yeah, fair point. Let's not go with the structured return.

One other idea - could we revert to that single signature idea, and manufacture a NameError if evaluated is False based on the input?

Viicos · 2024-11-08T16:30:20Z

You mean something like dict[str, tuple[Any, bool | NameError]] in the return type?

sydney-runkle · 2024-11-08T16:35:05Z

Or even like dict[str, tuple[Any, bool], but when the second value is false and we want to raise on failure, just then raise PydanticUndefinedAnnotation(<information about the failed eval similar to what the NameError would have provided>)

Viicos · 2024-11-12T13:00:31Z

but the issue is we currently raise using PydanticUndefinedAnnotation.from_name_error so that's why we need the exception to be raised originally.

dict[str, tuple[Any, bool | NameError]] could work but just moves the burden of checking for bool (actually Literal[True]) and NameError to the caller.

sydney-runkle

Looking over this again, seems reasonable, given the constraints we've discussed re typing and overloads.

sydney-runkle · 2024-11-14T15:25:47Z

Going to go ahead and merge. We can cherry pick relevant stuff for the v2.10 official release (not including this).

By adding a new `evaluated` attribute that can be set during model fields collection. Also refactor a bit the utility functions to eval such annotations.

…ing (#10769)

github-actions bot added the relnotes-fix Used for bugfixes. label Nov 5, 2024

Viicos commented Nov 5, 2024

View reviewed changes

pydantic/_internal/_generate_schema.py Show resolved Hide resolved

Viicos commented Nov 5, 2024

View reviewed changes

pydantic/_internal/_typing_extra.py Show resolved Hide resolved

Viicos force-pushed the fieldinfo-evaluated branch 2 times, most recently from 367f48a to 88aa322 Compare November 5, 2024 17:06

sydney-runkle reviewed Nov 7, 2024

View reviewed changes

sydney-runkle added topic-performance and removed relnotes-fix Used for bugfixes. labels Nov 7, 2024

sydney-runkle mentioned this pull request Nov 7, 2024

Refactor _typing_extra module #10725

Merged

5 tasks

sydney-runkle approved these changes Nov 13, 2024

View reviewed changes

sydney-runkle added relnotes-performance Used for performance improvements. and removed topic-performance labels Nov 14, 2024

sydney-runkle enabled auto-merge (squash) November 14, 2024 16:07

Viicos added 2 commits November 14, 2024 17:47

Only evaluate FieldInfo annotations if required during schema building

49c57bf

By adding a new `evaluated` attribute that can be set during model fields collection. Also refactor a bit the utility functions to eval such annotations.

Feedback

076da3c

Viicos force-pushed the fieldinfo-evaluated branch from ed6e232 to 076da3c Compare November 14, 2024 16:47

sydney-runkle merged commit bcfd413 into main Nov 14, 2024
52 checks passed

sydney-runkle deleted the fieldinfo-evaluated branch November 14, 2024 16:53

sydney-runkle pushed a commit that referenced this pull request Nov 26, 2024

Only evaluate FieldInfo annotations if required during schema build…

d6fc7fc

…ing (#10769)

Viicos mentioned this pull request Feb 5, 2025

Collect model fields when rebuilding a model #11388

Merged

5 tasks

		'`eval_type_lenient` is deprecated, use `eval_type` with `lenient=True` instead.',
		'`eval_type_lenient` is deprecated, use `try_eval_type` instead.',

Uh oh!

Only evaluate FieldInfo annotations if required during schema building #10769

Only evaluate FieldInfo annotations if required during schema building #10769

Uh oh!

Conversation

Viicos commented Nov 5, 2024

Change Summary

Related issue number

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudflare-workers-and-pages bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying pydantic-docs with Cloudflare Pages

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!

codspeed-hq bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #10769 will improve performances by 8.54%

Summary

Benchmarks breakdown

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sydney-runkle commented Nov 7, 2024

Uh oh!

sydney-runkle commented Nov 7, 2024

Uh oh!

sydney-runkle commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Viicos commented Nov 8, 2024

Uh oh!

sydney-runkle commented Nov 8, 2024

Uh oh!

Viicos commented Nov 8, 2024

Uh oh!

sydney-runkle commented Nov 8, 2024

Uh oh!

Viicos commented Nov 12, 2024

Uh oh!

sydney-runkle left a comment

Choose a reason for hiding this comment

Uh oh!

sydney-runkle commented Nov 14, 2024

Uh oh!

Uh oh!

Uh oh!

Only evaluate `FieldInfo` annotations if required during schema building #10769

Only evaluate `FieldInfo` annotations if required during schema building #10769

cloudflare-workers-and-pages bot commented Nov 5, 2024 •

edited

Loading

codspeed-hq bot commented Nov 5, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024 •

edited

Loading

sydney-runkle commented Nov 7, 2024 •

edited

Loading