Skip to content
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

Remove Module.__getattr__ #2359

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Remove Module.__getattr__ #2359

merged 1 commit into from
Jun 2, 2023

Conversation

saitcakmak
Copy link
Collaborator

GPyTorch Module class defines a custom __getattr__ method that used to serve a purpose, but now only calls super().__getattr__. This leads to deepening of stack trace and can cause mild confusion. Since it is a no-op, removing it seems like the right move to me.

Aside: First calling __getattr__ then falling back to __getattribute__ is a bit strange on its own. Python first calls __getattribute__ and if an attribute is not found __getattr__ is invoked, in case there's some custom logic. Looking at the changelog, it seems like __getattribute__ was called using some base_name when this wasn't a no-op, so it's probably a relic from those times.

@saitcakmak saitcakmak requested a review from Balandat June 2, 2023 18:28
Copy link
Member

@gpleiss gpleiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine!

@dme65 dme65 merged commit 6cfea77 into master Jun 2, 2023
@saitcakmak saitcakmak deleted the saitcakmak-patch-1 branch June 2, 2023 18:38
@Balandat
Copy link
Collaborator

Balandat commented Jun 2, 2023

Not sure this is relevant, just pointing out that pytorch overrides __getattr__ in a way that can sometimes cause unintiutive behavior: https://github.com/pytorch/pytorch/blob/main/torch/nn/modules/module.py#L1617-L1631. But it seems here this is indeed a noop.

@saitcakmak
Copy link
Collaborator Author

Yeah, PyTorch overwrites both __getattr__ and __setattr__. There's a known bug where some properties can effectively become inaccessible but fixing it leads to backwards-incompatible changes as I've found out (fix got reverted, the bug is still there).

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

Successfully merging this pull request may close these issues.

4 participants