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

Replace use_auth_token arg by token everywhere #1122

Merged
merged 21 commits into from
Oct 26, 2022

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Oct 19, 2022

Fix #1094 (see discussion there).

This is step 1 on the long-term plan to have a single token argument (either Optional[str] or Optional[Union[bool, str]]) instead of either token or use_auth_token depending on the use case. Since use_auth_token is still widely used, we are only smoothly deprecating it by accepting it without warning while having token as the preferred (and documented) argument. After a few months and adaptations in downstream libraries, we will start a proper deprecation cycle for use_auth_token. At the moment, all the magic is implemented in validate_hf_hub_args. I hope this will not be too misleading for users.

I have written the step by step plan in the smoothly_deprecate_use_auth_token method. I would be good to have a look at the generated documentation so that we are all aligned.

This should finally be the last piece of work on tokens in hugginface_hub after #1064 (always send token when logged in), #1084 (send token on LFS upload), #1111 (unified login() method) and #1116 (add token to HfApi(token=...). Hope that authentication will now be as smooth as possible for all users. (EDIT: I forgot #1051 about git credential store to change...).

cc @adrinjalali @lhoestq @sgugger @julien-c @LysandreJik @osanseviero who contributed to the discussion on this topic.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 20, 2022

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

@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Base: 84.46% // Head: 84.54% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (fe2eaaa) compared to base (720ea11).
Patch coverage: 97.18% of modified lines in pull request are covered.

❗ Current head fe2eaaa differs from pull request most recent head e30656d. Consider uploading reports for the commit e30656d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1122      +/-   ##
==========================================
+ Coverage   84.46%   84.54%   +0.08%     
==========================================
  Files          41       41              
  Lines        4074     4089      +15     
==========================================
+ Hits         3441     3457      +16     
+ Misses        633      632       -1     
Impacted Files Coverage Δ
src/huggingface_hub/_snapshot_download.py 96.07% <ø> (ø)
src/huggingface_hub/repocard.py 93.78% <ø> (ø)
src/huggingface_hub/hf_api.py 87.67% <91.66%> (-0.03%) ⬇️
src/huggingface_hub/_commit_api.py 92.85% <100.00%> (ø)
src/huggingface_hub/file_download.py 87.87% <100.00%> (+0.14%) ⬆️
src/huggingface_hub/hub_mixin.py 85.58% <100.00%> (+0.13%) ⬆️
src/huggingface_hub/inference_api.py 88.88% <100.00%> (+0.31%) ⬆️
src/huggingface_hub/keras_mixin.py 89.47% <100.00%> (ø)
src/huggingface_hub/repository.py 79.44% <100.00%> (+0.07%) ⬆️
src/huggingface_hub/utils/__init__.py 100.00% <100.00%> (ø)
... and 3 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Wauplin Wauplin marked this pull request as ready for review October 21, 2022 08:39
@Wauplin Wauplin changed the title [draft] Replace use_auth_token arg by token everywhere Replace use_auth_token arg by token everywhere Oct 21, 2022
Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Nice thanks ! :)

src/huggingface_hub/utils/_validators.py Show resolved Hide resolved
src/huggingface_hub/utils/_validators.py Show resolved Hide resolved
Copy link
Contributor

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

Very much like this one!

@Wauplin Wauplin merged commit aa165c9 into main Oct 26, 2022
@Wauplin Wauplin deleted the 1094-remove-use-auth-token-from-everywhere branch October 26, 2022 14:52
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.

Remove use_auth_token argument in favor of token everywhere
5 participants