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 URL in get_safetensors_metadata docstring #1951

Merged
merged 5 commits into from
Jan 9, 2024
Merged

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Jan 4, 2024

Tweak safetensors parser: fetch only 25kb by default (instead of 100kb currently). Still valid for most use cases (82% in top 1000 models) and save some bandwidth. Most probably won't be noticed. (see #1855 (comment)) => EDIT: not changed in the end.

Fix typo in docs: wrong url in get_safetensors_metadata docstring.

@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.

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2bdab6a) 82.42% compared to head (d76e293) 82.48%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1951      +/-   ##
==========================================
+ Coverage   82.42%   82.48%   +0.06%     
==========================================
  Files          66       66              
  Lines        8154     8155       +1     
==========================================
+ Hits         6721     6727       +6     
+ Misses       1433     1428       -5     

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

@LysandreJik
Copy link
Member

Did we encounter a problem with 100kb? I think it's reasonable enough already so wondering if we need the change

@Wauplin
Copy link
Contributor Author

Wauplin commented Jan 9, 2024

Did we encounter a problem with 100kb? I think it's reasonable enough already so wondering if we need the change

No problems reported no.
It's just that I thought saving 4x bandwidth in 82% percent of cases while adding an extra HEAD call in 15% of cases seemed reasonable. But fine with keeping 100kb if you feel like it

@julien-c
Copy link
Member

julien-c commented Jan 9, 2024

i would also tend to not change anything, personally..

@Wauplin
Copy link
Contributor Author

Wauplin commented Jan 9, 2024

Ok, reverted in 456555a.

PR becomes "fix typo in docs" :)

@Wauplin Wauplin changed the title Tweak safetensors parser + fix docs Fix URL in get_safetensors_metadata docstring Jan 9, 2024
@Wauplin Wauplin merged commit af0fa6e into main Jan 9, 2024
15 checks passed
@Wauplin Wauplin deleted the tweak-safetensors-parser branch January 9, 2024 17:20
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.

None yet

4 participants