-
Notifications
You must be signed in to change notification settings - Fork 632
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
Refine 404 errors #878
Refine 404 errors #878
Conversation
The documentation is not available anymore as the PR was closed or merged. |
This PR can be independent of the |
bc45c4e
to
9058578
Compare
992aa13
to
2ef1f90
Compare
src/huggingface_hub/utils/_errors.py
Outdated
elif error_code == "EntryNotFound": | ||
raise EntryNotFoundError( | ||
f"404 Client Error: Entry Not Found for url: {request.url}" | ||
) | ||
elif error_code == "RevisionNotFound": | ||
raise RevisionNotFoundError( | ||
f"404 Client Error: Revision Not Found for url: {request.url}" | ||
) |
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.
nit but I would change the order of those two as EntryNotFound
implies that a revision was found
i.e. the errors are more and more "closer to being successful"
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.
Otherwise LGTM.
src/huggingface_hub/utils/_errors.py
Outdated
Raised when trying to access a hf.co URL with an invalid repository name, or | ||
with a private repo name the user does not have access to. | ||
""" |
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.
The first line should be a short single line explanation, then a longer description if necessary, an an example of when the exception would be raised would also be nice.
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.
Good call! Updated in latest commit
""" | ||
|
||
|
||
class EntryNotFoundError(HTTPError): |
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.
Should this maybe be FileNotFoundError
?
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.
Will let @julien-c comment on the choice of "entry", but I'd rather stay as close as possible to the error thrown by the backend
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 strong opinion! up to you.
Maybe we can document that this mean that the repo AND the revision exist/are accessible, BUT not the file/entry? or is it clear enough already?
0ff6f39
to
8e65f05
Compare
Will adapt to the new 401 error. |
@SBrandeis, @adrinjalali, could you verify and confirm this looks good to you? |
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.
Otherwise LGTM!
@@ -1180,6 +1181,18 @@ def model_info( | |||
|
|||
Returns: | |||
[`huggingface_hub.hf_api.ModelInfo`]: The model repository information. | |||
|
|||
<Tip> |
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.
should this be a tip or in the Raises
section? (same for the following changes)
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.
So far we have been using <Tip>
across the codebase as Raises
wasn't working with doc-builder. It's my fault, it seems I was delaying huggingface/doc-builder#141.
If it's fine with you, I'll merge it like this and open an issue to modify all of those <Tip>
to Raises:
once the PR above is merged.
url = hf_hub_url("bert-base", filename="pytorch_model.bin") | ||
with self.assertRaisesRegex( | ||
RepositoryNotFoundError, "401 Client Error: Repository Not Found" | ||
): | ||
_ = cached_download(url, legacy_cache_layout=True) |
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.
Note the behavior changes when authenticated - in that case the error message will be:
404 Client Error: Repository Not Found
LGTM 👍 Let's maybe add a comment stating that Or maybe add an additional check in Feel free to disregard if you think it's not relevant :) |
if "X-Error-Code" in request.headers: | ||
error_code = request.headers["X-Error-Code"] | ||
if error_code == "RepoNotFound": | ||
raise RepositoryNotFoundError( | ||
f"404 Client Error: Repository Not Found for url: {request.url}. " | ||
"If the repo is private, make sure you are authenticated." | ||
) | ||
elif error_code == "RevisionNotFound": | ||
raise RevisionNotFoundError( | ||
f"404 Client Error: Revision Not Found for url: {request.url}" | ||
) | ||
elif error_code == "EntryNotFound": | ||
raise EntryNotFoundError( | ||
f"404 Client Error: Entry Not Found for url: {request.url}" | ||
) | ||
|
||
if request.status_code == 401: | ||
# The repo was not found and the user is not Authenticated | ||
raise RepositoryNotFoundError( | ||
f"401 Client Error: Repository Not Found for url: {request.url}. " | ||
"If the repo is private, make sure you are authenticated." | ||
) |
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.
Logic of _raise_for_status
LGTM (also cc @Pierrci @allendorf @coyotte508 for a double check)
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.
BTW we should maybe have "extended" i.e. wrapped the existing HTTPError instead of re-instantiating a new one.
This change broke this code in Speechbrain:
https://github.com/speechbrain/speechbrain/blob/d6bfe138a90dff3490f7196acbd9c62939289d46/speechbrain/pretrained/fetching.py#L115-L121
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.
Also the 404 shouldn't be hardcoded but should ideally be replaced by {request.status_code}
to reflect the actual server output
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.
see #924
I added some comments @SBrandeis. Should be good to merge. |
* Add errors * Style * Review * hf_hub_download support * Errors for hf_hub_download * Typo * Handle 401 error * Tests for 401 error * Typo * Review comments
* added autoeval fields to repocard types * modified test, linted * Fix typo (#902) * Refine 404 errors (#878) * Add errors * Style * Review * hf_hub_download support * Errors for hf_hub_download * Typo * Handle 401 error * Tests for 401 error * Typo * Review comments * added documentation for the repocard change in repocard, updated metadata_eval_result, added documentation * 🏗 Use `hub-ci` for tests (#898) * 🏗 Use `hub-ci` for tests cc @XciD * 🩹 Also update URL for staging mode * ✅ 401 is raised when the user is not authenticated * 🗑 Deprecare `identical_ok` * Longer deprecation period * ✅ Fix the last failing test * Warning match docstring * FIX Avoid creating repository when it exists on remote (#900) * fix for spaces * fix for spaces * removed creating repository and added warning * revert my changes * added tests * removed debugger 😐 * fixed repository removal * Added tests and error * import pytest * fixed tests * fixed tests * style * removed repo removal * make style * fixed test with Lysandres patch * added fix * Remove deprecations (#910) * Remove deprecations * Typo * Update src/huggingface_hub/README.md Co-authored-by: Julien Chaumond <julien@huggingface.co> Co-authored-by: Julien Chaumond <julien@huggingface.co> * Add request ID to all requests (#909) * Add request ID to all requests * Typo * Typo * Review comments * Update src/huggingface_hub/utils/_errors.py Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * Invert deprecation for create_repo (#912) * Invert deprecation for create_repo * Set to 0.10 * Constant was accidentally removed during deprecation transition (#913) Co-authored-by: Lyle Nel <lyle@owlin.com> * Update src/huggingface_hub/repocard_types.py Co-authored-by: lewtun <lewis.c.tunstall@gmail.com> * Update repocard_types.py * Update repocard_types.py * reorded metadata_eval_request_docs, added metrics_config and metrics_verified to tests Co-authored-by: lsb <leebutterman@gmail.com> Co-authored-by: Lysandre Debut <lysandre.debut@reseau.eseo.fr> Co-authored-by: Simon Brandeis <33657802+SBrandeis@users.noreply.github.com> Co-authored-by: Merve Noyan <merveenoyan@gmail.com> Co-authored-by: Julien Chaumond <julien@huggingface.co> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: Lyle Nel <lyle-nel@users.noreply.github.com> Co-authored-by: Lyle Nel <lyle@owlin.com> Co-authored-by: lewtun <lewis.c.tunstall@gmail.com>
This PR adds finer error management to the Hugging Face Hub error messages.
Here's the difference between current
main
and this PR on the following errors:An example for
hf_api
:model_info
Using the
model_info
method:Before:
After:
An example for
cached_download
cached_download
is an interesting test-case as it can raise the three different errors. Right now, onmain
, using either of the three following errors results in the same error message:This PR will now provide tailored error messages:
Wrong repo ID
Wrong file
Wrong revision