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

[Feature | CI] Added a github action to build wheels #746

Merged
merged 12 commits into from
Aug 21, 2023

Conversation

Danielkinz
Copy link
Contributor

Changes

Created a new action called "Create Release" (publish.yml) which automatically detects a branch of the format "release/<version>", creates a release with same version, build and uploads wheels for said release.

Supported Versions:

The following covers which environments the wheels cover:

  • Linux with glibc 2.31 or newer (ex. ubuntu 20.04 or newer) - github doesn't have older runners
  • Cuda toolkit 11.8 - github runner failed to install torch for cuda 11.7. Older versions not supported by torch >=2.0.0.
  • Python 3.8, 3.9, 3.10, 3.11 - torch doesn't support older versions

Notes

  • The github action can be edited to fit the main repository's release pattern.
  • Example runs and results can be seen in the forked repository

Next Steps

  • Replace manual build call with cibuildwheel - could possibly add more flexibility to the wheel versions.
  • Add support for windows/macos if needed.
  • Add cuda 12.x once torch 2.1.0 becomes stable

@WoosukKwon WoosukKwon self-requested a review August 14, 2023 16:37
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.

Wonderful! Thank you so much for this amazing work @Danielkinz !
I've checked that the wheels are automatically generated by the GitHub action, and the wheels can be installed very smoothly on python 3.8, 3.9, 3.10, and 3.11. It also works on the NVIDIA PyTorch containers with CUDA 12 installed. 👍

I only request small changes:

  1. Could you change the names of the generated wheels from, e.g., vllm-0.1.3-cp310-cp310-linux_x86_64.whl to vllm-0.1.3-cp310-cp310-manylinux1_x86_64.whl which is compatible with the naming convention in PyPI?
  2. When creating a release, could you also include the auto-generated release note?
  3. Could you change the GitHub action trigger to any new v* tag?

After all, this is very nice work. Thank you so much again @Danielkinz

.github/workflows/scripts/cuda-install.sh Outdated Show resolved Hide resolved
.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Show resolved Hide resolved
.github/workflows/scripts/env.sh Outdated Show resolved Hide resolved
@Danielkinz
Copy link
Contributor Author

Danielkinz commented Aug 15, 2023

Thank you for the review @WoosukKwon :)

About the requested changes:

  1. Changing the name should pose no issue at all. I already experimented with it a bit. Also, I believe I saw an action to automatically upload to PyPI. Would you like me to check it out as well?
  2. How does the autogeneration work currently? I didn't see a .github/release.yml file.
  3. Switching to trigger on tag should be simple. If you prefer its also possible to trigger on release to fill in the wheels, which might solve 2 as well

@WoosukKwon
Copy link
Collaborator

Hi @Danielkinz,

  1. Yeah, it would be awesome if the GitHub action can also rename and upload the wheels to PyPI.
  2. I think it is possible by using GitHub CLI. Could you refer to this link?
  3. Yes, please switch the trigger. I personally feel the v* tag trigger is a bit more intuitive.

In summary, the ideal workflow for us is that the GitHub action is triggered when a new v* tag is created, then it builds python wheels, creates new release with the wheels and an auto-generated release note, and uploads the wheels to PyPI. However, we can skip 2 (release notes) for now if it is not easy.

@Danielkinz
Copy link
Contributor Author

Alright. I'll take a look at it over the next couple of days :)

@WoosukKwon
Copy link
Collaborator

Hi @Danielkinz, what's going on this PR? Please let us know if you need any help.

@Danielkinz
Copy link
Contributor Author

Danielkinz commented Aug 18, 2023

Ah sorry about that. Last night I updated the fork and moved some branches around and the fork's main branch ended up on the same commit as the main repo's. Seems like github automatically closes PR's when that happens.
The requested features are almost done and are currently in testing. The only feature I'm having a bit of trouble with is the PyPI auto upload since its more difficult to test it. (since it requires to set up a test.pypi project)

@Danielkinz Danielkinz reopened this Aug 18, 2023
@WoosukKwon
Copy link
Collaborator

Hi @Danielkinz, thanks for sharing the status! Then, I think we can manually upload the wheels for now, and implement the feature later. WDYT? This PR is already super awesome.

@Danielkinz
Copy link
Contributor Author

As long as you're fine with it. Theoretically the PyPI auto upload should work, I simply can't test it easily. If you'd like you can try it out on the main branch with test.pypi next time there's a release on the main branch, it shouldn't affect the rest of the pipeline.
I currently encountered another problem - it seems like nvidia has updated their apt repo last night, which causes an error when trying to install their libraries:
"E: Failed to fetch https://developer.download.nvidia.com/compute/cuda/repos/ubuntu2004/x86_64/Packages.gz File has unexpected size (1127072 != 1126689). Mirror sync in progress? [IP: 152.195.19.142 443]"
Their repo: https://developer.download.nvidia.com/compute/cuda/repos/ubuntu2004/x86_64/
For now I can't continue testing the features since this error kills the pipeline. It should be a temporary problem.

@WoosukKwon
Copy link
Collaborator

As long as you're fine with it. Theoretically the PyPI auto upload should work, I simply can't test it easily. If you'd like you can try it out on the main branch with test.pypi next time there's a release on the main branch, it shouldn't affect the rest of the pipeline.

Yes, let's have it in a future PR, as many users are waiting for this PR to be merged. 😃 Could you just let me know if 1) the release notes are now auto-generated, and 2) the wheels are renamed to the format like vllm-0.1.3-cp310-cp310-manylinux1_x86_64.whl? I believe these two are also optional and should not block this PR though.

@Danielkinz
Copy link
Contributor Author

Yes, both are implemented. After I test them the PR should be good to go.

@WoosukKwon
Copy link
Collaborator

@Danielkinz Great, please let me know if the PR is ready.

@Danielkinz
Copy link
Contributor Author

@WoosukKwon The PR is ready :)
Seems like the issue with nvidia's repo was resolved last night so I could test it out.

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.

@Danielkinz LGTM. Thanks for the super awesome work! This will save many users from the tedious installation issues. Great work!

@WoosukKwon WoosukKwon merged commit c393af6 into vllm-project:main Aug 21, 2023
@WoosukKwon WoosukKwon mentioned this pull request Aug 23, 2023
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Feb 13, 2024
yma11 pushed a commit to yma11/vllm that referenced this pull request Jan 31, 2025
The vLLM notebooks in the the Gaudi-tutorials repo were moved, which
resulted in broken links in the README file. This PR fixes those links.

Signed-off-by: Dina Suehiro Jones <dina.s.jones@intel.com>
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.

2 participants