-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[CI] fix docker image build by specifying merge-base commit id when downloading pre-compiled wheels #29930
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
…d python-only build test Signed-off-by: Shengqi Chen <harry-chen@outlook.com>
Signed-off-by: Shengqi Chen <harry-chen@outlook.com>
Signed-off-by: Shengqi Chen <harry-chen@outlook.com>
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 correctly addresses an issue with downloading pre-compiled wheels during CI builds by using the merge-base commit. The overall approach is sound. However, I've found a critical issue in one of the updated test scripts that will cause it to fail. Please see the detailed comment for the fix.
Signed-off-by: Shengqi Chen <harry-chen@outlook.com>
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Shengqi Chen <harry-chen@outlook.com>
| @@ -1,46 +0,0 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
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.
is this file not used anymore?
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.
Yes, this file is replaced by .buildkite/scripts/generate_nightly_index.py
Purpose
Currently the docker image building stage in CI will try to download prebuilt wheels from the latest HEAD commit on the main branch:
vllm/setup.py
Lines 512 to 514 in 506ed87
However, this might lead to 404 error when the main branch is too new (e.g. it taks ~30min to build a wheel), especially when PR author rebases it to the latest main.
Before PR #29838 (4b61266) was merged, the wheel downloading mechanism in
setup.pywill fallback to/nightly/if 404 occurrs:vllm/setup.py
Lines 674 to 680 in 7c1ed45
This was not safe, because
/nightly/might be newer than the merge-base of the current PR, and have native code changes, causing their.sofiles to change.This PR passes the merge base commit to the Docker build process (and Python-only build test) in CI, so it could download the correct wheels. The
VLLM_MERGE_BASE_COMMITis provided by buildkite bootstrap script in vllm/ci-infra repo.Test Plan
Tested by CI.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.