When parametrizing generic models, thegeneric_recursion_self_type context manager is entered, and is used to avoid infinite recursions if the same generic model happens to be parametrized again with the same args during the first parametrization.

However, upon exiting the context manager (and thus when the parametrized model is fully created), we forgot to remove the type ref from the set. This happened only if we were already in the process of parametrizing another model, as otherwise the_generic_recursion_cache would be reset (see theif token condition).

In theory, this couldn't cause issues because parametrized models are cached, and the cache is checkedbefore entering the context manager. However, because we have custommro() implementation on theBaseModel metaclass, this ends up causing issues is some really specific scenarios.

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

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

Dec 18, 2024

Copy link

cloudflare-workers-and-pagesbot commentedDec 18, 2024•
edited
Loading

Deploying pydantic-docs with Cloudflare Pages

Latest commit:	`75241c2`
Status:	✅ Deploy successful!
Preview URL:	https://95df4efe.pydantic-docs.pages.dev
Branch Preview URL:	https://11024.pydantic-docs.pages.dev

View logs

Copy link

codspeed-hqbot commentedDec 18, 2024•
edited
Loading

CodSpeed Performance Report

Merging#11143 willimprove performances by 6.26%

_{Comparing11024 (75241c2) withmain (c08eab2)}

Summary

⚡ 1 improvements
✅ 45 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`11024`	Change
⚡	`test_simple_model_validation[model_validate]`	43 µs	40.5 µs	+6.26%

Copy link

Contributor

github-actionsbot commentedDec 18, 2024•
edited
Loading

Coverage report

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

Copy link

MemberAuthor

Viicos commentedDec 18, 2024•
edited
Loading

I'll note, the current implementation is confusing and I think this is what led to the oversight. Here is a simpler implementation:

_generic_recursion_cache:ContextVar[set[str]]=ContextVar('_generic_recursion_cache',default=set())@contextmanagerdefgeneric_recursion_self_type(origin:type[BaseModel],args:tuple[Any, ...])->Iterator[PydanticRecursiveRef|None]:"""This contextmanager should be placed around the recursive calls used to build a generic type,    and accept as arguments the generic origin type and the type arguments being passed to it.    If the same origin and arguments are observed twice, it implies that a self-reference placeholder    can be used while building the core schema, and will produce a schema_ref that will be valid in the    final parent schema.    """previously_seen_type_refs=_generic_recursion_cache.get()type_ref=get_type_ref(origin,args_override=args)iftype_refinpreviously_seen_type_refs:self_type=PydanticRecursiveRef(type_ref=type_ref)yieldself_typeelse:try:previously_seen_type_refs.add(type_ref)yieldfinally:previously_seen_type_refs.remove(type_ref)

Diff

