Enabling hf_transfer use. #1272
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
|
@Wauplin I can safely ignore codecov right ? |
Yes no worries ! |
Codecov ReportBase: 83.94% // Head: 83.72% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1272 +/- ##
==========================================
- Coverage 83.94% 83.72% -0.22%
==========================================
Files 47 47
Lines 4622 4640 +18
==========================================
+ Hits 3880 3885 +5
- Misses 742 755 +13
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Hey @Narsil, following up on your PR. I made a few changes (renamed the env variable to HF_HUB_ENABLE_HF_TRANSFER, mentioned it in the docs and added more debug info).
I tested it locally and got a 1.3x speed up when downloading this file but as said in the call, my bottleneck is the network bandwith anyway.
Otherwise PR looks good to me now. Will wait from other reviews here.
|
I don't have any problems regarding the implementation. Still, I'm not too sure about adding an optional rust dependency for a speedup that seems to affect setups with high-speed bandwidth links (if I understand correctly from the internal comments). Is the idea that we would only use this internally? I don't think it would have community usage anyway, given that it would be particularly useful for larger artifacts, and I would think progress bars are imperative to large downloads as they would otherwise feel like shooting in the dark. |
|
yes my understanding is this is mostly (or only) for internal use (Inference API notably) |
|
As I see it, this is mostly for internal purposes. I have documented it only at 1 place in an "advanced" section and with warnings on the fact that it's "experimental" (understand "use it if you want but don't complain"). I don't think we will ever promote it out loud but having it is still relevant IMO |
|
For the sake of discussion, we could also monkey-patch this internally, no (potentially adding hooks to |
|
I would like also to mention @apolinario which thinks it could be very useful for Spaces (People tend to include the model files directly in the space because it's faster, but putting strain on the spaces infra as far as I understand). The caveat for spaces, is that by default for CPU there is a The actual benefit will depend on the kind of instance used to host the space (and neighboring usage of the node). @McPatate is also playing with DL speeds because the security scanner spends most of its time downloading (it's written in node, so wouldn't benefit directly from this, and it's already capping its network, but Luc recently upgraded the machines with the intended goal to get better network). Clearly the biggest win was this anyway: #1267 |
|
Ok in that case, if you feel strongly and think it won't hut maintenance @Wauplin, let's merge it in! |
|
Let's merge it then :) |
This enables optional faster download using
hf_transfer: https://github.com/huggingface/hf_transferHF_TRANSFER=1(makes it easy to disable).With it enabled, the download might look "slow" because most of the time can be taken while moving from a tmpfs to actual disk the model files.
This is deemed out of scope for now.
hf_transfer provides no feedback during download, again this can be added later if we want to grow the spec for this.