-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-117182: Allow lazily loaded modules to modify their own __class__ #117185
Conversation
b272bf5
to
393bb7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@effigies thank you!
I agree with the changes, and the implementation looks good to me.
Regarding backporting, I think it'd be better to just merge this into main as an improvement, without backporting it to older versions. I don't feel strongly about this though, so if another dev thinks it makes sense to backport, that'd be fine by me.
Btw, I was forced to rename as the commit message was too big. |
Hey, I think this issue is affecting our codebase (https://github.com/pulumi/pulumi, specifically its Python SDK), which makes use of lazy loading modules that modify their own If not, is there a workaround at all? I've posted what I think is a minimal reproduction for our use case which I've tested with 3.8.19 ✔️, 3.9,19 ✔️, 3.10.14 ✔️, 3.11.8 ✔️, 3.11.9 🔴, and 3.12.3 🔴 at https://github.com/lunaris/python-lazy-loading-repro if that's useful. Thanks for any support and thoughts! |
Ah, that reproduction demonstrates that it's a regression introduced by #114781, which makes sense. We deferred resetting the module class until after load, to prevent races, but that would override subclassed module types where we did not previously. @FFY00 I think it would be worth back-porting this. |
@lunaris If it doesn't get backported, you can simply copy the |
@effigies Thanks for this -- really useful to know! |
This PR resolves #117182. The meat of the PR is to check whether the module modified its own
__class__
during load, and only set the default class if not.I also realized that the
LazyLoader
saves the original__class__
in theloader_state
dict. I haven't found a case where that__class__
isn'tModuleType
, but if it ever does occur, it does make sense to restore that type. And if that type for some reason overrides__getattribute__
, it would make sense to use that instead ofobject.__getattribute__
.