-
Notifications
You must be signed in to change notification settings - Fork 570
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
Point out that the token must have write scope #2053
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2053 +/- ##
==========================================
+ Coverage 80.64% 80.70% +0.06%
==========================================
Files 71 71
Lines 8519 8522 +3
==========================================
+ Hits 6870 6878 +8
+ Misses 1649 1644 -5 ☔ View full report in Codecov by Sentry. |
src/huggingface_hub/utils/_errors.py
Outdated
+ "\nIf you are trying to create or update content," | ||
+ "make sure you have a token with the `write` role." | ||
) | ||
raise BadRequestError(message, response=response) from e |
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.
Hey @bmuskalla, thanks for the very clean and concise PR! 🚀
Everything looks good to me except that in case of a HTTP 403
I would raise a generic HfHubHTTPError
instead of a BadRequestError
. A Bad Request error might be misinterpreted as a HTTP 400 which is not the case here. Regarding the error message I would make it simpler:
message = f"{e}\nIf you are trying to create or update content, make sure you have a token with the `write` role."
raise HfHubHTTPError(message, response=response) from e
What do you think?
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.
Point taken on the confusing nature of using BadRequestError
, changed to HfHubHTTPError
.
For the message, I've played a bit more with a real error and ended up using Forbidden+server-side message + the hint.
Example:
Traceback (most recent call last):
File "/Users/bmuskalla/git/huggingface_hub/play.py", line 5, in <module>
create_repo(repo_id="bmuskalla/super-cool-model", token=HF_RO_TOKEN)
File "/Users/bmuskalla/git/huggingface_hub/src/huggingface_hub/utils/_validators.py", line 118, in _inner_fn
return fn(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^
File "/Users/bmuskalla/git/huggingface_hub/src/huggingface_hub/hf_api.py", line 3352, in create_repo
hf_raise_for_status(r)
File "/Users/bmuskalla/git/huggingface_hub/src/huggingface_hub/utils/_errors.py", line 367, in hf_raise_for_status
raise HfHubHTTPError(message, response=response) from e
huggingface_hub.utils._errors.HfHubHTTPError: (Request ID: Root=1-65df9500-64c1f6dd124322f75dae5c8f;b7c328b6-c3fa-47f6-830e-a488fca19cdd)
403 Forbidden: You don't have the rights to create a model under this namespace.
Cannot access content at: https://huggingface.co/api/repos/create.
If you are trying to create or update content,make sure you have a token with the `write` role.
I found this the most readable but it's your call in the end.
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.
For the message, I've played a bit more with a real error and ended up using Forbidden+server-side message + the hint.
Sounds good yes! Thanks for iterating on it :)
In case of an HTTP 403, inform the user that their token might not have the right scope for the action.
As things are today, the HF Hub API only returns
'X-Error-Message': "You don't have the rights to create a model under this namespace"
in that case so we have to rely on the HTTP error.I think a better solution would be for the API to return a proper
X-Error-Code
in case of entity creation/update + a read-only scoped token but that requires server-side changes first.Fixes #1653