Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove use_auth_token argument in favor of token everywhere #1094

Closed
Wauplin opened this issue Sep 30, 2022 · 6 comments · Fixed by #1122
Closed

Remove use_auth_token argument in favor of token everywhere #1094

Wauplin opened this issue Sep 30, 2022 · 6 comments · Fixed by #1122
Assignees
Milestone

Comments

@Wauplin
Copy link
Contributor

Wauplin commented Sep 30, 2022

This topic is directly related to the discussion in #928 and the change of strategy from #1064.

For what I read in #928, everyone agreed that it would be good to have a unique naming for the token (or use_auth_token) argument. The idea was to move everything to token to make things easy for devs. The case token=False would be semantically weird but still acceptable. All of this was before @julien-c highlighted the fact that use_auth_token was used a lot in downstream libraries and that changing it just for a naming problem not worth it.

The decision has been to say "all methods that require only a read access to the Hub would use use_auth_token while write actions will use token". I still think that this rule is too implicit for devs that do not really know when to use use_auth_token or token without looking at the documentation. What do you think to move all write-action methods to use use_auth_token parameter ? If a user uses use_auth_token=False on a write-action it will fail straight away with an explicit message "hey, the token is required here" (+I doubt this will happen very often anyway).

On the downside, this means a deprecation cycle and to adapt downstream libraries to this naming. On the positive side, we will never have to discuss again about this token vs use_auth_token thingy + it will make life easier for anyone using hfh as a single parameter name will be used everywhere.

WDYT ? (ping @julien-c @LysandreJik @osanseviero @adrinjalali @lhoestq @SBrandeis who were opinionated on this subject)

(I feel that I'm a bit reopening a closed topic but I still feel quite uneasy with the current rule -especially since we refactored how tokens are passed-. If you think it's too unnecessary to talk about this again, just tell me and I'll close it for good)

EDIT: changed the PR title to exactly the opposite. Let's go for having token everywhere !

@lhoestq
Copy link
Member

lhoestq commented Sep 30, 2022

Sounds good to me - thinking about it again I agree the current situation can be confusing sometimes

@adrinjalali
Copy link
Contributor

I think a better design would be for HfApi to take token as a constructor argument and use it in its methods, and deprecate accepting token as an arg in all those methods.

@Wauplin
Copy link
Contributor Author

Wauplin commented Sep 30, 2022

I think a better design would be for HfApi to take token as a constructor argument

I'm actually also in favor of something like this. Easier to use and test.

deprecate accepting token as an arg in all those methods.

I'm more worried about the huge number of changes that this would mean for downstream libraries.

@adrinjalali
Copy link
Contributor

I'm more worried about the huge number of changes that this would mean for downstream libraries.

We could leave things as is, and have a longer deprecation cycle for removing them? More like, if token is given to a method, it overwrites the one from self.

@julien-c
Copy link
Member

I agree with moving to a single param name.

I think #1064 changed the situation quite a bit and I would be supportive of aligning everything to token= everywhere.

Maybe we can do a longer deprecation cycle than usual (e.g. 4-5 months) for use_auth_token i.e. first, transparently support both param names (for a few months) using a decorator, then doing our regular deprecation.

@Wauplin
Copy link
Contributor Author

Wauplin commented Oct 10, 2022

I kinda like @julien-c's idea to have a longer deprecation cycle, first without warning for a few month and then with a regular one.

As of the suggestion of having a token attribute in HfApi, I created a separate issue for that: #1104.

@Wauplin Wauplin added this to the v0.11 milestone Oct 10, 2022
@Wauplin Wauplin changed the title Remove token argument in favor of use_auth_token everywhere Remove use_auth_token argument in favor of token everywhere Oct 13, 2022
@Wauplin Wauplin self-assigned this Oct 13, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants