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

Warn when submodule attributes and submodule names conflict #8

Closed

Conversation

stefanv
Copy link
Member

@stefanv stefanv commented May 16, 2022

Otherwise, it can be hard to predict what will be returned on
attribute access (probably the submodule).

This is in response to huggingface/huggingface_hub#874 (comment) on huggingface/huggingface_hub#874

stefanv added 2 commits May 16, 2022 16:49
Otherwise, it can be hard to predict what will be returned on
attribute access (probably the submodule).
@adrinjalali
Copy link
Contributor

adrinjalali commented May 17, 2022

I agree with the warning, but shouldn't we also make sure the behavior here is the same as Python's non-lazy-loaded behavior? I agree the behavior is very unclear, but we probably want to not break people's code if they depend on Python's existing behavior ;)

@TortoiseHam
Copy link

For the record this did catch my project by surprise. We ended up having to rename many of our files to start with underscores to avoid the name conflicts

@stefanv
Copy link
Member Author

stefanv commented May 17, 2022

@adrinjalali @TortoiseHam I agree that it would be ideal to avoid a change in behavior. I tried a fix with sys.modules yesterday, but failed. I'll poke at it some more, but please let me know if you have ideas.

@stefanv
Copy link
Member Author

stefanv commented May 17, 2022

Here's the problem:

If an attribute is not found on a module object through the normal lookup (i.e. object.getattribute), then getattr is searched in the module dict before raising an AttributeError.

__getattribute__ returns the module, __getattr__ returns the function.

@stefanv
Copy link
Member Author

stefanv commented May 17, 2022

See #9 for a different solution. I'd need some testing on that one, please!

@stefanv stefanv closed this May 17, 2022
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.

3 participants