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

Fix custom handling of refined HTTPError #924

Merged
merged 14 commits into from
Jun 28, 2022
Merged

Fix custom handling of refined HTTPError #924

merged 14 commits into from
Jun 28, 2022

Conversation

osanseviero
Copy link
Contributor

As discussed internally, #878 introduced a backwards compatibility issue as it does not contain all the information from the original error (such as the status_code), which was being used in downstream libraries such as speechbrain.

This PR changes the error handling to re-use the original error but with additional custom message. To avoid this situation happening again, also introducing some tests.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 22, 2022

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

Comment on lines -1562 to +1561
try:
_raise_for_status(r)
except requests.exceptions.RequestException as e:
try:
message = e.response.json()["error"]
except JSONDecodeError:
message = e.response.text
except AttributeError:
message = e.args[0]
raise type(e)(message) from e
_raise_for_status(r)
Copy link
Contributor Author

@osanseviero osanseviero Jun 23, 2022

Choose a reason for hiding this comment

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

This was causing issues. The existing fix would raise an error such as

huggingface_hub.utils._errors.RepositoryNotFoundError: 404 Client Error: Repository Not Found for url: https://huggingface.co/api/models/efwfwefw. If the repo is private, make sure you are authenticated. (Request ID: ktudRx2-3fp48WpTK6013)

but the e.response.text is still Repository not found. On the other hand with _raise_for_status(r) there is no need anymore to capture the type of error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still to be figured would be this though

try:
   model_info("efwfwefw")
except RepositoryNotFoundError as e:
   print(e.response.text)
{"error":"Repository not found"}

but

model_info("efwfwefw")

correctly gives

huggingface_hub.utils._errors.RepositoryNotFoundError: 404 Client Error: Repository Not Found for url: https://huggingface.co/api/models/efwfwefw. If the repo is private, make sure you are authenticated. (Request ID: WJtGL2IulzooY9JLwHEBn)

Co-authored-by: Simon Brandeis <33657802+SBrandeis@users.noreply.github.com>
@julien-c julien-c mentioned this pull request Jun 24, 2022
osanseviero and others added 4 commits June 24, 2022 12:12
Co-authored-by: Julien Chaumond <julien@huggingface.co>
@osanseviero
Copy link
Contributor Author

I did a second pass removing _raise_with_request_id and doing some cleanup/refactoring.

@osanseviero
Copy link
Contributor Author

At the end of _raise_for_status as a fallback if there is no error in the response header's, it raises the usual HTTPError with the addition of the request ID. For example:

  File "/home/osanseviero/Desktop/workspace/huggingface_hub/test.py", line 5, in <module>
    HfApi().get_model_tags()
  File "/home/osanseviero/miniconda3/envs/huggingface_hub/lib/python3.9/site-packages/huggingface_hub/hf_api.py", line 677, in get_model_tags
    _raise_for_status(r)
  File "/home/osanseviero/miniconda3/envs/huggingface_hub/lib/python3.9/site-packages/huggingface_hub/utils/_errors.py", line 107, in _raise_for_status
    raise e
  File "/home/osanseviero/miniconda3/envs/huggingface_hub/lib/python3.9/site-packages/huggingface_hub/utils/_errors.py", line 70, in _raise_for_status
    response.raise_for_status()
  File "/home/osanseviero/miniconda3/envs/huggingface_hub/lib/python3.9/site-packages/requests/models.py", line 960, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://huggingface.co/api/models-tags-by-typee (Request ID: IPaFZ_WCu1fOsmaavFXFX)

@osanseviero osanseviero requested a review from SBrandeis June 24, 2022 14:13
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 is very nice, thanks for working on it @osanseviero! Appreciate the work on the tests as well.

Co-authored-by: Lysandre Debut <lysandre.debut@reseau.eseo.fr>
@osanseviero
Copy link
Contributor Author

Feel free to merge if you think this looks good 😄 quality is unrelated (#934)

@LysandreJik LysandreJik merged commit 113e994 into main Jun 28, 2022
@LysandreJik LysandreJik deleted the fix-status branch June 28, 2022 15:00
@LysandreJik
Copy link
Member

Thanks for your work, @osanseviero!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants