[WIP][Rebase] Align vLLM main branch with vLLM Omni main branch#1003
[WIP][Rebase] Align vLLM main branch with vLLM Omni main branch#1003tzhouam wants to merge 0 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed8b03224b
ℹ️ 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".
| else \ | ||
| echo "ERROR: set VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL"; \ | ||
| exit 1; \ |
There was a problem hiding this comment.
Avoid hard-failing when build args are unset
The new else branch exits the build unless VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL is provided. In the current Buildkite pipeline, the CI image is built with docker build --file docker/Dockerfile.ci -t vllm-omni-ci . (see .buildkite/pipeline.yml line 6) and no --build-arg flags are passed, so this change will make the image build (and thus the entire CI pipeline) fail by default. If the nightly install is optional, consider keeping the base image’s vLLM when args are unset or updating the pipeline to pass the required args.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements a new vLLM version management strategy for the CI Docker image as part of the vLLM-Omni alignment effort described in RFC #996. The changes allow the CI Docker image to dynamically install a specific vLLM version (either from a nightly index or a wheel URL) instead of relying solely on the version pinned in the base image.
Changes:
- Restructured Dockerfile.ci to support dynamic vLLM version installation via build arguments
- Added logic to uninstall the base image's vLLM and install a specified nightly or wheel version
- Improved Docker layer organization by separating system dependencies, vLLM installation, and package installation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "ERROR: set VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL"; \ | ||
| exit 1; \ |
There was a problem hiding this comment.
The Dockerfile now requires either VLLM_NIGHTLY_INDEX_URL or VLLM_WHEEL_URL to be set as build arguments, but the buildkite pipeline at line 6 of .buildkite/pipeline.yml does not pass any build arguments. This will cause the Docker build to fail with the error "ERROR: set VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL" and exit 1.
The docker build command needs to be updated to include at least one of these build arguments, for example:
- docker build --build-arg VLLM_NIGHTLY_INDEX_URL=https://example.com/nightly --file docker/Dockerfile.ci -t vllm-omni-ci .
or - docker build --build-arg VLLM_WHEEL_URL=https://example.com/wheel.whl --file docker/Dockerfile.ci -t vllm-omni-ci .
| echo "ERROR: set VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL"; \ | |
| exit 1; \ | |
| echo "No VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL provided; installing stable vllm from default index."; \ | |
| uv pip install --python "$PY" --no-cache-dir -U vllm; \ |
| --extra-index-url "$VLLM_NIGHTLY_INDEX_URL" \ | ||
| vllm; \ | ||
| else \ | ||
| echo "ERROR: set VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL"; \ |
There was a problem hiding this comment.
The error message "ERROR: set VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL" is unclear about how to set these variables. It should specify that these are Docker build arguments that need to be provided using --build-arg flag during docker build.
Consider improving the error message to: "ERROR: VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL must be provided as build arguments (--build-arg)"
| echo "ERROR: set VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL"; \ | |
| echo "ERROR: VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL must be provided as build arguments (--build-arg)"; \ |
| PY="$(python3 -c 'import sys; print(sys.executable)')"; \ | ||
| uv pip uninstall --python "$PY" -y vllm || true; \ | ||
| if [ -n "$VLLM_WHEEL_URL" ]; then \ | ||
| uv pip install --python "$PY" --no-cache-dir "$VLLM_WHEEL_URL"; \ | ||
| elif [ -n "$VLLM_NIGHTLY_INDEX_URL" ]; then \ | ||
| uv pip install --python "$PY" --no-cache-dir -U --pre \ | ||
| --extra-index-url "$VLLM_NIGHTLY_INDEX_URL" \ | ||
| vllm; \ | ||
| else \ | ||
| echo "ERROR: set VLLM_WHEEL_URL or VLLM_NIGHTLY_INDEX_URL"; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| python3 -c "import vllm; print('vllm installed:', getattr(vllm,'__version__','<unknown>'))" |
There was a problem hiding this comment.
The variable PY is used to store the Python executable path, but it's defined within a multi-line RUN command that uses set -eux. If the python3 command fails or returns an unexpected value, the error handling might not be clear. Consider adding error handling or a comment explaining why this approach is safe.
Additionally, the variable is quoted in some places ("$PY") but the pattern is inconsistent with how python3 is called directly in line 37. For consistency and safety, consider using "$PY" throughout or explaining why direct python3 calls are used later.
|
|
||
| # Install vllm-omni into the same uv-managed Python environment used by the base image. | ||
| # Copy after system deps (optional, but keeps layers cleaner) | ||
| COPY . . |
There was a problem hiding this comment.
The code is copied to the working directory at line 15 before the vLLM nightly installation (lines 24-37), but the vLLM installation doesn't actually use the copied code. This means the Docker layer cache will be invalidated whenever any file in the repository changes, even if only the vLLM installation parameters change.
Consider moving the COPY command to after line 37 (right before line 40 where the copied code is actually used) to improve Docker layer caching. The comment on line 14 acknowledges this with "(optional, but keeps layers cleaner)" but the current order actually makes layers less clean and more cache-inefficient.
| # Provide ONE of these at build time: | ||
| # - VLLM_NIGHTLY_INDEX_URL (preferred) OR | ||
| # - VLLM_WHEEL_URL (explicit wheel) | ||
| ARG VLLM_NIGHTLY_INDEX_URL= | ||
| ARG VLLM_WHEEL_URL= | ||
|
|
||
| RUN set -eux; \ | ||
| PY="$(python3 -c 'import sys; print(sys.executable)')"; \ | ||
| uv pip uninstall --python "$PY" -y vllm || true; \ | ||
| if [ -n "$VLLM_WHEEL_URL" ]; then \ | ||
| uv pip install --python "$PY" --no-cache-dir "$VLLM_WHEEL_URL"; \ | ||
| elif [ -n "$VLLM_NIGHTLY_INDEX_URL" ]; then \ |
There was a problem hiding this comment.
The priority logic gives precedence to VLLM_WHEEL_URL over VLLM_NIGHTLY_INDEX_URL (checked first in the if statement). However, the comment on lines 18-20 states that VLLM_NIGHTLY_INDEX_URL is "preferred", which conflicts with the actual implementation priority.
Either update the comment to match the implementation (VLLM_WHEEL_URL has priority), or swap the if/elif conditions to check VLLM_NIGHTLY_INDEX_URL first if it's truly preferred.
|
@tzhouam can you update vllm-omni/docker/Dockerfile.rocm Line 1 in 08fa85a 0.15.0 as well?
|
| RUN uv pip install --python "$(python3 -c 'import sys; print(sys.executable)')" --no-cache-dir ".[dev]" | ||
| # Install vLLM wheel for the specific commit | ||
| # URL format: https://wheels.vllm.ai/<commit_hash>/ | ||
| # Current vLLM commit: f176443446f659dbab5315e056e605d8984fd976 (v0.15.0) |
There was a problem hiding this comment.
@tzhouam must we use this? ROCm does not have nightly builds. If we want to do this I will need modify the dockerfile to help support this by rebuilding the vllm from source.
|
@amy-why-3459 PTAL for these bug from qwen-omni to see whether we have resolved these |
| defaults: {window_size: -1, max_inflight: 1} | ||
| edges: | ||
| - {from: 0, to: 1, window_size: -1} | ||
| - {from: 1, to: 2, window_size: -1} |
There was a problem hiding this comment.
May I ask why do we need these configs?
| _ic_cache = getattr(transfer_manager, "_cached_ic", None) | ||
| if _ic_cache is None: | ||
| _ic_cache = {} | ||
| transfer_manager._cached_ic = _ic_cache |
There was a problem hiding this comment.
It seems to be unrelated to upgrading. Not sure whether the original code is redundant. I think we need to have a double-check. cc @linyueqian
There was a problem hiding this comment.
Please remove this image.
THIS PR SHOULD NEVER BE MERGED
Purpose
As RFC #996 , we are creating a new branch as dev/vllm-align to align the vllm main branch and vllm omni main branch.
This PR is to trigger the CI Test ensuring that there is no trival bugs for the rebase.
Test Plan
Going to develop a test pipeline to ensure all examples can get correst result.
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)