🗑 Deprecate token in read-only methods of HfApi in favor of use_auth_token

[SBrandeis]

This PR deprecates the token argument in read-only methods of HfApi in favor of use_auth_token.

use_auth_token is more flexible as it allows not passing a token, whereas token=None fetches the token from HfFolder.

I personally prefer the semantics of use_auth_token, that I find very legible:

• use_auth_token=False
• use_auth_token="token"
• use_auth_token=True

cc @lhoestq

[HuggingFaceDocBuilderDev]

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

[adrinjalali]

I'd say adding token=False semantics is probably a better change. token is quite a crucial parameter and deprecating it would cause quite a bit of friction for downstream libraries. I'd say add the same semantics if you will, but to token.

Related: #837

It would also be nice to let users pass token to a HFApi instance and not persist it: #796

[lhoestq]

+1 for keeping token: the only case the use_auth_token semantic is useful is when passing False, which is definitely not the common case anyway - therefore renaming the parameter is maybe not worth it

[julien-c]

i agree that it would be good to have consistency (and we shouldn't have introduced two different names in the first place)

I do not really mind whether it's called use_auth_token or token, though

[LysandreJik]

Agree with team members above on token rather than use_auth_token :)

[julien-c]

Thinking more on it, I think it's better to align everything to use_auth_token given it's used extensively in both transformers (794 occurences in 78 files) and datasets (198 occurences in 18 files)

I think the benefit of renaming to a "better" param name is too small compared to the breakage in downstream libraries

i.e., I support @SBrandeis's PR here

EDIT following discussion with @LysandreJik: PR is cool because it's for read-only methods. token still makes sense for write-only methods

[LysandreJik]

Across libraries there seems to be a general tendency to have token for methods that will need a token in all cases (for example to push something to the hub), and use_auth_token in case a token might not be necessary.

To that end, if you feel strongly, I'm fine with going forward with this PR as it adds the use_auth_token argument to read only methods. However, in that case, we should go all in and remove the token from the signature and add it to a kwargs, and remove it from the documentation.

[SBrandeis]

cc @Wauplin

[Wauplin]

Coming a bit after the battle but I would also have been for a unique token parameter instead of both token and use_auth_token depending on the use case. Maybe I'm wrong but what I have in mind is that a typical user/dev will not really care about if a server needs a token or not. Worst case scenario, the server needs one and prompt an error to login once.

Quick question: is there a chance that a request that is today "public" become "private" (token required) in the future or the opposite ? If yes, having a single parameter name would avoid to handle a name change in huggingface_hub (+breaking changes, deprecate,...). I can't find an example yet but it doesn't seem that impossible.

In any case, I get the "it is as it is and vastly used already so let's keep both"-argument. It's a pretty strong argument in favor of this PR 😄

EDIT(side note): what I found confusing in the current naming is that token and auth_token suggest that there is 2 types of tokens. At least that's what I first thought when reading the documentation. No big deal.

[Wauplin]

Just an update on this PR. I think there is kind of a consensus on moving forward to merge it. The rule will be "use use_auth_token for read-only methods and token otherwise". It makes sense and shouldn't break too much external libraries.

That said, next steps are:

• merge from main and try to resolve conflicts -> I did it from github
• fix the tests -> I think I broke them with the merge
• deprecate token from v0.12
• expect deprecation in unittest + add new tests with use_auth_token
• make sure CI is fine

@SBrandeis if you'd like I can take care of it. I just need the permissions to push commits on your fork.
Let's get this done :)

[codecov]

Codecov Report

Merging #928 (e878793) into main (59873e7) will increase coverage by 0.05%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##             main     #928      +/-   ##
==========================================
+ Coverage   83.53%   83.58%   +0.05%     
==========================================
  Files          37       37              
  Lines        3918     3930      +12     
==========================================
+ Hits         3273     3285      +12     
  Misses        645      645              

Impacted Files	Coverage Δ
src/huggingface_hub/_snapshot_download.py	92.98% <ø> (ø)
src/huggingface_hub/keras_mixin.py	90.53% <ø> (ø)
src/huggingface_hub/repository.py	80.00% <ø> (ø)
src/huggingface_hub/hf_api.py	87.45% <96.29%> (+0.18%)	⬆️
src/huggingface_hub/inference_api.py	88.57% <100.00%> (ø)

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