-
Notifications
You must be signed in to change notification settings - Fork 867
Add optional cache to whoami
#3568
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9fa9495
Add optional cache to whoami
Wauplin 7938ecb
Update src/huggingface_hub/hf_api.py
Wauplin 33a5ebd
Update src/huggingface_hub/hf_api.py
Wauplin 0de490b
Fix token=False not working
Wauplin e3471ff
add test for token=False
Wauplin 635943b
Merge branch 'add-caching-to-whoami' of github.com:huggingface/huggin…
Wauplin 8463f98
Update src/huggingface_hub/hf_api.py
Wauplin fcf7063
Update src/huggingface_hub/hf_api.py
Wauplin 957f909
code quality
Wauplin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change in behavior, right? Previously, if
token=False, we could still get the token fromget_token(), but now we raise an exception?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since
token=Falseexplicitly means "do not retrieve the token locally". The breaking change is that previously it would raise an HTTP 401 unauthorized issue, and now aValueError. I don't believe though that it's better like this to fail early.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I had to step away earlier but to clarify, I do agree that this change is the way to go and is inline with the docstring. But what I was saying was that previously, you would actually use the locally-saved token, even if a user passed
token=False, becausetoken or self.token or get_token()would keep going until a truthy value was found, so the logic would still fall back to a local token viaget_token().Unless I'm misunderstanding something, this would be a breaking change -- although I do agree that this is the right way to go. In fact, maybe a nit but I'd say we go even farther, and replace
token = self.tokenwithtoken = self.token if token is None else token, because again, this would mean that if a user doeswhoami(token=False), but has atokensaved to the class would end up using that token, which is contrary to the docstring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^agree that we should do
token = self.token if token is None else tokensincetokencan beFalse(as we do in_build_hf_headers).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow yes, agree with the above. Thanks for flagging!
I've fixed it in 0de490b and added a regression test in e3471ff. I would not consider this as a breaking change but as a bug fix 😄