diff --git a/pydantic/_internal/_generics.py b/pydantic/_internal/_generics.pyindex 7b57fc71c..dc474861a 100644--- a/pydantic/_internal/_generics.py+++ b/pydantic/_internal/_generics.py@@ -401,7 +401,7 @@ def check_parameters_count(cls: type[BaseModel], parameters: tuple[Any, ...]) ->         raise TypeError(f'Too {description} parameters for {cls}; actual {actual}, expected {expected}')-_generic_recursion_cache: ContextVar[set[str] | None] = ContextVar('_generic_recursion_cache', default=None)+_generic_recursion_cache: ContextVar[set[str]] = ContextVar('_generic_recursion_cache', default=set())   @contextmanager@@ -416,24 +416,17 @@ def generic_recursion_self_type(     final parent schema.     """     previously_seen_type_refs = _generic_recursion_cache.get()-    if previously_seen_type_refs is None:-        previously_seen_type_refs = set()-        token = _generic_recursion_cache.set(previously_seen_type_refs)-    else:-        token = None+    type_ref = get_type_ref(origin, args_override=args)-    try:-        type_ref = get_type_ref(origin, args_override=args)-        if type_ref in previously_seen_type_refs:-            self_type = PydanticRecursiveRef(type_ref=type_ref)-            yield self_type-        else:+    if type_ref in previously_seen_type_refs:+        self_type = PydanticRecursiveRef(type_ref=type_ref)+        yield self_type+    else:+        try:             previously_seen_type_refs.add(type_ref)             yield+        finally:             previously_seen_type_refs.remove(type_ref)-    finally:-        if token:-            _generic_recursion_cache.reset(token)   def recursively_defined_type_refs() -> set[str]:

Edit: turns out this is not going to work well as context variables shouldn't have mutable defaults.

Viicos added the backport-2.10 label

Dec 18, 2024

sydney-runkle approved these changes

Dec 18, 2024

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.

Nice work, easy enough fix. Thanks for the thorough test.

sydney-runkle mentioned this pull request

Dec 18, 2024

Known v2.10 Issues - Fixes in Progress#10910

Closed

36 tasks

Make sure the type reference is removed from the seen references

75241c2

When parametrizing generic models, the `generic_recursion_self_type`context manager is entered, and is used to avoid infinite recursionsif the same generic model happens to be parametrized again with thesame args during the first parametrization.However, upon exiting the context manager (and thus when the parametrizedmodel is fully created), we forgot to remove the type ref from the set.This happened only if we were already in the process of parametrizinganother model, as otherwise the `_generic_recursion_cache` wouldbe reset (see the `if token` condition).In theory, this couldn't cause issues because parametrized models arecached, and the cache is checked *before* entering the context manager.However, because we have custom `mro()` implementation on the `BaseModel`metaclass, this ends up causing issues is some really specific scenarios.

Viicos force-pushed the11024 branch fromdda0572 to75241c2Compare

December 18, 2024 16:07

Copy link

MemberAuthor

Viicos commentedDec 18, 2024

Well credits goes to@kc0506 for this one

Viicosenabled auto-merge (squash)

December 18, 2024 16:09

Viicos merged commit488aa52 intomain

Dec 18, 2024

52 checks passed

Viicos deleted the 11024 branch

December 18, 2024 16:14

Viicos added a commit that referenced this pull request

Dec 18, 2024

Make sure the type reference is removed from the seen references (#11143

3bcf706

)When parametrizing generic models, the `generic_recursion_self_type`context manager is entered, and is used to avoid infinite recursionsif the same generic model happens to be parametrized again with thesame args during the first parametrization.However, upon exiting the context manager (and thus when the parametrizedmodel is fully created), we forgot to remove the type ref from the set.This happened only if we were already in the process of parametrizinganother model, as otherwise the `_generic_recursion_cache` wouldbe reset (see the `if token` condition).In theory, this couldn't cause issues because parametrized models arecached, and the cache is checked *before* entering the context manager.However, because we have custom `mro()` implementation on the `BaseModel`metaclass, this ends up causing issues is some really specific scenarios.Co-authored-by: kc0506 <kchong0506@gmail.com>

Viicos mentioned this pull request

Dec 18, 2024

Make sure the type reference is removed from the seen references#11145

Merged

5 tasks

Viicos mentioned this pull request

Dec 27, 2024

FixPydanticRecursiveRef returned bymro in multi-level generic model#11037

Closed

5 tasks

Labels

relnotes-fix

Used for bugfixes.

3 participants

Movatterモバイル変換

Uh oh!

Make sure the type reference is removed from the seen references#11143

Make sure the type reference is removed from the seen references#11143

Uh oh!

Conversation

Viicos commentedDec 18, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Change Summary

Related issue number

Checklist

Uh oh!

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

Uh oh!

CodSpeed Performance Report

Merging#11143 willimprove performances by 6.26%

Summary

Benchmarks breakdown

Uh oh!

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

Uh oh!

Coverage report

Uh oh!

Viicos commentedDec 18, 2024• 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!

Viicos commentedDec 18, 2024

Uh oh!

Uh oh!

Uh oh!

Viicos commentedDec 18, 2024•
edited
Loading

cloudflare-workers-and-pagesbot commentedDec 18, 2024•
edited
Loading

codspeed-hqbot commentedDec 18, 2024•
edited
Loading

github-actionsbot commentedDec 18, 2024•
edited
Loading

Viicos commentedDec 18, 2024•
edited
Loading