CodSpeed Performance Report

Merging#10100 willnot alter performance

_{Comparingkc0506:fix-generic-mro (dc7cd94) withmain (287c266)}

Summary

✅ 49 untouched benchmarks

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.

@dmontagu Thanks for the review!
I made some changes, so now this can work:

classA(BaseModel,Generic[T1]): ...classB1(A[T1],Generic[T1,T2]): ...assertA[int]inB1[T1,str][int].__mro__assertA[int]inB1[int,str].__mro__

Please let me know if there are still some cases I didn't think through.

@dmontagu I found that my current solution cannot handle this (very unlikely) case:

classA(BaseModel,Generic[T1]): ...classB(A[T2],Generic[T2]): ...classC(B[T3],Generic[T3]): ...classD(C[T1],Generic[T1]): ...classE(D[T2],Generic[T2]):...

because the MRO ofD[T2] will be like

D[~T2], D,C[~T2], C[~T1], C,B[~T2], B[~T1], B[~T3], B,A[~T2], A[~T1], A[~T3], A[~T2], A, ...

and the repeatedA[T2] is invalid.

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.

@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 insdiemro. The new approach does not need an extra class variable and can solve the failed case mentioned above, as well as remove redundant bases (e.g.A[int], A[T2], A[T1], A, ... will now become simplyA[int], A, ...).

I think this is ready to be merged. Looking forward to any feedbacks!

@kc0506,

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).