-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[CI/Build] Add x86 CPU wheel release pipeline #28848
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new CI pipeline for building and releasing x86 CPU wheels and correctly separates them into a vllm-wheels/cpu path in the S3 bucket. The changes are well-structured and mostly correct. However, I found a significant issue in the upload-wheels.sh script where it copies an incorrect index file for CPU wheel builds. This could lead to installation problems for users. I've provided a specific comment with a suggested fix to address this.
.buildkite/scripts/upload-wheels.sh
Outdated
| aws s3 cp "s3://vllm-wheels/nightly/index.html" "s3://vllm-wheels/$BUILDKITE_COMMIT/index.html" | ||
| # also upload cpu wheels as is available on both x86 and arm64 | ||
| aws s3 cp index.html "s3://$ROOT_PATH/$BUILDKITE_COMMIT/vllm/index.html" | ||
| aws s3 cp "s3://vllm-wheels/nightly/index.html" "s3://$ROOT_PATH/$BUILDKITE_COMMIT/index.html" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an issue with this aws s3 cp command. For CPU wheel builds, $ROOT_PATH will be vllm-wheels/cpu, but the source S3 path is hardcoded to s3://vllm-wheels/nightly/index.html, which is the index for CUDA wheels. This will result in copying the CUDA wheel index into the CPU wheel commit-specific directory, leading to incorrect package resolution for users.
The source path should also use the $ROOT_PATH variable to ensure the correct index is copied for CPU builds. Additionally, it's good practice to handle the case where the nightly index might not exist yet (e.g., for the first build of a day) to prevent the step from failing.
| aws s3 cp "s3://vllm-wheels/nightly/index.html" "s3://$ROOT_PATH/$BUILDKITE_COMMIT/index.html" | |
| aws s3 cp "s3://$ROOT_PATH/nightly/index.html" "s3://$ROOT_PATH/$BUILDKITE_COMMIT/index.html" || true |
|
Hi @khluu Could you please help to check this PR? Thanks! |
|
The changes on |
|
Thanks a lot @khluu The CPU wheel index is valid. For CUDA there should have no change. |
.buildkite/scripts/upload-wheels.sh
Outdated
| ROOT_PATH="vllm-wheels" | ||
|
|
||
| if [[ $version == *cpu* ]]; then | ||
| ROOT_PATH="$ROOT_PATH/cpu" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about the refactor recently. We should add directories inside s3://vllm-wheels/$BUILDKITE_COMMIT/, e.g. s3://vllm-wheels/$BUILDKITE_COMMIT/cpu/ .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Now CPU wheels and index will be uploaded to s3://vllm-wheels/$BUILDKITE_COMMIT/cpu/ and s3://vllm-wheels/$BUILDKITE_COMMIT/cpu/vllm
35ba6c1 to
1fb3af0
Compare
Signed-off-by: jiang1.li <jiang1.li@intel.com>
Purpose
Seperate CPU wheels (both x86 and Arm) tovllm-wheels/cpuAfter #29838 merged, we only need to add a x86 CPU release pipeline.
With this change, CPU wheels can be installed via:
Test Plan
verified the scripts locally
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.