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

Add autocomplete + tests + type checking #1041

Merged
merged 13 commits into from
Sep 12, 2022
Merged

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Sep 8, 2022

Fix #1036.

This PR adds static imports in huggingface_hub.__init__.py file. This allows IDEs to have autocomplete capabilities when importing stuff from huggingface_hub:

from huggingface_hub import c # <- "create_commit" suggested here

create_commit( # <- docstring + args help suggested here

To allow this, explicit imports have to be made under the if TYPE_CHECKING section which means newly added modules/functions have to be duplicated in the file (once for lazy load, once for static part). Suggested by @sgugger in #1036 (comment). I didn't implement it exactly the same way as transformers as we don't need the same level of detail/technicality.

In order to mitigate discrepancies, I added a test that checks all modules are declared twice. When adding a new module in the lazy-loading part, one need to run the following command to generate the static part and then commit the changes:

pytest tests/test_init_lazy_loading.py -k test_static_imports --update-init-file

Finally, this PR adds mypy type checking in the CI for the root __init__.py file. This is to check that we don't have a module exposed that doesn't exist (we previously had from .hf_api import login, logout but login and logout apparently doesn't exist anymore).

EDIT: also added a part in the test to ensure _SUBMOD_ATTRS definition is alphabetically ordered and rewrite it if it's not the case. Doesn't change much things but I tend to prefer (easier to read it/easier to look for something in it).

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 8, 2022

The documentation is not available anymore as the PR was closed or merged.

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #1041 (7b7d1c5) into main (b9d8617) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1041      +/-   ##
==========================================
- Coverage   83.69%   83.67%   -0.02%     
==========================================
  Files          37       37              
  Lines        3919     3915       -4     
==========================================
- Hits         3280     3276       -4     
  Misses        639      639              
Impacted Files Coverage Δ
src/huggingface_hub/__init__.py 75.75% <100.00%> (-2.63%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, it should make life easier for everyone!
I like the automated completion but IMO it shouldn't be done via pytest but via make style (will add the new lines in init) and make quality (will check that the init is properly written).

Also very very very much not a fan of mypy. I would have personally written a custom script to do the same check, but to each their own.

setup.py Outdated Show resolved Hide resolved
tests/test_init_lazy_loading.py Outdated Show resolved Hide resolved
@Wauplin
Copy link
Contributor Author

Wauplin commented Sep 8, 2022

Also very very very much not a fan of mypy. I would have personally written a custom script to do the same check, but to each their own.

@sgugger Out of curiosity, what would be the advantage of having a custom script instead of using mypy here ? Some import stuff is not always trivial so I thought relying mypy would save some hassle to check they are all legit.

Just to mention that in general, I am in favor of running mypy on the full codebase to ensure we don't have stupid mistakes messing around. Not highest prio but more on a mid-term perspective.

@sgugger
Copy link
Contributor

sgugger commented Sep 8, 2022

In my experience, mypy is the one making the stupid mistakes as soon as you have some complex types ;-)

@Wauplin
Copy link
Contributor Author

Wauplin commented Sep 9, 2022

I like the automated completion but IMO it shouldn't be done via pytest but via make style (will add the new lines in init) and make quality (will check that the init is properly written).

@sgugger I made the requested change to have it in a separate folder utils and add the command to make style and make quality. It follows more the transformers pattern like this.

For mypy I'll keep it as it is for now as it is working fine.

Copy link
Contributor

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this, LGTM!

.github/workflows/python-quality.yml Show resolved Hide resolved
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Ok, looks good to me! Also not super in favor of mypy, but if it doesn't require any deep changes here it's fine for me to keep it as is. Let's just remember to stay reasonable when it comes to typing :)

.github/workflows/python-quality.yml Show resolved Hide resolved
tests/test_init_lazy_loading.py Show resolved Hide resolved
@Wauplin
Copy link
Contributor Author

Wauplin commented Sep 12, 2022

Thanks @sgugger @LysandreJik for the reviews. I'm merging.

@Wauplin Wauplin merged commit be0e648 into main Sep 12, 2022
@Wauplin Wauplin deleted the 1036-autocomplete-in-ide branch September 12, 2022 16:12
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.

Handle autocomplete in IDE
5 participants