fix(container): backport vllm PR #40932 to unblock Qwen3-VL disagg multimodal - #9491
Conversation
…ltimodal
PR #40932 ([Bugfix] Remove invalid deepstack boundary check for Qwen3-VL)
was merged into upstream vLLM `main` and shipped on the `v0.20.1rc0` tag,
but it was dropped between rc0 and the final `v0.20.1` tag. Both Dynamo
CUDA images currently pin `vllm_ref: v0.20.1`, so the 1.2.0rc0 image
inherits the regression: any disaggregated multimodal request to a
Qwen3-VL model crashes the EngineCore on first inference with
ValueError: Requested more deepstack tokens than available in buffer:
num_tokens=288 > self.deepstack_input_embeds_num_tokens=276
The 12-token gap is cudagraph capture-bucket padding (276 -> 288 = next
captured size). The strict check that trips was added by upstream
#40145; upstream #40932 simply removes it because the underlying
`[:num_tokens]` indexing already handles the case correctly. Aggregated
multimodal is unaffected because it never parks requests in
WAITING_FOR_EMBEDDINGS.
A clean vllm_ref bump isn't an option: `v0.20.1rc0` predates `v0.20.1`
on the release branch (would regress unrelated commits), `main` carries
many unrelated changes, and there's no patched-v0.20.1 tag.
Instead, add a small per-vllm-ref patch directory under
`container/deps/vllm/patches/<ref>/`. `install_vllm.sh` applies any
*.patch in there to the vLLM site-packages directly after
`uv pip install vllm` succeeds. When the upstream wheel is bumped past
this fix the directory becomes inert automatically (no matching ref
subdir), so it self-cleans rather than rotting.
Verified locally: applied the same patch to the `vllm-runtime` image,
rebuilt Dynamo, ran `disagg_multimodal_e_pd.sh` with `Qwen3-VL-2B-Instruct`
on 2 GPUs with cudagraph fully enabled (the size-288 bucket was
captured), and sent three image+text chat requests. All returned 200
with correct image descriptions; zero `Requested more deepstack` or
`EngineCore encountered a fatal error` lines in worker logs.
Signed-off-by: Qi Wang <qiwa@nvidia.com>
Adds an explicit upper-bound guard to the patches step in
install_vllm.sh. When a future maintainer bumps vllm_ref past v0.20.1
to a release that already contains PR #40932 upstream (expected by
v0.21.0), the patches block now no-ops with a clear log line:
vLLM 0.21.0 >= 0.21 - skipping vendored patches (assumed merged upstream)
Previously the script relied only on `patches/<VLLM_REF>/` directory
existence: bumping the ref made the patch inert via name mismatch, but
nothing prevented someone from re-creating a `patches/v0.21.0/` dir
without first checking whether the fix had landed.
rc / post / dev / + suffixes are stripped before comparison (e.g.
0.21.0rc0 also trips the skip). Non-semver refs (commit SHAs, branch
names like `main`) fall through the regex guard and use only the
directory-existence check.
Signed-off-by: Qi Wang <qiwa@nvidia.com>
WalkthroughThe PR introduces a mechanism to apply vendored vLLM patches during container build and includes a bugfix patch that removes invalid boundary checks in Qwen3-VL model code. The installation script conditionally applies patches based on vLLM version, skipping v0.21.0 and later, while a new patch file targets two Qwen3 model classes. ChangesvLLM Patch Infrastructure and Qwen3-VL Fix
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
container/deps/vllm/install_vllm.sh (1)
264-264: ⚡ Quick winDerive
PATCH_DIRfrom the script location instead of hard-coding/tmp/deps.This path assumption is brittle; if the image layout changes, patching will be skipped and the regression can reappear.
Proposed change
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -PATCH_DIR="/tmp/deps/vllm/patches/${VLLM_REF}" +PATCH_DIR="${SCRIPT_DIR}/patches/${VLLM_REF}"Based on learnings: "Flag hard-coded portability-reducing constants in shell/scripts across the repository (e.g., fixed memory sizes, fixed memory fractions, static ports, hard-coded temporary file names)."
🤖 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 `@container/deps/vllm/install_vllm.sh` at line 264, The PATCH_DIR is hard-coded to /tmp/deps which breaks portability; change how PATCH_DIR is computed in install_vllm.sh to derive it from the script's location instead (use the script directory as the base and join "patches/${VLLM_REF}" to it), e.g., compute the script base with dirname/BASH_SOURCE (or an equivalent portable approach), then set PATCH_DIR="${script_base}/patches/${VLLM_REF}" and ensure the directory is created/validated before use so patching works regardless of image layout.
🤖 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 `@container/deps/vllm/install_vllm.sh`:
- Around line 271-274: The patch command in the for loop that iterates over
"$PATCH_DIR"/*.patch can prompt interactively on mismatches and hang builds;
update the invocation of patch (the line using patch -p1 -d
"$VLLM_SITE_PACKAGES" < "$p") to run non-interactively by adding the --batch and
--forward flags so it suppresses prompts and ignores already-applied or reversed
patches.
---
Nitpick comments:
In `@container/deps/vllm/install_vllm.sh`:
- Line 264: The PATCH_DIR is hard-coded to /tmp/deps which breaks portability;
change how PATCH_DIR is computed in install_vllm.sh to derive it from the
script's location instead (use the script directory as the base and join
"patches/${VLLM_REF}" to it), e.g., compute the script base with
dirname/BASH_SOURCE (or an equivalent portable approach), then set
PATCH_DIR="${script_base}/patches/${VLLM_REF}" and ensure the directory is
created/validated before use so patching works regardless of image layout.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c25ac7e-a80e-46bf-b559-f47d8e663aa7
📒 Files selected for processing (2)
container/deps/vllm/install_vllm.shcontainer/deps/vllm/patches/v0.20.1/0001-pr40932-remove-invalid-deepstack-boundary-check-for-qwen3-vl.patch
|
Quick Question |
This errors shows up in cudagraph.
|
The previous version of the patches block ran `python3 -c "import vllm, os; print(...)"` to locate the site-packages dir, but at the point this block executes the install script's cwd is still `$INSTALLATION_DIR/vllm` (from the earlier `cd vllm` for the upstream clone). That clone contains a `vllm/` Python package on disk, so cwd-on-sys.path shadows the venv, `vllm.__file__` resolves to `/opt/vllm/vllm/__init__.py`, and the patch lands on the source tree that's only used for DeepGEMM/EP-kernel install scripts — not on the installed wheel that the runtime actually imports. The runtime workload (`python -m dynamo.vllm`, `python -m dynamo.frontend`, K8s entrypoints) never runs with cwd=/opt/vllm, so `import vllm` there resolves to the venv wheel. With the patch sitting in the unused source clone, the engine kept the buggy strict check and the disagg multimodal crash returned at deploy time despite a "successful" patch step in the build log. Fix: `cd /` before the python lookup so cwd-on-sys.path can't shadow the installed distribution. Same pattern is already used in the post-install verification block above (line 191). Caught by reviewer feedback on PR #9491. Re-tested locally: with cwd=/, `vllm.__file__` correctly resolves to `/opt/dynamo/venv/lib/python3.12/site-packages/vllm/__init__.py`, and the patch lands on the file the runtime actually imports. Signed-off-by: Qi Wang <qiwa@nvidia.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
please apply copyright/license comment
|
Thanks for the fix, Qi! If you haven't done this, could we add some ticket to remove this patch when vLLM ships 0.20.2 with #40932 backported? |
…d-deepstack-boundary-check-for-qwen3-vl.patch Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
`patch` without --batch will prompt `File to patch:` on stdin when a hunk's path doesn't match. In an unattended `docker build` there is no stdin, so the build hangs until the operator's idle timeout kills it. --batch suppresses the prompt and exits immediately on mismatch. --forward additionally lets re-runs (or wheels that already carry the fix — e.g. a hypothetical v0.20.1.post1 that lands #40932) succeed silently instead of erroring out on a "Reversed (or previously applied)" diagnosis. GNU patch exit codes: 0 = clean apply, 1 = some hunks rejected or skipped via --forward, >=2 = fatal. We accept rc=1 (so the patches block doesn't become a brittle build gate on benign skip cases) and propagate >=2. Caveat: rc=1 also covers "target file not found at all," so a future patch with a wrong path would silently no-op. For the current PR-#40932 patch the targets (qwen3_vl.py, qwen3_omni_moe_thinker.py) exist in every plausible vLLM version, so this conflation isn't a live concern. If future vendored patches make this risk material, swap the tolerance for a `patch --dry-run --reverse` pre-check that distinguishes already-applied from broken. Verified locally: timeout-wrapped `patch --batch --forward` against an empty target tree exits rc=1 in <5 ms instead of blocking on stdin. Signed-off-by: Qi Wang <qiwa@nvidia.com>
Why
The vllm-runtime image pins
vllm_ref: v0.20.1, which is missing upstream vllm-project/vllm#40932 ([Bugfix] Remove invalid deepstack boundary check for Qwen3-VL). That fix removes a strict size check originally added by vllm-project/vllm#40145 ([Opt] Optimize deepstack buffer handling for multimodal Qwen3 models). #40932 landed onmainand shipped on thev0.20.1rc0tag, but it was dropped between rc0 and the finalv0.20.1tag — so the 1.2.0rc0 image inherits the regression.Symptom: the EngineCore crashes on the first inference of any disaggregated multimodal request to a Qwen3-VL model:
ValueError: Requested more deepstack tokens than available in buffer: num_tokens=288 > self.deepstack_input_embeds_num_tokens=276— the gap is cudagraph capture-bucket padding the scheduler can't avoid.A clean
vllm_refbump isn't an option:v0.20.1rc0predatesv0.20.1on the release branch (regresses unrelated commits),maincarries unrelated changes, and there's no patched-v0.20.1 tag.What Change
container/deps/vllm/patches/<ref>/directory0001-pr40932-...patchunderpatches/v0.20.1/install_vllm.shapplies any matching patches after the PyPI wheel install; skipped whenVLLM_VER >= 0.21.0(assumes upstream merges by then)Test Plan
disagg_multimodal_e_pd.sh --model Qwen/Qwen3-VL-2B-Instructon 2 GPUs with cudagraph active (bucket 288 captured)Requested more deepstack/EngineCore encountered a fatal errorlines in logsSummary by CodeRabbit
Bug Fixes
Chores