Skip to content

Conversation

effigies
Copy link
Contributor

@effigies effigies commented Mar 23, 2024

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 the loader_state dict. I haven't found a case where that __class__ isn't ModuleType, 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 of object.__getattribute__.

Copy link
Member

@FFY00 FFY00 left a 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.

@FFY00 FFY00 changed the title gh-117182: Allow modules to modify their own __class__ when lazily loaded gh-117182: Allow lazily loaded modules to modify their own __class__ Apr 9, 2024
@FFY00 FFY00 merged commit 19a2202 into python:main Apr 9, 2024
@FFY00
Copy link
Member

FFY00 commented Apr 9, 2024

Btw, I was forced to rename as the commit message was too big.

@effigies effigies deleted the fix-issue-117182 branch April 9, 2024 10:14
@lunaris
Copy link

lunaris commented Apr 17, 2024

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 __class__. Responding to the comments above, would it be possible to backport this to 3.11/3.12/any affected versions? Presently Pulumi's Python SDK won't work for our users if they are on 3.11.9 or 3.12 due to this.

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!

@effigies
Copy link
Contributor Author

effigies commented Apr 17, 2024

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.

@effigies
Copy link
Contributor Author

@lunaris If it doesn't get backported, you can simply copy the LazyLoader and _LazyModule classes and use those instead of importlib.util.LazyLoader. There's nothing magic about the stdlib ones.

@lunaris
Copy link

lunaris commented Apr 26, 2024

@effigies Thanks for this -- really useful to know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants