Handle X | Y union in GenericModel #4977

thenx · 2023-01-23T15:16:56Z

Change Summary

The root cause of the issue is that X | Y has type types.UnionType which does not have __getitem__. This makes pydantic.generics.replace_types to fail with TypeError: 'type' object is not subscriptable.

In this PR we handle types.UnionType separately.

Related issue number

fix #4146

Peculiarities

Had to mute Do not compare types, use 'isinstance()' (E721)
For some reason there was no TypeError when running for the second time the code from the issue. Didn't investigate.

Checklist

Unit tests for the changes exist
Tests pass on CI and coverage remains at 100%
Documentation reflects the changes where applicable
changes/<pull request or issue id>-<github username>.md file added describing change
(see changes/README.md for details)
My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

thenx · 2023-01-23T15:39:58Z

please review

samuelcolvin

Seems like a good start.

@dmontagu what do you think?

changes/4146-thenx.md

samuelcolvin · 2023-01-23T15:55:58Z

tests/test_generics.py

@@ -853,6 +853,17 @@ class Model(GenericModel, Generic[T]):
        # example)
        assert replace_types(list[Union[str, list, T]], {T: int}) == list[Union[str, list, int]]

+    if sys.version_info >= (3, 10):


separate test with skipif please.

also, please can you add a standalone test for unions on a generic model.

There should be examples elsewhere of testing | style unions.

separate test with skipif please.

done

also, please can you add a standalone test for unions on a generic model.

There should be examples elsewhere of testing | style unions.

kinda done though i don't have a feeling that that's enough and might add some more later.

sounds good, let me know when it's done.

samuelcolvin · 2023-01-23T15:59:53Z

pydantic/generics.py

+        # PEP-604 syntax (Ex.: list | str) is represented with a types.UnionType object that does not have __getitem__.
+        # We also cannot use isinstance() since we have to compare types.
+        if sys.version_info >= (3, 10) and origin_type is types.UnionType:  # noqa: E721
+            return functools.reduce(operator.or_, resolved_type_args)


this seems like a slightly weird approach, could we use pydantic.typing.convert_generics?

I'll take a closer look at convert_generics, not sure how it fits, but i guess at least it can be recreated with typing._UnionGenericAlias as it's done in convert_generics.

Co-authored-by: ⬢ Samuel Colvin <s@muelcolvin.com>

samuelcolvin · 2023-01-23T17:27:16Z

please update.

samuelcolvin

LGTM, @dmontagu would appreciate if you could confirm you're happy with this as "king of generics".

please review.

dmontagu · 2023-01-31T15:15:20Z

@thenx Looks great.

The only one thing I'd like to request: could we add a test that there are no issues with order-of-arguments caching? My main concern is that since int | str == str | int we might one day refactor the generic cache lookup in a way that breaks things.

Basically just copying this test but using the X | Y syntax:

pydantic/tests/test_generics.py

Lines 718 to 735 in 0bc7cd3

    
           def test_generic_model_caching_detect_order_of_union_args_basic(create_module): 
        
               # Basic variant of https://github.com/pydantic/pydantic/issues/4474 
        
               @create_module 
        
               def module(): 
        
                   from typing import Generic, TypeVar, Union 
        
                   from pydantic.generics import GenericModel 
        
                   t = TypeVar('t') 
        
                   class Model(GenericModel, Generic[t]): 
        
                       data: t 
        
                   int_or_float_model = Model[Union[int, float]] 
        
                   float_or_int_model = Model[Union[float, int]] 
        
                   assert type(int_or_float_model(data='1').data) is int 
        
                   assert type(float_or_int_model(data='1').data) is float

No need to copy the (currently skipped) test below.

samuelcolvin · 2023-01-31T17:12:48Z

