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

timm: add __all__ to __init__ #2399

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Conversation

adamjstewart
Copy link
Contributor

This fixes mypy errors on files like:

import timm

timm.create_model("resnet18")

The error can be reproduced using:

> mypy --strict test.py
test.py:3: error: Module "timm" does not explicitly export attribute "create_model"  [attr-defined]
Found 1 error in 1 file (checked 1 source file)

@rwightman
Copy link
Collaborator

Does something like __all__ = [name for name in locals() if not name.startswith('_')] keep mypy happy? because redefining everything in __init__.py in __all__ is extremely redundant and just adds to maint burden.

@adamjstewart
Copy link
Contributor Author

Unfortunately that doesn't work for mypy, flake8, ruff, or any other static style checker as it is dynamically generated.

The maintenance burden depends on how often you update __init__ and whether or not you use tools like mypy/flake8/ruff to enforce it. FWIW, I'm willing to help maintain this myself, and can update all __init__.py files for you if you'll let me add one of the above tools to make sure it stays up to date. I would also be interested in avoiding star imports as well.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@rwightman
Copy link
Collaborator

If doing this, I think rather than use the all approach, equally redundant/pedantic, but less error prone in terms of getting out of sync with futuer changes is the import X as X

from .version import (
    __version__ as __version__,
)

from .layers import (
    is_scriptable as is_scriptable,
    is_exportable as is_exportable,
    set_scriptable as set_scriptable,
    set_exportable as set_exportable,
)

from .models import (
    create_model as create_model,
    list_models as list_models,
    list_pretrained as list_pretrained,
    is_model as is_model,
    list_modules as list_modules,
    model_entrypoint as model_entrypoint,
    is_model_pretrained as is_model_pretrained,
    get_pretrained_cfg as get_pretrained_cfg,
    get_pretrained_cfg_value as get_pretrained_cfg_value,
)

@rwightman
Copy link
Collaborator

probably worth mentioning, while I don't mind improving many aspects of typing, mypy compat... however, I probably will hit a limit where I won't agree with some changes mypy (esp strict) where verbosity, overhead outweights (my) perceived benefits

@adamjstewart
Copy link
Contributor Author

import X as X

Interesting. I've never seen anyone use this hack before, so I'm not sure if it's recommended, but I can confirm that it works. Let me know if you would like me to go with this solution instead.

but less error prone in terms of getting out of sync with futuer changes

If you're concerned about this, any of the above tools can be used to enforce this, so it will never get out of sync.

probably worth mentioning, while I don't mind improving many aspects of typing, mypy compat... however, I probably will hit a limit where I won't agree with some changes mypy (esp strict) where verbosity, overhead outweights (my) perceived benefits

Completely understand. But note that this decision also affects other libraries as a result. In TorchGeo, we now have to add # type: ignore[attr-defined] to every single usage of timm.create_model (there are many) and there is no other possible workaround. Following best practices isn't always easy, but it is usually useful (even if the perceived benefits aren't immediately obvious).

@adamjstewart
Copy link
Contributor Author

@rwightman what are the next steps for this PR?

@rwightman
Copy link
Collaborator

@adamjstewart can proceed with the import x as x pattern as per the example for this specific __init__.py ... it's the route that PyTorch itself went to to addressing this concern and I also feel it's a bit better than maintining more sets of __all__

@adamjstewart
Copy link
Contributor Author

it's the route that PyTorch itself went

Where do you see this? I see __all__ in https://github.com/pytorch/pytorch/blob/main/torch/__init__.py

@rwightman
Copy link
Collaborator

@adamjstewart I guess they did it inconsistently https://github.com/pytorch/pytorch/blob/main/torch/nn/__init__.py

@rwightman rwightman merged commit d81da93 into huggingface:main Jan 22, 2025
22 checks passed
@adamjstewart adamjstewart deleted the types/__init__ branch January 22, 2025 20:02
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