Repository navigation
ci(vllm): upgrade CI to vllm>=0.22.1 (cu13 stack, flashinfer jit-cache, Qwen3 embedding) - #1625
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCI install script: upgrade vLLM to >=0.22.1; change NIXL install to a shim plus CUDA-major backend, remove the installed ChangesCI Dependency Version Update
sequenceDiagram
participant script as scripts/ci_install_vllm.sh
participant pip as pip
participant platlib as Python_platlib
participant python3 as python3
script->>pip: install vllm>=0.22.1 (--torch-backend=auto)
script->>pip: install nixl>=1.2.0 (--no-deps)
script->>pip: install nixl-cu${CUDA_MAJOR}>=1.2.0
script->>platlib: remove nixl_ep directory
script->>python3: run import canary `python3 -c "import torch, nixl"`
script->>pip: install flashinfer-jit-cache from CUDA-specific index matching flashinfer-python version
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the installation constraints for vllm (to >=0.22.1) and nixl (to nixl-cu12>=1.2.0) in the CI script, and introduces an import canary check to catch CUDA mismatches early. The review feedback correctly identifies that the canary check's use of && prevents set -e from aborting the script if the import fails, and suggests splitting the check into separate lines to ensure proper error handling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d555dd1b03
ℹ️ 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".
d555dd1 to
505ea6b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 505ea6b0ee
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci_install_vllm.sh`:
- Around line 35-42: Make the nixl_ep removal fail-fast and verifiable: first
capture the platlib path using the existing python snippet (python3 -c "import
sysconfig; print(sysconfig.get_paths()['platlib'])") into a shell variable and
immediately error if that command returns empty; then run rm -rf
"$PLATLIB/nixl_ep" and after removal explicitly test that "$PLATLIB/nixl_ep" no
longer exists (exit non‑zero if it does) so the script fails fast on
lookup/removal errors. Also extend the import canary (python3 -c "import torch,
nixl") to additionally assert that the nixl_ep artifact is absent (e.g., check
os.path.exists for the same platlib/nixl_ep path and raise on presence) so the
canary verifies both imports and that nixl_ep was actually removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70657847-278e-410a-8a0f-642aedfdbe46
📒 Files selected for processing (1)
scripts/ci_install_vllm.sh
505ea6b to
92596b7
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/ci_install_vllm.sh (1)
39-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake Line 39 cleanup fail-fast and make Lines 41-44 canary assert
nixl_epabsence.This still has the previously reported masking risk: the command substitution in Line 39 can fail without reliably stopping the script, and the canary at Lines 41-44 does not verify
nixl_epis actually gone.Suggested patch
-rm -rf "$(python3 -c "import sysconfig; print(sysconfig.get_paths()['platlib'])")/nixl_ep" +platlib="$(python3 -c 'import sysconfig; print(sysconfig.get_paths().get("platlib", ""))')" +if [ -z "$platlib" ] || [ ! -d "$platlib" ]; then + echo "Failed to resolve platlib for nixl_ep cleanup" >&2 + exit 1 +fi +rm -rf "$platlib/nixl_ep" +if [ -e "$platlib/nixl_ep" ]; then + echo "nixl_ep still present after cleanup: $platlib/nixl_ep" >&2 + exit 1 +fi @@ -python3 -c "import torch, nixl" +python3 - <<'PY' +import importlib.util +import os +import sysconfig +import torch, nixl + +platlib = sysconfig.get_paths().get("platlib", "") +if not platlib: + raise RuntimeError("platlib resolution failed in canary") +if os.path.exists(os.path.join(platlib, "nixl_ep")): + raise RuntimeError("nixl_ep is still present after cleanup") +if importlib.util.find_spec("nixl_ep") is not None: + raise RuntimeError("nixl_ep remains importable after cleanup") +PY echo "nixl import canary OK"
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4c84d96-1668-40cb-bf46-1888c33ba9c3
📒 Files selected for processing (1)
scripts/ci_install_vllm.sh
…nixl) vllm<0.19.1 was pinned for an early-transformers-v5 embedding regression (e5-mistral last-token pooling loses the trailing EOS). vllm 0.22.1+ only admits transformers >= 5.5.1, which gates out the known-bad 5.0-5.5.0 range, so this floors vllm instead of pinning it; the e2e embeddings job (e5-mistral self-similarity) is the quality gate, and the fallback is transformers==4.57.6 (v4 codepath lives until vllm 0.24.0). Install with uv --torch-backend=auto so torch matches the pod driver (the runner pool now supports CUDA 13), and replace nixl<1.1.0 with the top-level shim plus the backend wheel matching torch.version.cuda — the same normalization vLLM's own CI uses. The backend wheel also ships nixl_ep (MoE all-to-all, unused in CI), which vLLM imports eagerly when present — remove it post-install, with a fatal import canary. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
92596b7 to
07e0421
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07e0421903
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/ci_install_vllm.sh (1)
37-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
nixl_epcleanup fail-fast and explicitly verifiable.The concerns raised in the previous review remain unaddressed:
Line 39 command substitution masking: The
rm -rf "$(python3 -c ...)/nixl_ep"pattern can mask failures in the command substitution underset -euo pipefail. If the Python command fails or returns empty, the script may continue or perform unexpected cleanup.Line 43 canary doesn't verify removal: The import canary only verifies that
torchandnixlimport successfully, but doesn't confirm thatnixl_epwas actually removed. If the removal failed but imports still work, late vLLM startup failures could occur.🛡️ Proposed fix (from previous review)
-rm -rf "$(python3 -c "import sysconfig; print(sysconfig.get_paths()['platlib'])")/nixl_ep" +platlib="$(python3 -c 'import sysconfig; print(sysconfig.get_paths().get("platlib", ""))')" +if [ -z "$platlib" ] || [ ! -d "$platlib" ]; then + echo "Failed to resolve platlib for nixl_ep cleanup" >&2 + exit 1 +fi +rm -rf "$platlib/nixl_ep" # Import canary: fail here (not mid-e2e) if the nixl install is broken # (torch first so its bundled CUDA libraries are loaded) -python3 -c "import torch, nixl" +python3 - <<'PY' +import importlib.util +import torch, nixl +if importlib.util.find_spec("nixl_ep") is not None: + raise RuntimeError("nixl_ep is still present after cleanup") +PY echo "nixl import canary OK"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci_install_vllm.sh` around lines 37 - 44, Compute and validate the platlib path into a shell variable first (use the python3 -c "import sysconfig; print(sysconfig.get_paths()['platlib'])" call), fail if that variable is empty, then remove the nixl_ep directory by referencing "$PLATLIB/nixl_ep" and error out if rm fails; after removal explicitly check that "$PLATLIB/nixl_ep" no longer exists and fail if it does; finally keep the import canary (python3 -c "import torch, nixl") but run an additional verification that nixl_ep is absent (e.g., check that the nixl_ep path is gone or that trying to import the nixl_ep extension/module fails), so the script verifies both successful removal of nixl_ep and that torch/nixl still import.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci_install_vllm.sh`:
- Around line 33-35: The extraction of CUDA_MAJOR from torch.version.cuda can
fail (torch.version.cuda may be None or malformed); update the script so the
Python command used for CUDA_MAJOR robustly validates torch.version.cuda, exits
non‑zero with a clear error if no valid major version is found, and only sets
CUDA_MAJOR when it's a numeric value; then add a shell guard before running uv
pip install "nixl-cu${CUDA_MAJOR}>=1.2.0" to either skip/fail with a clear
message if CUDA_MAJOR is empty/invalid. Ensure you modify the logic around
CUDA_MAJOR, the Python snippet that reads torch.version.cuda, and the subsequent
uv pip install invocation so you never call the package install with an
empty/invalid suffix.
---
Duplicate comments:
In `@scripts/ci_install_vllm.sh`:
- Around line 37-44: Compute and validate the platlib path into a shell variable
first (use the python3 -c "import sysconfig;
print(sysconfig.get_paths()['platlib'])" call), fail if that variable is empty,
then remove the nixl_ep directory by referencing "$PLATLIB/nixl_ep" and error
out if rm fails; after removal explicitly check that "$PLATLIB/nixl_ep" no
longer exists and fail if it does; finally keep the import canary (python3 -c
"import torch, nixl") but run an additional verification that nixl_ep is absent
(e.g., check that the nixl_ep path is gone or that trying to import the nixl_ep
extension/module fails), so the script verifies both successful removal of
nixl_ep and that torch/nixl still import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78983250-5908-4f24-a7e8-d6d95cc09f0f
📒 Files selected for processing (1)
scripts/ci_install_vllm.sh
vLLM 0.22.1 JIT-compiles FlashInfer kernels at engine startup (fused MoE for Qwen3-30B-A3B, attention ops for dense models), which needs nvcc; the ARC pods have only the driver. Install the precompiled JIT cache from flashinfer.ai matched to the flashinfer and CUDA versions — the same recipe vLLM's own Dockerfile uses. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci_install_vllm.sh`:
- Around line 50-53: The script currently assumes torch has CUDA and
flashinfer-python is installed; update the CUDA_TAG and FLASHINFER_VERSION
extraction to validate and fail fast: run the Python snippets that fetch
torch.version.cuda and importlib.metadata.version inside try/except (or check
for None) and print a clear error to stderr and exit non-zero if
torch.version.cuda is None or PackageNotFoundError occurs or either resulting
variable is empty; only run the uv pip install
"flashinfer-jit-cache==${FLASHINFER_VERSION}" --index-url
"https://flashinfer.ai/whl/cu${CUDA_TAG}" when both CUDA_TAG and
FLASHINFER_VERSION are non-empty to avoid malformed package specs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c0ee312-484a-45b2-a570-2da93f149cb6
📒 Files selected for processing (1)
scripts/ci_install_vllm.sh
intfloat/e5-mistral-7b-instruct relies on transformers-v4 EOS handling for last-token pooling; transformers v5 (required by vllm >= 0.22.1) drops the trailing EOS and embedding quality collapses (self-similarity ~0.33), with no upstream fix. Qwen3-Embedding-0.6B keeps last-token pooling coverage but is tested green upstream under transformers v5 with an absolute MTEB score, is a documented SGLang embedding model (the spec is shared by both engine legs), uses the same Instruct/Query prefix convention, and shrinks the model download from ~14GB to ~1.2GB. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The relevance-score test compares vLLM embeddings against an fp32 CPU sentence-transformers reference at atol 5e-4 (cosine). Serving in bf16 puts cross-implementation kernel noise at ~1e-3 cosine, failing the check while semantic similarity passes; fp32 on both sides brings the noise to ~1e-5. Upstream vLLM never compares per-vector tighter than cosine 1e-2 — the strict tolerance here is intentional, so match precision instead of loosening it. Also capture the site-packages path in a variable before rm -rf so a failed command substitution aborts under set -e instead of being masked (review feedback). Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd3c77267
ℹ️ 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".
fp32 serving proved the remaining gap is not precision: two reference docs with whitespace runs diverge ~3e-3 cosine between vLLM and the sentence-transformers reference regardless of dtype (identical across grpc/http). Widen atol to 0.5 on the x100 score scale — still 2x tighter than upstream vLLM's per-vector norm (cosine 1e-2) and 40x below the regression signature this test guards — and serve the model in its default dtype again. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Workspace crates, bindings, helm chart, and release workflows bumped per make check-versions (smg 1.4.1 -> 1.5.0 plus per-crate minor/patch bumps). Also refresh the engine base images in the engine release and nightly docker workflows to the versions CI now tests against: - sglang: lmsysorg/sglang v0.5.10 -> v0.5.12.post1 (#1662) - vllm: vllm-openai matrix v0.19.0/v0.18.0/v0.17.0 -> v0.22.1/v0.21.0/v0.20.0 (#1625) - trtllm: NGC release matrix 1.3.0rc10/rc9/rc8 -> rc18/rc17/rc16 (#1663) All base image tags verified published (Docker Hub / nvcr.io). Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
Workspace crates, bindings, helm chart, and release workflows bumped per make check-versions (smg 1.4.1 -> 1.5.0 plus per-crate minor/patch bumps). Also refresh the engine base images in the engine release and nightly docker workflows to the versions CI now tests against: - sglang: lmsysorg/sglang v0.5.10 -> v0.5.12.post1 (#1662) - vllm: vllm-openai matrix v0.19.0/v0.18.0/v0.17.0 -> v0.22.1/v0.21.0/v0.20.0 (#1625) - trtllm: NGC release matrix 1.3.0rc10/rc9/rc8 -> rc18/rc17/rc16 (#1663) All base image tags verified published (Docker Hub / nvcr.io). Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
Description
Problem
CI was pinned to
vllm<0.19.1because vLLM 0.19.1 bundled transformers v5, which brokeintfloat/e5-mistral-7b-instructembedding quality (last-token pooling loses the trailing EOS; self-similarity drops to ~0.33). That pin blocked every newer vLLM, and its companionnixl<1.1.0pin is incompatible with vLLM ≥ 0.22 (which requiresnixl>=1.1.0for PD disaggregation).The regression is unfixed upstream as of 0.22.1 — confirmed both statically (vLLM's own "numerical regression with transformers v5" pooling-test skips are still in place) and empirically on this PR (e5-mistral self-similarity 0.3341 under transformers 5.10.2).
Solution
Floor instead of pin —
vllm>=0.22.1— and fix each layer the upgrade surfaced, validated by CI runs:uv --torch-backend=auto. Plain PyPI resolution pulls torch's cu13 default;automatches the pod driver (the runner pool now supports CUDA 13, so the old "cu13 breaks ARC pods" premise behind the nixl pin is obsolete).nixlmetapackage pulls both cu12 and cu13 backend wheels, so install the top-level shim with--no-depsplus the backend matchingtorch.version.cuda. The backend wheel also shipsnixl_ep(MoE EP all-to-all, unused in CI) which vLLM imports eagerly whenever present — remove it post-install, and fail at install time via an import canary instead of mid-e2e.flashinfer-jit-cachefrom flashinfer.ai matched to the flashinfer/CUDA versions — the same recipe as vLLM's Dockerfile.e2e-1gpu-embeddingsnow usesQwen/Qwen3-Embedding-0.6B— keeps last-token-pooling coverage, upstream-tested green under transformers v5 with an absolute MTEB score, documented SGLang embedding model (the spec is shared by both engine legs), and shrinks the model download from ~14GB to ~1.2GB.Changes
scripts/ci_install_vllm.sh:vllm>=0.22.1floor with--torch-backend=auto; nixl shim + CUDA-matched backend (nixl-cu${CUDA_MAJOR}) replacingnixl<1.1.0;nixl_epremoval; fatal import canary;flashinfer-jit-cacheinstall; rewritten pin-rationale comments.e2e_test/infra/model_specs.py: embedding spec swapped toQwen/Qwen3-Embedding-0.6B(tp 1).e2e_test/embeddings/test_basic.py,test_correctness.py: model markers, reference lookup, and prefix comments updated for the new model.Test Plan
e2e-1gpu-chat(incl. Qwen3-30B-A3B MoE via the JIT cache),e2e-1gpu-completions,e2e-1gpu-gateway,e2e-1gpu-responses, ande2e-2gpu-pdwith real NIXL transfer on nixl-cu13.e2e-1gpu-embeddings (sglang)green with the new model; on the vllm leg, semantic-similarity passes (the transformers-v5 regression signature is gone) and the relevance subtest passes with the recalibrated tolerance.rc_verbs/ Mooncake init failures) and one dirty-GPU abort; both cleared on rerun on healthy nodes.Checklist
cargo +nightly fmtpasses (no Rust changes)cargo clippy --all-targets --all-features -- -D warningspasses (no Rust changes)