Improve generic subclass support #2549

diabolo-dan · 2021-03-19T16:35:44Z

Change Summary

Add parameterised subclasses to __bases__ when constructing new parameterised classes, so that A <: B => A[int] <: B[int].

This helps to facilitate a use case based on abstract classes, and leads to less unexpected behaviour when comparing between concrete versions of classes.

Related issue number

fix #2007

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)

codecov · 2021-03-19T16:37:54Z

Codecov Report

Merging #2549 (26cba97) into master (7b7e705) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 26cba97 differs from pull request most recent head 193ee06. Consider uploading reports for the commit 193ee06 to get more accurate results

@@            Coverage Diff            @@
##            master     #2549   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5133   +24     
  Branches      1050      1059    +9     
=========================================
+ Hits          5109      5133   +24

Impacted Files	Coverage Δ
pydantic/generics.py	`100.00% <100.00%> (ø)`
pydantic/main.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a35cde9...193ee06. Read the comment docs.

tests/test_generics.py

diabolo-dan · 2021-03-22T12:51:56Z

test fastAPI is failling with:
ImportError: cannot import name 'RowProxy' from 'sqlalchemy.engine.result' (/opt/hostedtoolcache/Python/3.7.10/x64/lib/python3.7/site-packages/sqlalchemy/engine/result.py

As far as I can tell that's not anything I could've incidentally broken. I'm not sure what the best way to proceed with it would be?

PrettyWood · 2021-03-22T13:32:20Z

@diabolo-dan Don't worry you can have a rest for the CI! ;)
It's due to fastapi tests that uses databases without pinning sqlalchemy dependency.
I already submitted a fix encode/databases#299 for that

pydantic/generics.py

davidhyman

Some minor thoughts

pydantic/generics.py

tests/test_generics.py

pydantic/generics.py

diabolo-dan · 2021-03-24T09:46:15Z

I notice that there was significant generic implementation done by @dmontagu , including the line:

    assert concrete_model.__name__ == 'Model[int, BT][str]'

Which is I believe the only concrete claim about the behaviour of reapplication of partially parameterised generic models. (namely extending from the former).

This behaviour adds complexity to establishing parameterised bases, and makes for less (because a concrete type could be constructed through arbitrary many type variables). I also think it's less intuitive than always building from the base, (so that Model[int, BT][str] is Model[int, str]). The alternative isn't perfect, because there's still the issue of checking issubclass(Model[int, BT][str], Model[int, AT]) which for various assumptions on the TypeVars, should ideally hold, but I don't think it's possible to achieve that (at least through setting __bases__).

I'm just wondering if there was a particular reason for implementing it this way, or if it was just the simplest thing that met the requirements at the time.

samuelcolvin

Honestly, generics.py scares the hell out of me. I don't pretend to remember/understand everything in there and I barely ever (never?) use generics myself.

The only consolation is that this isn't part of core validation, just isinstance checks. (I think?)

I've got a few notes and questions, but overall, and from what I can see it looks reasonable.

pydantic/generics.py

samuelcolvin · 2021-05-09T11:16:42Z

pydantic/generics.py

+            elif cls in _assigned_parameters and base_model in _assigned_parameters:
+                # cls is partially parameterised but not from base_model
+                # e.g. cls = B[S], base_model = A[S]
+                # B[S][int] should subclass A[int],  (and will be transitively via B[int])
+                # but it's not viable to consistently subclass types with arbitrary construction
+                # So don't attempt to include A[S][int]
+                continue
+            elif cls in _assigned_parameters and base_model not in _assigned_parameters:
+                # cls is partially parameterized, base_model is original generic
+                # e.g.  cls = B[str, T], base_model = B[S, T]
+                # Need to determine the mapping for the base_model parameters
+                mapped_types: Parametrization = {
+                    key: typevars_map.get(value, value) for key, value in _assigned_parameters[cls].items()
+                }
+                yield from build_base_model(base_model, mapped_types)


can't these two cases be covered by

elif cls in _assigned_parameters: if base_model in _assigned_parameters: ... else: ...

?

My thinking was that the 2 cases were in some sense only incidentally related, so it helped the clarity to have them as top level cases.

I've moved them back to being under a common _assigned_parameters case.

pydantic/generics.py

samuelcolvin · 2021-05-09T11:20:40Z

please update.

The type was incorrectly being picked up for this style of subclassing, and it can be regardless inferred through cls.

* Improve parameterisation explanation * fix typos * Alias Parameterisation type

* start docstring with newline. * Use None as default over empty tuple. Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

…rics

diabolo-dan · 2021-05-17T15:14:01Z

please review.

samuelcolvin · 2021-12-05T13:40:32Z

great, thank you.

* Derive concrete subclasses for parameterised generics * Resolve type issues * Add negative assertions to generic subclass tests * Remove incorrect subclassing of partial. The type was incorrectly being picked up for this style of subclassing, and it can be regardless inferred through cls. * Apply feedback: * Improve parameterisation explanation * fix typos * Alias Parameterisation type * Apply suggestions from code review * start docstring with newline. * Use None as default over empty tuple. Co-authored-by: Samuel Colvin <samcolvin@gmail.com> * Combine _assigned_parameters cases in __paramaterized_bases__ of generics * Add description for the `_assigned_parameters` variable. Co-authored-by: Samuel Colvin <samcolvin@gmail.com> Co-authored-by: Samuel Colvin <s@muelcolvin.com>

samuelcolvin · 2022-05-18T11:23:08Z

Just a note for anyone watching this PR, this changes here I think have caused a memory build up, see #3829.

To mitigate this, I've implemented (and will soon merge) #4083 which limits the size of _generic_types_cache and _assigned_parameters_limit to 1000. These configured by settings generics._generic_types_cache_limit and generics._assigned_parameters_limit.

Feedback on the PR welcome, but in order to include this fix in v1.9.1 I won't being waiting for feedback before merging - any further changes would need to be in another PR.

diabolo-dan mentioned this pull request Mar 19, 2021

GenericModel - Recursive abstract class fails validation #2007

Closed

3 tasks

diabolo-dan force-pushed the improve_generic_subclass_support branch 3 times, most recently from 6ef9a8d to b731c84 Compare March 19, 2021 17:07

choogeboom reviewed Mar 19, 2021

View reviewed changes

tests/test_generics.py Show resolved Hide resolved

diabolo-dan force-pushed the improve_generic_subclass_support branch 2 times, most recently from adb7c8e to 2d690f9 Compare March 22, 2021 13:30

diabolo-dan commented Mar 23, 2021

View reviewed changes

pydantic/generics.py Outdated Show resolved Hide resolved

davidhyman reviewed Mar 23, 2021

View reviewed changes

diabolo-dan force-pushed the improve_generic_subclass_support branch 2 times, most recently from 050b132 to 26cba97 Compare March 23, 2021 20:39

diabolo-dan force-pushed the improve_generic_subclass_support branch 2 times, most recently from 0bc3564 to 39d5566 Compare April 1, 2021 10:05

samuelcolvin reviewed May 9, 2021

View reviewed changes

github-actions bot added the awaiting author revision label May 9, 2021

github-actions bot assigned diabolo-dan May 9, 2021

diabolo-dan and others added 8 commits May 17, 2021 16:00

Derive concrete subclasses for parameterised generics

181b912

Resolve type issues

a121bb7

Add negative assertions to generic subclass tests

3c7ffed

Remove incorrect subclassing of partial.

ca49da7

The type was incorrectly being picked up for this style of subclassing, and it can be regardless inferred through cls.

Apply feedback:

2677824

* Improve parameterisation explanation * fix typos * Alias Parameterisation type

Apply suggestions from code review

358efb5

* start docstring with newline. * Use None as default over empty tuple. Co-authored-by: Samuel Colvin <samcolvin@gmail.com>

Combine _assigned_parameters cases in __paramaterized_bases__ of gene…

5505ec4

…rics

Add description for the _assigned_parameters variable.

b4a8841

diabolo-dan force-pushed the improve_generic_subclass_support branch from 9f1d7ab to b4a8841 Compare May 17, 2021 15:00

diabolo-dan requested a review from samuelcolvin May 17, 2021 15:06

diabolo-dan removed their assignment May 17, 2021

github-actions bot added ready for review and removed awaiting author revision labels May 17, 2021

github-actions bot assigned PrettyWood and samuelcolvin May 17, 2021

Merge branch 'master' into diabolo-dan-improve_generic_subclass_support

193ee06

samuelcolvin merged commit e71f53d into pydantic:master Dec 5, 2021

samuelcolvin mentioned this pull request May 14, 2022

Memory leak after updating to Pydantic v1.9.0 #3829

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve generic subclass support #2549

Improve generic subclass support #2549

diabolo-dan commented Mar 19, 2021 •

edited by samuelcolvin

codecov bot commented Mar 19, 2021 •

edited

diabolo-dan commented Mar 22, 2021

PrettyWood commented Mar 22, 2021

davidhyman left a comment

diabolo-dan commented Mar 24, 2021

samuelcolvin left a comment

samuelcolvin May 9, 2021

diabolo-dan May 17, 2021

samuelcolvin commented May 9, 2021

diabolo-dan commented May 17, 2021

samuelcolvin commented Dec 5, 2021

samuelcolvin commented May 18, 2022

Improve generic subclass support #2549

Improve generic subclass support #2549

Conversation

diabolo-dan commented Mar 19, 2021 • edited by samuelcolvin

Change Summary

Related issue number

Checklist

codecov bot commented Mar 19, 2021 • edited

Codecov Report

diabolo-dan commented Mar 22, 2021

PrettyWood commented Mar 22, 2021

davidhyman left a comment

Choose a reason for hiding this comment

diabolo-dan commented Mar 24, 2021

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin May 9, 2021

Choose a reason for hiding this comment

diabolo-dan May 17, 2021

Choose a reason for hiding this comment

samuelcolvin commented May 9, 2021

diabolo-dan commented May 17, 2021

samuelcolvin commented Dec 5, 2021

samuelcolvin commented May 18, 2022

diabolo-dan commented Mar 19, 2021 •

edited by samuelcolvin

codecov bot commented Mar 19, 2021 •

edited