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 cache errors in errors.py #2456

Closed
wants to merge 0 commits into from

Conversation

010kim
Copy link
Contributor

@010kim 010kim commented Aug 19, 2024

Below is the list of descriptions for this PR.

  1. Defined two errors (CacheNotFound, CorruptedCacheExcpetion) in errors.py
  • imported typing and pathlib package to keep the original definition of CacheNotFound
  1. Adjusted the import paths in init.py

  2. Removed the original definitions of the errors in _cache_manager.py

  • added "from huggingface_hub.errors import CacheNotFound, CorruptedCacheException" for API support

#2069

@010kim 010kim marked this pull request as ready for review August 19, 2024 02:14
@010kim
Copy link
Contributor Author

010kim commented Aug 19, 2024

@Wauplin Hello! I have a question about the definition for CacheNotFound class. I imported pathlib and typing packages in errors.py to keep the original definition of CacheNotFound. But I found in the previous comments of yours that you want to keep errors.py as simple as possible. Do you have any suggestions?

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.

That's perfect, thanks!

I imported pathlib and typing packages in errors.py to keep the original definition of CacheNotFound. But I found in the previous comments of yours that you want to keep errors.py as simple as possible. Do you have any suggestions?

What I meant in #2069 (comment) is that we should not add logic to errors.py (for example, logic to raise an error from a failed requests.Response. However, it's perfectly fine to import standard modules. What would be annoying would be to have from huggingface_hub.... import ... in errors.py (which is unnecessary and can cause circular imports issues)

@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 19, 2024

There seems to be styling issues. Could you install dev dependencies locally (see here) and then run make style (see here) to fix them. You can then test with make quality and commit the changes.

@010kim 010kim closed this Aug 21, 2024
@010kim 010kim force-pushed the move_cache_errors branch from da63a88 to 359093f Compare August 21, 2024 05:10
@010kim
Copy link
Contributor Author

010kim commented Aug 21, 2024

I closed the PR to sync with the newest main branch. I will create another PR with the original fix.

@Wauplin
Copy link
Contributor

Wauplin commented Aug 21, 2024

It's also possible to update click on "update branch" on the PR page to get the latest update on your branch 😉

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.

3 participants