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

Define error #2444

Merged
merged 8 commits into from
Aug 20, 2024
Merged

Define error #2444

merged 8 commits into from
Aug 20, 2024

Conversation

cjfghk5697
Copy link
Contributor

#2069

I have implemented the suggestion from issue #2069. The FileMetadataError class has been moved from huggingface_hub/inference/_templating.py to huggingface_hub/errors.py.

Could you please review this PR? If this is correct, I will move the other classes in the same way.

@cjfghk5697 cjfghk5697 closed this Aug 13, 2024
@cjfghk5697 cjfghk5697 reopened this Aug 13, 2024
@cjfghk5697
Copy link
Contributor Author

@Wauplin Are there any other error classes that need to be modified or moved besides FileMetadataError? Also, do you think there's a need for any error handling code for 500-level errors?

@010kim
Copy link
Contributor

010kim commented Aug 13, 2024

Hello, I was going over the same code actually. It looks like FileMetadataError is the only error that is not affected by hf_raise_for_status, so all the other errors _errors.py should remain there. @Wauplin So I won't create any separate PR. Thanks a lot for your response!

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Hi @cjfghk5697 , thanks for your PR! Actually all errors from src/huggingface_hub/utils/_errors.py are entitled to move to src/huggingface_hub/errors.py as mentioned in #2069 (comment). The logic for hf_raise_for_status stays where it is but the error classes definitions have to move. It can be done in this PR or a separate one.

Also, do you think there's a need for any error handling code for 500-level errors?

I don't think there is a special need for that no. Those are usually caught as HfHttpError and then filtered using the status code directly.

src/huggingface_hub/file_download.py Outdated Show resolved Hide resolved
@Wauplin
Copy link
Contributor

Wauplin commented Aug 13, 2024

Hello, I was going over the same code actually. It looks like FileMetadataError is the only error that is not affected by hf_raise_for_status, so all the other errors _errors.py should remain there. @Wauplin So I won't create any separate PR. Thanks a lot for your response!

@010kim sorry for the duplicate work. To avoid such a situation, would you like to focus on moving the 2 exceptions from src/huggingface_hub/utils/_cache_manager.py for example? They also have to be moved in a separate PR similar to this one.

@010kim
Copy link
Contributor

010kim commented Aug 13, 2024

Hello, I was going over the same code actually. It looks like FileMetadataError is the only error that is not affected by hf_raise_for_status, so all the other errors _errors.py should remain there. @Wauplin So I won't create any separate PR. Thanks a lot for your response!

@010kim sorry for the duplicate work. To avoid such a situation, would you like to focus on moving the 2 exceptions from src/huggingface_hub/utils/_cache_manager.py for example? They also have to be moved in a separate PR similar to this one.

Of course! That's great, I'm on it!

@cjfghk5697
Copy link
Contributor Author

@Wauplin, thank you for your reply! Do you have any recommended suggestions or issues for improving Hub?

@Wauplin
Copy link
Contributor

Wauplin commented Aug 16, 2024

Hi @cjfghk5697, thanks for asking! Sorry for the noise on your PR, let's refocus on this one first 😉 As mentioned in #2444 (review), it would be great to move all errors from src/huggingface_hub/utils/_errors.py to src/huggingface_hub/errors.py, not only FileMetadataError. The hf_raise_for_status should stay where it is. Could you have a look at that? 🙏 🤗

@cjfghk5697
Copy link
Contributor Author

cjfghk5697 commented Aug 16, 2024

@Wauplin Thank you! I'm a bit confused should the class used in hf_raise_for_status also be moved to error.py? For example, the RevisionNotFoundError class is used in hf_raise_for_status, but should it also be moved to error.py?🤗

@Wauplin
Copy link
Contributor

Wauplin commented Aug 16, 2024

I'm a bit confused should the class used in hf_raise_for_status also be moved to error.py? For example, the RevisionNotFoundError class is used in hf_raise_for_status, but should it also be moved to error.py?🤗

Yes, all of those classes! (RevisionNotFoundError, GatedRepoError, etc.)
Then, you'll have to import them back from src/huggingface_hub/errors.py in src/huggingface_hub/utils/_errors.py. The goal is to have all classes defined in the same errors module but we should still keep some logic outside (where it is currently)

@cjfghk5697
Copy link
Contributor Author

cjfghk5697 commented Aug 18, 2024

I'm a bit confused should the class used in hf_raise_for_status also be moved to error.py? For example, the RevisionNotFoundError class is used in hf_raise_for_status, but should it also be moved to error.py?🤗

Yes, all of those classes! (RevisionNotFoundError, GatedRepoError, etc.) Then, you'll have to import them back from src/huggingface_hub/errors.py in src/huggingface_hub/utils/_errors.py. The goal is to have all classes defined in the same errors module but we should still keep some logic outside (where it is currently)

I got it and appreciate your reply😊 The classes in _errors.py have been moved to errors.py. Could you check the file? 🤗.

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks @cjfghk5697 for the changes! Almost everything looks good to me for a merge. Left a minor comment and then we should be good to go! 🚀

Comment on lines 9 to 21
REPO_API_REGEX = re.compile(
r"""
# staging or production endpoint
^https://[^/]+
(
# on /api/repo_type/repo_id
/api/(models|datasets|spaces)/(.+)
|
# or /repo_id/resolve/revision/...
/(.+)/resolve/(.+)
)
""",
flags=re.VERBOSE,
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be kept in ./utils/_errors.py since it's not a Exception class definition

@cjfghk5697
Copy link
Contributor Author

Thanks @Wauplin! I have put it back!

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Perfect!

I fixed a circular import in f3525e6. That's why I want to avoid as much as possible logic to be defined in errors.py. Here we can't do without it so it's fine :)

@HuggingFaceDocBuilderDev

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.

@Wauplin
Copy link
Contributor

Wauplin commented Aug 20, 2024

Failing tests are unrelated to this PR so let's merge it :) Thanks @cjfghk5697 for the work and iterations 🤗

@Wauplin Wauplin merged commit 509a431 into huggingface:main Aug 20, 2024
14 of 18 checks passed
@cjfghk5697 cjfghk5697 deleted the define_errors branch August 20, 2024 12:36
@cjfghk5697
Copy link
Contributor Author

cjfghk5697 commented Aug 22, 2024

@Wauplin
Is there anything else I can assist you with besides handling this issue? I'd be happy to offer additional support alongside the HuggingFace OSSCA team!

@Wauplin
Copy link
Contributor

Wauplin commented Aug 27, 2024

Hi @cjfghk5697, thanks again for your support! What do you say about tackling #2466? :) If not, you can always have a look to the list in https://github.com/huggingface/huggingface_hub/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22. Let me know if you have any preferences.

@cjfghk5697
Copy link
Contributor Author

Hi @Wauplin, thanks for the suggestion! I’ll go ahead and tackle #2466. Looking forward to working on it!

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.

4 participants