Fix mro of generic subclass
#10100
Fix mro of generic subclass
#10100
Conversation
CodSpeed Performance ReportMerging #10100 will not alter performanceComparing Summary
|
Coverage report
This PR does not seem to contain any modification to coverable code. |
|
Hmm, I think the way to solve this is actually going to be revalidating instances of classes that have a shared generic parent. Happy to think about this proposal more tomorrow. @dmontagu, I haven't looked in depth, but this is up your alley, and might be worth a glance. |
There was a problem hiding this comment.
So I think this may make sense in principle, but if we merge it I think we should try to do a better job of substituting the parameters in intermediate classes even when they aren’t the same.
E.g., if I have class A(BaseModel, Generic[T]):… and class B(A[T], Generic[S]): … we should have A[int] in the mro of B[int, str]. I don’t think it should be too hard to achieve this (or at least do a better job than exact equality checking), but if it is unreasonably hard to follow through the substitutions we don’t need to let perfect get in the way of better.
That said, I am not 100% sure if this is better than revalidating into the other generic type. But I think this will result in better behavior in the cases that are affected, so I’m inclined to merge this approach.
|
@dmontagu Thanks for the review! class A(BaseModel, Generic[T1]): ...
class B1(A[T1], Generic[T1, T2]): ...
assert A[int] in B1[T1, str][int].__mro__
assert A[int] in B1[int, str].__mro__Please let me know if there are still some cases I didn't think through. |
| # class B(A[int], Generic[T]): ... | ||
| # class C(B[str], Generic[T]): ... | ||
| # | ||
| key = '__pydantic_inserted_mro_origins__' |
There was a problem hiding this comment.
This __pydantic_inserted_mro_origins__ should be added as an annotated attribute on the BaseModel class if we are going to use it like this. I think it would also be reasonable to add it as a field in the __pydantic_generic_metadata__ if that makes sense, but if we are using a new attribute in BaseModel it should be added near this area:
Line 141 in ae77703
There was a problem hiding this comment.
See https://github.com/pydantic/pydantic/pull/10100/files#r1721798498; once that is addressed I'm good with this PR.
|
@dmontagu I found that my current solution cannot handle this (very unlikely) case: class A(BaseModel, Generic[T1]): ...
class B(A[T2], Generic[T2]): ...
class C(B[T3], Generic[T3]): ...
class D(C[T1], Generic[T1]): ...
class E(D[T2], Generic[T2]):...because the MRO of and the repeated Do you think this is worth fixing, or we could mark it as an expected fail and address it in other PR? |
|
I'd say it's at least worth adding an xfailing test, I don't think it's the craziest case but I also don't think getting that right needs to prevent releasing a partial fix that fixes cases that are much more likely to be encountered. |
|
@kc0506, if you're able to finish this up in the next couple of days, we can get this into our v2.9 release! Thanks for your awesome work here. |
Sure, I will try to wrap it up by this weekend. |
|
I've updated the logic insdie I think this is ready to be merged. Looking forward to any feedbacks! |
|
Awesome work here. I'm going to do another review here as well this afternoon, then we'll merge this after our v2.9 release (we've already released the beta, so can't introduce any major changes during this beta trial period). |
Change Summary
Override
ModelMetaclass.mrosuch that indexed generic type can be inserted into__mro__of its subclasses.For example:
This allow
C()to be assigned toA[bool].I have checked all testcases in
test_generic.py. This change doesn't break existing tests. Also, I collected all the tests where__mro__is affected by this change, and add them into new tests.Related issue number
fix #10039
Checklist