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

Fix a bug to respect the HF_HUB_ETAG_TIMEOUT. #1728

Merged
merged 4 commits into from
Oct 13, 2023

Conversation

Shahafgo
Copy link
Contributor

The purpose of this pr is to fix a bug that seems the HF_HUB_ETAG_TIMEOUT is not respected.
It looks like the HF_HUB_DOWNLOAD_TIMEOUT is respected in places that the HF_HUB_ETAG_TIMEOUT should be respected.

@Shahafgo
Copy link
Contributor Author

@Wauplin , a small fix for the previous pr, seems like when I took your suggestion, there were couple of places you put HF_HUB_DOWNLOAD_TIMEOUT instead of HF_HUB_ETAG_TIMEOUT.
So it seems that on the main branch for now, only the HF_HUB_DOWNLOAD_TIMEOUT is respected (:

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 12, 2023

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

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (30f5ed4) 49.23% compared to head (9b1a2b4) 82.04%.

❗ Current head 9b1a2b4 differs from pull request most recent head 357e0f9. Consider uploading reports for the commit 357e0f9 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1728       +/-   ##
===========================================
+ Coverage   49.23%   82.04%   +32.81%     
===========================================
  Files          62       62               
  Lines        7278     7278               
===========================================
+ Hits         3583     5971     +2388     
+ Misses       3695     1307     -2388     
Files Coverage Δ
src/huggingface_hub/file_download.py 84.97% <50.00%> (+35.99%) ⬆️

... and 43 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Hey @Shahafgo I am sorry for the fuss and merging #1720 while you were working at it. I really thought it was in a final state and did not realize about the bug you've noticed. Thanks a lot for opening a follow-up PR. To be sure, I added 4 small test cases that should ensure HF_HUB_ETAG_TIMEOUT is respected correctly.
Thanks again, I'll merge this hot-fix PR as well now.

@Wauplin Wauplin merged commit c36eb68 into huggingface:main Oct 13, 2023
14 checks passed
@Shahafgo
Copy link
Contributor Author

@Wauplin Thank you very much for your help!

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