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

add hf_transfer to requirements.txt #3031

Merged
merged 4 commits into from
Mar 13, 2024
Merged

Conversation

RonanKMcGovern
Copy link
Contributor

This will allow users to set 'HF_HUB_ENABLE_HF_TRANSFER=True' and greatly speed up weight download speed from HF.

Copy link
Collaborator

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

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

According to the introduction, it seems like hf_transfer is not at a stable stage as of now?

This library is a power user tool, to go beyond ~500MB/s on very high bandwidth network, where Python cannot cap out the available bandwidth.
This is not meant to be a general usability tool. It purposefully lacks progressbars and comes generally as-is.

Since using the library only requires installing hf_transfer and setting the env variable, I believe we can leave this as optional for users. Instead, maybe we can mention it in our docs it so that the feature is more visible. WDYT? @RonanKMcGovern

@simon-mo
Copy link
Collaborator

@WoosukKwon I think it is actually pretty stable. The main problem is just progress bar which is fine to ignore for product use case.

requirements.txt Outdated Show resolved Hide resolved
@WoosukKwon
Copy link
Collaborator

WoosukKwon commented Feb 26, 2024

I think it is actually pretty stable.

@simon-mo Could you provide more background on this?

@simon-mo
Copy link
Collaborator

It is actively maintained and I also spent quite some time in the past benchmarking/reading its code.

@WoosukKwon
Copy link
Collaborator

WoosukKwon commented Feb 26, 2024

I see. Still, my impression is that this library is experimental.

Even if we'd like to use it for vLLM, I believe we should set the env variable like os.environ["HF_HUB_ENABLE_HF_TRANSFER"] = "1"? Otherwise, I don't think we have to include it as vLLM's dependency.

@Yard1
Copy link
Collaborator

Yard1 commented Feb 26, 2024

Agreed, we should not make optional packages into core requirements.

@chenxu2048
Copy link
Contributor

Agreed, we should not make optional packages into core requirements.

Maybe we could add a guide in Quickstart instead of an optional package in requirements.txt.

@RonanKMcGovern
Copy link
Contributor Author

RonanKMcGovern commented Feb 27, 2024 via email

@RonanKMcGovern
Copy link
Contributor Author

FWIW hf_transfer is installed in the TGI docker image

@Yard1
Copy link
Collaborator

Yard1 commented Feb 27, 2024

Perhaps we could include it in the vllm docker image, but not in the requirements?

@RonanKMcGovern
Copy link
Contributor Author

Perhaps we could include it in the vllm docker image, but not in the requirements?

That would fit my needs anyway.

Shall I put it under "# install build and runtime dependencies"

@simon-mo
Copy link
Collaborator

This would be a good place to put it

pip install accelerate

@RonanKMcGovern
Copy link
Contributor Author

This would be a good place to put it

pip install accelerate

Thanks! I've done that now and pulled it out from where I had added it to reqs

@WoosukKwon
Copy link
Collaborator

Shouldn't we also add something like os.environ["HF_HUB_ENABLE_HF_TRANSFER"] = "1" to the dockerfile?

@simon-mo
Copy link
Collaborator

@WoosukKwon I think that can be optional? User can enable it by docker run -e HF_HUB_ENABLE_HF_TRANSFER=1 or setting the relevant variable in their configuration.

Dockerfile Outdated Show resolved Hide resolved
@WoosukKwon
Copy link
Collaborator

does not it require installing rust for this package to work?

@simon-mo Is this true? Otherwise, I feel we can merge the PR?

@simon-mo
Copy link
Collaborator

It had prebuilt package. Rust is not required.

@WoosukKwon
Copy link
Collaborator

@simon-mo Got it. Let's merge the PR then?

@simon-mo simon-mo merged commit e221910 into vllm-project:main Mar 13, 2024
20 of 22 checks passed
starmpcc pushed a commit to starmpcc/vllm that referenced this pull request Mar 14, 2024
@RonanKMcGovern
Copy link
Contributor Author

thanks for the merge, this is great

@RonanKMcGovern
Copy link
Contributor Author

does the docker image need to be re-built to incorporate this? @simon-mo

@simon-mo
Copy link
Collaborator

Yes. It will be there for next release. Thank you for the patience.

@RonanKMcGovern
Copy link
Contributor Author

Thanks @simon-mo , I think it's worth pushing asap because the docker image is much more usable for drbx if hf_transfer is there. Otherwise it takes an age to start model.

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.

6 participants