LGTM, @thenx the PR is still in draft, if you think it's ready to be merged, please mark it as "ready for review".

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [pydantic](https://togithub.com/pydantic/pydantic) | `==1.10.4` -> `==1.10.5` | [![age](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/compatibility-slim/1.10.4)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/confidence-slim/1.10.4)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>pydantic/pydantic</summary> ### [`v1.10.5`](https://togithub.com/pydantic/pydantic/releases/tag/v1.10.5): (2023-02-15) [Compare Source](https://togithub.com/pydantic/pydantic/compare/v1.10.4...v1.10.5) - Fix broken parametrized bases handling with `GenericModel`s with complex sets of models, [#5052](https://togithub.com/pydantic/pydantic/issues/5052) by [@MarkusSintonen](https://togithub.com/MarkusSintonen) - Invalidate mypy cache if plugin config changes, [#5007](https://togithub.com/pydantic/pydantic/issues/5007) by [@cdce8p](https://togithub.com/cdce8p) - Fix `RecursionError` when deep-copying dataclass types wrapped by pydantic, [#4949](https://togithub.com/pydantic/pydantic/issues/4949) by [@mbillingr](https://togithub.com/mbillingr) - Fix `X | Y` union syntax breaking `GenericModel`, [#4146](https://togithub.com/pydantic/pydantic/issues/4146) by [@thenx](https://togithub.com/thenx) - Switch coverage badge to show coverage for this branch/release, [#5060](https://togithub.com/pydantic/pydantic/issues/5060) by [@samuelcolvin](https://togithub.com/samuelcolvin) #### New Contributors - [@thenx](https://togithub.com/thenx) made their first contribution in [pydantic/pydantic#4977 - [@mbillingr](https://togithub.com/mbillingr) made their first contribution in [pydantic/pydantic#4963 - [@MarkusSintonen](https://togithub.com/MarkusSintonen) made their first contribution in [pydantic/pydantic#5052 **Full Changelog**: pydantic/pydantic@v1.10.4...v1.10.5 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/allenporter/flux-local).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [pydantic](https://togithub.com/pydantic/pydantic) | `==1.10.4` -> `==1.10.5` | [![age](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/compatibility-slim/1.10.4)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/pydantic/1.10.5/confidence-slim/1.10.4)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>pydantic/pydantic</summary> ### [`v1.10.5`](https://togithub.com/pydantic/pydantic/releases/tag/v1.10.5): (2023-02-15) [Compare Source](https://togithub.com/pydantic/pydantic/compare/v1.10.4...v1.10.5) - Fix broken parametrized bases handling with `GenericModel`s with complex sets of models, [#5052](https://togithub.com/pydantic/pydantic/issues/5052) by [@MarkusSintonen](https://togithub.com/MarkusSintonen) - Invalidate mypy cache if plugin config changes, [#5007](https://togithub.com/pydantic/pydantic/issues/5007) by [@cdce8p](https://togithub.com/cdce8p) - Fix `RecursionError` when deep-copying dataclass types wrapped by pydantic, [#4949](https://togithub.com/pydantic/pydantic/issues/4949) by [@mbillingr](https://togithub.com/mbillingr) - Fix `X | Y` union syntax breaking `GenericModel`, [#4146](https://togithub.com/pydantic/pydantic/issues/4146) by [@thenx](https://togithub.com/thenx) - Switch coverage badge to show coverage for this branch/release, [#5060](https://togithub.com/pydantic/pydantic/issues/5060) by [@samuelcolvin](https://togithub.com/samuelcolvin) #### New Contributors - [@thenx](https://togithub.com/thenx) made their first contribution in [pydantic/pydantic#4977 - [@mbillingr](https://togithub.com/mbillingr) made their first contribution in [pydantic/pydantic#4963 - [@MarkusSintonen](https://togithub.com/MarkusSintonen) made their first contribution in [pydantic/pydantic#5052 **Full Changelog**: pydantic/pydantic@v1.10.4...v1.10.5 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/allenporter/pyrainbird).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Fix X | Y union syntax breaks GenericModel (#4146)

8a84227

pydantic-hooky bot added the ready for review label Jan 23, 2023

pydantic-hooky bot assigned hramezani, PrettyWood and samuelcolvin Jan 23, 2023

samuelcolvin reviewed Jan 23, 2023

View reviewed changes

Update changes/4146-thenx.md

c9602f6

Co-authored-by: ⬢ Samuel Colvin <s@muelcolvin.com>

pydantic-hooky bot added awaiting author revision and removed ready for review labels Jan 23, 2023

pydantic-hooky bot assigned thenx and unassigned hramezani, samuelcolvin and PrettyWood Jan 23, 2023

Improve tests

ae61d5a

thenx marked this pull request as draft January 30, 2023 23:23

Recreate newstyle union via typing._UnionGenericAlias

bbe9f8b

samuelcolvin approved these changes Jan 31, 2023

View reviewed changes

pydantic-hooky bot added ready for review and removed awaiting author revision labels Jan 31, 2023

pydantic-hooky bot assigned hramezani, PrettyWood and samuelcolvin and unassigned thenx Jan 31, 2023

samuelcolvin assigned dmontagu and unassigned hramezani, samuelcolvin and PrettyWood Jan 31, 2023

Add basic pep-604 union args order caching detection test

2060efb

thenx marked this pull request as ready for review January 31, 2023 17:27

yezz123 approved these changes Feb 4, 2023

View reviewed changes

dmontagu merged commit a699707 into pydantic:1.10.X-fixes Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle X | Y union in GenericModel #4977

Handle X | Y union in GenericModel #4977

thenx commented Jan 23, 2023 •

edited

thenx commented Jan 23, 2023

samuelcolvin left a comment

samuelcolvin Jan 23, 2023

samuelcolvin Jan 23, 2023

thenx Jan 23, 2023

samuelcolvin Jan 23, 2023

samuelcolvin Jan 23, 2023

thenx Jan 23, 2023 •

edited

samuelcolvin commented Jan 23, 2023

samuelcolvin left a comment

dmontagu commented Jan 31, 2023

samuelcolvin commented Jan 31, 2023

Handle X | Y union in GenericModel #4977

Handle X | Y union in GenericModel #4977

Conversation

thenx commented Jan 23, 2023 • edited

Change Summary

Related issue number

Peculiarities

Checklist

thenx commented Jan 23, 2023

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin Jan 23, 2023

Choose a reason for hiding this comment

samuelcolvin Jan 23, 2023

Choose a reason for hiding this comment

thenx Jan 23, 2023

Choose a reason for hiding this comment

samuelcolvin Jan 23, 2023

Choose a reason for hiding this comment

samuelcolvin Jan 23, 2023

Choose a reason for hiding this comment

thenx Jan 23, 2023 • edited

Choose a reason for hiding this comment

samuelcolvin commented Jan 23, 2023

samuelcolvin left a comment

Choose a reason for hiding this comment

dmontagu commented Jan 31, 2023

samuelcolvin commented Jan 31, 2023

thenx commented Jan 23, 2023 •

edited

thenx Jan 23, 2023 •

edited