Skip to content
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

Clearer HTTP error messages in huggingface_hub #1019

Merged
merged 19 commits into from
Sep 5, 2022

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Aug 30, 2022

Fix #989 (and duplicate #989).

What this PR do

The initial issue was about adding meaningful information to the HTTPError (especially advice like "create_commit expects the repo to exist" or "make sure you specific the right repo_id/repo_type"). This PR aims to solve that but also improve and harmonize how HTTP Errors are processed in huggingface_hub.

It introduces:

  • hf_raise_for_status -> meant to be the single way to raise for status in HF Hub. Also meant to be reused "officially" by third-party libraries like transformers (at the moment huggingface_hub.utils._errors._raise_for_status is called).
  • HfHubHTTPError -> a custom error that inherits from response.HTTPError and that is parents to the RepositoryNotFoundError, RevisionNotFoundError,... It handles the formatting of the Hub details in the error message.
  • I also added a small section in the documentation for hf_raise_for_status usage.

At the moment, _raise_for_status, _raise_with_request_id and _raise_convert_bad_request that were doing similar things are all kept as aliases for hf_raise_for_status. I deprecated their usage instead of removing them. I can also keep them without warning message but I like it less (cc @sgugger).

Examples

I refined the error messages to have multi-line descriptions. I prefer it this way to make it clearer for the end-user but please let me know if you see a problem with that. Errors look like this:

huggingface_hub.utils._errors.RepositoryNotFoundError: 401 Client Error. (Request ID: PvMw_VjBMjVdMz53WKIzP)

Repository Not Found for url: https://huggingface.co/api/models/%3Cnon_existent_repository%3E.
Please make sure you specified the correct `repo_id` and `repo_type`.
If the repo is private, make sure you are authenticated.
Invalid username or password.
huggingface_hub.utils._errors.RepositoryNotFoundError: 404 Client Error. (Request ID: xTE4dk9e6-ZATpr3JeWgC)

Repository Not Found for url: https://hub-ci.huggingface.co/api/models/__DUMMY_TRANSFORMERS_USER__/repo_that_do_not_exist/preupload/main.
Please make sure you specified the correct `repo_id` and `repo_type`.
If the repo is private, make sure you are authenticated.
Note: Creating a commit assumes that the repo already exists on the Huggingface Hub. Please use `create_repo` if it's not the case.
huggingface_hub.utils._errors.RevisionNotFoundError: 404 Client Error. (Request ID: Mwhe_c3Kt650GcdKEFomX)

Revision Not Found for url: https://huggingface.co/bert-base-cased/resolve/%3Cnon-existent-revision%3E/config.json.

(cc @NielsRogge @merveenoyan @cakiki who showed interest in the issue)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 30, 2022

The documentation is not available anymore as the PR was closed or merged.

@Wauplin Wauplin changed the title [WIP] Clearer error messages in huggingface_hub [WIP] Clearer HTTP error messages in huggingface_hub Aug 30, 2022
@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #1019 (a545192) into main (48ddc62) will increase coverage by 0.17%.
The diff coverage is 95.14%.

❗ Current head a545192 differs from pull request most recent head 3dca9e4. Consider uploading reports for the commit 3dca9e4 to get more accurate results

@@            Coverage Diff             @@
##             main    #1019      +/-   ##
==========================================
+ Coverage   82.16%   82.34%   +0.17%     
==========================================
  Files          35       35              
  Lines        3588     3596       +8     
==========================================
+ Hits         2948     2961      +13     
+ Misses        640      635       -5     
Impacted Files Coverage Δ
src/huggingface_hub/commands/lfs.py 0.00% <0.00%> (ø)
src/huggingface_hub/utils/__init__.py 100.00% <ø> (ø)
src/huggingface_hub/lfs.py 74.68% <66.66%> (+0.16%) ⬆️
src/huggingface_hub/_commit_api.py 91.96% <100.00%> (-0.08%) ⬇️
src/huggingface_hub/file_download.py 85.75% <100.00%> (+0.11%) ⬆️
src/huggingface_hub/hf_api.py 86.91% <100.00%> (+0.06%) ⬆️
src/huggingface_hub/utils/_deprecation.py 100.00% <100.00%> (ø)
src/huggingface_hub/utils/_errors.py 100.00% <100.00%> (+5.63%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Wauplin Wauplin marked this pull request as ready for review August 31, 2022 09:01
@Wauplin Wauplin changed the title [WIP] Clearer HTTP error messages in huggingface_hub Clearer HTTP error messages in huggingface_hub Aug 31, 2022
@sgugger
Copy link
Contributor

sgugger commented Aug 31, 2022

The warnings will pollute the use of Transformers by any user using a source install of huggingface_hub, without them being able to do anything to silence them, so they should be removed.
This is not a normal deprecation cycle, just a temporary alias until the next release (as we discussed previously). The old names can all be removed in the last commit before releasing the new version of huggingface_hub.

@Wauplin
Copy link
Contributor Author

Wauplin commented Sep 1, 2022

@sgugger I removed the warning from _raise_for_status that is used in Transformers in commit a545192 and added a TODO there.

In general I'm not a big fan of having a temporary alias that we need to remember about when we do the release since it requires that we synchronize ourselves on the releases. So for this one I suggest that we keep the alias without warning and starting from just after v0.10 (so not in official v0.10 release) I will deprecate it/remove it considering that Transformers no longer uses it. Would that be ok ? The only difference is that we keep the alias for one more version and in between Transformers should be able to adapt but no warnings are thrown.

@sgugger
Copy link
Contributor

sgugger commented Sep 1, 2022

Yes, that works even better. Thanks!

Copy link
Contributor

@sgugger sgugger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of nits, but looks great otherwise!
Thanks!

docs/source/package_reference/utilities.mdx Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
src/huggingface_hub/utils/_errors.py Outdated Show resolved Hide resolved
src/huggingface_hub/utils/_errors.py Outdated Show resolved Hide resolved
src/huggingface_hub/utils/_errors.py Outdated Show resolved Hide resolved
Wauplin and others added 3 commits September 1, 2022 17:34
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks for working on it, @Wauplin, very happy to have a stable and maintained public API for hf_raise_for_status.

src/huggingface_hub/utils/_errors.py Show resolved Hide resolved
@Wauplin Wauplin merged commit 49c27b4 into main Sep 5, 2022
@Wauplin Wauplin deleted the 989-better-error-messages-in-hf-api branch September 5, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error messages when pushing/pulling to/from the hub
4 participants