Make token optional in all HfApi methods. #379
Conversation
sgugger
changed the title
There was a problem hiding this comment.
This is a very welcome change which should make many things clearer. I think having the token as first parameter in most methods isn't ideal, as it is generally not the key parameter of the method.
Ont nit, however: would it be possible to split the PR in two, with one PR changing the API and the second changing the tests? Modifying both at the same time is a bit dangerous as we try to prevent breaking changes.
@osanseviero, this PR introduces a change in the methods', but Sylvain also introduced mechanisms to be backwards compatible as it would otherwise be very breaking. This will, however, change in a future version with future warnings removed.
Do you know if libraries have an unpinned version of huggingface_hub, and, if so, if they use any of the changed methods here? Namely, create_repo, delete_repo, update_repo_visibility, and upload_file? Thank you.
| if token is None: | ||
| raise ValueError( | ||
| "You need to pass a valid `token` or login by using `huggingface-cli login`" | ||
| ) |
src/huggingface_hub/hf_api.py
Outdated
| def _is_valid_token(self, token: str): | ||
| try: | ||
| self.whoami(token=token) | ||
| return True | ||
| except HTTPError: | ||
| return False |
There was a problem hiding this comment.
Nice helper, would it be possible to add a bit of documentation?
| warnings.warn( | ||
| "`create_repo` now takes `token` as an optional positional argument. " | ||
| "Be sure to adapt your code!", | ||
| FutureWarning, | ||
| ) | ||
| token, name = name, token |
|
Also cc @stevhliu, could you link which documentation pages should be updated to reflect this change? Thanks a lot |
|
I think these are the only two pages that will need to be updated: Let me know if I can help update these :) |
|
I've already updated those to the new API, normally. |
|
Thank you for doing the research @osanseviero! @Narsil this touches to some of the code in api-inference-community, could you take a quick look at the changes? |
There was a problem hiding this comment.
LGTM! As seen with @sgugger and @osanseviero, the only library that uses an unpinned version of huggingface_hub and that would potentially break is sentence-transformers cc @nreimers.
The workarounds in create_repo should prevent any breaking change, however.
This PR makes the
tokenargument of all HfApi methods optional and default to the saved token. As you can see in the updated docs, the result is quite nice in the sense a user never has to retrieve their token (and thus get some chance of leaking it by mistake).Since this changes the order of the argument and is thus a breaking change, some internal magic is implemented to detect if the argument at the old place of the token is the token, when the argument at the place of the token is not a valid token, and then do some switch.