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

Uniformize token retrieval/validation #837

Closed
adrinjalali opened this issue Apr 14, 2022 · 6 comments
Closed

Uniformize token retrieval/validation #837

adrinjalali opened this issue Apr 14, 2022 · 6 comments
Assignees
Labels

Comments

@adrinjalali
Copy link
Contributor

Right now there is HfApi._validate_or_retrieve_token() and HfFolder.get_token() and they're used in different places. We should have a single place where we deal with tokens.

ref: #792 (comment)

cc @merveenoyan maybe?

@merveenoyan
Copy link
Contributor

I can surely refactor and deprecate one of them. Let me check the code and come up with something.

@merveenoyan merveenoyan self-assigned this Apr 14, 2022
@merveenoyan
Copy link
Contributor

merveenoyan commented Apr 25, 2022

@adrinjalali Correct me if I'm wrong.
get_token is called in to check if there's a token, where token doesn't need any validation whatsoever but you just want to prompt user to provide one if there's none in environmental variables.
_validate_or_retrieve_token is called inside functions that execute something that needs auth. I know it's not super intuitive that both exist but they do serve different purpose maybe something like this should work:

  • get token inside _validate_or_retrieve_token (keep as is)
  • call _validate_or_retrieve_token everywhere get_token was called which will raise EnvironmentError as get_token normally would. (where get_token is called I mean)
    Does that work?

@adrinjalali
Copy link
Contributor Author

get token inside _validate_or_retrieve_token

This is what _validate_or_retrieve_token does right now.

Another option is to call a modified _validate_or_retrieve_token inside get_token (or move its content to get_token). We could allow None as a token since many methods don't need to pass a valid token to the server.

This means moving _is_valid_token and _validate_or_retrieve_token outside HfApi() and have them beside get_token in HfFolder I guess.

WDYT?

@merveenoyan
Copy link
Contributor

@adrinjalali no I meant we normally call get_token inside _validate_or_retrieve_token we can just write getting inside validation and call validation everywhere.

OR your suggestion can be having _validate_token (remove the retrieval part) called inside get_token which I'm fond of. I'll move them outside HfApi() and have them beside HfFolder. Thanks!

@adrinjalali
Copy link
Contributor Author

Yep, your latter suggestion is exactly what I was thinking.

@Wauplin
Copy link
Contributor

Wauplin commented Oct 18, 2022

Token retrieval has been refactored in #1064 with in particular a build_hf_headers(...) helper and a low-level _get_token_to_send(...) method. I'm closing this issue as _validate_or_retrieve_token doesn't exist anymore. Also worth mentioning the low-level and private _validate_token_to_send which makes token retrieval and validation separated.

@Wauplin Wauplin closed this as completed Oct 18, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants