Update vLLM model wrapper, Qwen3 VL patcher, and vision tower JIT for… - #3283
Update vLLM model wrapper, Qwen3 VL patcher, and vision tower JIT for…#3283amitkumar307d wants to merge 1 commit into
Conversation
82a071b to
9d03b81
Compare
b092030 to
f11adc9
Compare
… video performance Signed-off-by: Amit Kumar <amitmkumar@google.com>
f11adc9 to
5d610ad
Compare
QiliangCui
left a comment
There was a problem hiding this comment.
Thanks — the problems this targets (host/HBM replication OOM on large video inputs, VMEM OOM in vision-tower SDPA, cold video compiles) are real, and the pixel-shard + strip-after-forward design and the GridTHW pytree/slice changes look sound. I reviewed this against the vLLM commit this repo pins (.buildkite/vllm_lkg.version = 0ba2aa35a), and there are three things that need to be addressed before this can merge, plus inline comments.
1. The mm receiver-cache integration (item 4 in the description) doesn't function at the pinned vLLM. engine_receiver_cache_from_config is called with a ModelConfig where it takes the full VllmConfig → AttributeError at init of both disagg engine classes (any model, only under PREFILL_SLICES, which PR CI doesn't run). The application loops in add_request / execute_model check mm_inputs / mm_positions, which don't exist on the pinned request/scheduler types (only mm_features does), and _apply_mm_cache only exists on vLLM's WorkerWrapperBase, not on TPUWorker — so every branch is dead code. Details inline.
2. No unit tests. ~400 lines of production change across 5 modules with no test coverage — this repo requires unit tests for tpu_inference/** changes. The padding factorization, output-strip logic, _chunked_sdpa routing, and the GridTHW pytree round-trip are all cheaply unit-testable, and several of the inline findings would have been caught by them.
3. No accuracy validation. The PR validation is a throughput benchmark only. The padding/strip changes sit on the numerics path; please include an accuracy run (e.g. the CI accuracy harness or lm-eval on a VL benchmark) for at least one image and one video workload. Note the green CI here doesn't exercise the new seq-pad branch (its inputs are 4-aligned at small TP) nor the PREFILL_SLICES path.
Given the four changes are independent (pixel sharding+padding / SDPA flash redirect / precompile expansion / mm cache), splitting them into separate PRs would make each easy to test and land — the sharding+padding and SDPA pieces are closest to ready.
| self.step_with_batch_queue) | ||
|
|
||
| self.mm_receiver_cache = None | ||
| self.mm_receiver_cache = MULTIMODAL_REGISTRY.engine_receiver_cache_from_config( |
There was a problem hiding this comment.
[blocker] engine_receiver_cache_from_config takes the full VllmConfig, not a ModelConfig: at the pinned vLLM, MultiModalRegistry._get_cache_type immediately does model_config = vllm_config.model_config (vllm multimodal/registry.py), so passing vllm_config.model_config here raises AttributeError as soon as this class is constructed — for any model, multimodal or not. Same issue at line 744. PR CI doesn't catch it because these classes are only instantiated under PREFILL_SLICES disagg mode.
vLLM's own EngineCore.__init__ calls it with the full config: engine_receiver_cache_from_config(vllm_config).
| elif self.mm_receiver_cache is not None and getattr( | ||
| request, "mm_positions", None) and hasattr( | ||
| request, "mm_features"): | ||
| # Older vllm might use mm_features, checking both to be safe |
There was a problem hiding this comment.
[blocker] This block can never execute at the vLLM commit this repo pins: EngineCoreRequest has neither mm_inputs nor mm_positions — multimodal data lives in mm_features (vllm/v1/engine/__init__.py). So the first branch is always falsy and getattr(request, "mm_positions", None) is always None. (The comment is also inverted — mm_features is the newer schema, mm_inputs the older one.)
If engine-side feature caching is the goal, mirror vLLM's EngineCore.preprocess_add_request:
if self.mm_receiver_cache is not None and request.mm_features:
request.mm_features = self.mm_receiver_cache.get_and_update_features(request.mm_features)Also note the sibling DisaggEngineCore now creates the cache in __init__ but its add_request never applies it.
| self, | ||
| scheduler_output: SchedulerOutput, | ||
| ) -> Optional[ModelRunnerOutput]: | ||
| if getattr(self, "_apply_mm_cache", None) is not None: |
There was a problem hiding this comment.
[blocker] This whole block is dead code at the pinned vLLM:
_apply_mm_cacheis defined on vLLM'sWorkerWrapperBase;TPUWorkersubclasses the abstractWorkerBase, sogetattr(self, "_apply_mm_cache", None)is alwaysNonehere. When the executor path goes throughWorkerWrapperBase, itsexecute_modelalready applies the cache before delegating (vllm/v1/worker/worker_base.py).- In the
elif,NewRequestDatahas nomm_inputsand nomm_positions— onlymm_features(vllm/v1/core/sched/output.py) — so neither branch can fire.
Net effect: nothing runs. Either rewrite against req.mm_features (as WorkerWrapperBase._apply_mm_cache does) or drop this.
| import multiprocessing | ||
|
|
||
| from vllm.multimodal import MULTIMODAL_REGISTRY | ||
| self.mm_receiver_cache = MULTIMODAL_REGISTRY.worker_receiver_cache_from_config( |
There was a problem hiding this comment.
[minor] Two issues: (1) worker_receiver_cache_from_config only returns a cache for mm_processor_cache_type == "shm", and vLLM deliberately refuses to build it without the executor-provided shared_worker_lock (it raises in WorkerWrapperBase.init_worker) — a fresh private multiprocessing.Lock() can't provide cross-process mutual exclusion for the shm object store. (2) This is now the third place a worker receiver cache is created (vLLM's WorkerWrapperBase, disagg_executor.py:82). Plumb the shared lock through the executor or drop this. Also move the function-level multiprocessing / MULTIMODAL_REGISTRY imports to module top.
| # Find a pad_seq that can be factored into t, h, w where h>=2 and w>=2 | ||
| # to prevent spatial_merge_size downsampling from crashing on 0. | ||
| dummy_grid_thw = None | ||
| while dummy_grid_thw is None: |
There was a problem hiding this comment.
[major] This search only enforces h >= 2, w >= 2, but the vision tower also needs h and w divisible by spatial_merge_size: e.g. pad_seq=6 yields (1, 2, 3), and Qwen3-VL's rot_pos_ids does reshape(h//merge, merge, w//merge, merge) → ValueError (6 elements into (1,2,1,2)).
Today this is unreachable only because the qwen processors emit h/w as multiples of merge_size, so patch counts are ≡ 0 mod 4 and pad_seq always lands on a multiple of 4 (the h=2, w=2 case) — but nothing asserts that invariant, and anything that violates it crashes deep in the vision tower.
Simpler and safe: pad to a multiple of lcm(tp_size, merge_size**2) and use dummy_grid_thw = [pad_seq // merge_size**2, merge_size, merge_size]; the nested search loop then goes away entirely.
| _orig_sdpa = F.scaled_dot_product_attention | ||
|
|
||
|
|
||
| def _chunked_sdpa(query, |
There was a problem hiding this comment.
[nit] _chunked_sdpa doesn't chunk — it redirects to encoder_only_flash_attention. Rename to reflect behavior (e.g. _flash_attn_sdpa), and lift 40 * 1024 * 1024 into a named constant. Side note: the GQA bail-out comment says the kernel requires num_heads == num_kv_heads, but the kernel signature accepts distinct num_kv_heads — the fallback is fine, the comment overstates the constraint.
| # | ||
| # ⚠️ WARNING: Adding more frames or larger buckets here will significantly increase | ||
| # server startup time (XLA compilation) and can cause Host CPU OOMs during boot. | ||
| frame_counts = [1, 2, 4, 8, 16] |
There was a problem hiding this comment.
[major] Startup-cost / coverage concerns with this expansion:
- Fan-out per patch bucket goes 1 → 12 compilations for the listed model types, and 18 for everything else. Qwen3-Omni's
model_type(qwen3_omni_moe) is not in the tuple, so the currently-supported omni path now getsvideo_grid_thwand bare-grid_thwdummy calls it never received before, on top of the multiplier — and theVISION_MIN_SHIFTdefault change (4 → 6) silently drops the 16/32-patch buckets it used to warm. - The grid args are jit
static_argnames, so a precompiled(t, 2^a, 2^b)grid only matches media whose processed grid is exactly that. Real-world resolutions still compile at first request; the warm-up helps pow-2 shapes (like the 256x256x16 benchmark), but "fully warm up the JIT cache" overstates it.
Suggest scoping frame_counts to the video-capable archs that need it and keeping the previous defaults (min_shift=4, single image-grid call) for omni and image-only deployments.
| logger.warning( | ||
| f"Expect torch.Tensor, got {type(v)}") | ||
| return v | ||
| arr = t2j(v.contiguous(), use_dlpack=True) |
There was a problem hiding this comment.
[major] use_dlpack=False was chosen explicitly when this conversion was introduced (#2396); flipping it to True (+ .contiguous()) changes the tensor-conversion path for every multimodal model, and is unrelated to the padding/sharding this PR is about. Was False load-bearing (dtype/layout issues)? If the flip is intentional, add a comment with the rationale and validation; otherwise keep False here and make the dlpack switch its own change.
| self.vllm_config, "parallel_config"): | ||
| tp_size = self.vllm_config.parallel_config.tensor_parallel_size | ||
| else: | ||
| tp_size = 8 |
There was a problem hiding this comment.
[minor] self.vllm_config is always set in __init__, so the hasattr chain and this tp_size = 8 fallback are dead — and wrong if they ever fired (a TP=2/TP=4 deployment would pad/shard as if TP=8). Also, divisibility for PartitionSpec("model", None) should be checked against the actual mesh axis size (self.mesh.shape["model"]), not tensor_parallel_size — they aren't guaranteed equal under all sharding configs. The same block is duplicated at ~705; extract one helper.
|
|
||
| # 1. Homogeneous padding to maximize cache hits | ||
| grid_list = list(grid) if type( | ||
| grid).__name__ == "GridTHW" else grid.tolist( |
There was a problem hiding this comment.
[minor] GridTHW is imported at the top of this file — use isinstance(grid, GridTHW) instead of the type(grid).__name__ == "GridTHW" string check (4 occurrences), and this list(grid) if ... else grid.tolist() if ... else list(grid) ternary appears twice — worth a small _grid_to_list(grid) helper.
| Qwen3OmniMoeConfig | ||
| from vllm.config import VllmConfig | ||
| from vllm.model_executor.models.qwen3_5 import ( | ||
| Qwen3_5ForConditionalGeneration, Qwen3_5MoeForConditionalGeneration) |
There was a problem hiding this comment.
these were intentionally removed in https://github.com/vllm-project/tpu-inference/pull/2878/changes
There was a problem hiding this comment.
if you add a space around this line
There was a problem hiding this comment.
@lk-chen we can definitely leverage the benchmarking capability. One thing to note is the client args are different than what is needed. We need a video input with text and 1k output tokens.
Description
📊 Technical Analysis
This commit addresses several critical memory inefficiencies, OOMs, and runtime stalls when performing large-scale video inference with Qwen architectures on TPUs.
1. 🧮 Memory Inefficiency & OOMs in Multimodal Forward Pass
pixel_valuestensor was being fully replicated across all TPU devices. This led to severe HBM/Host Out-of-Memory (OOM) errors. Furthermore, input sequence lengths were often not divisible by the Tensor Parallelism (tp_size) factor, making efficient sharding impossible without crashing downstream spatial downsampling operations.tp_sizeand valid factoring dimensions.pixel_valuesandgrid_thwto be multiples oftp_size.(t, h, w)dimensions (wherePartitionSpec("model", None).🔍 Code (
tpu_inference/models/vllm/vllm_model_wrapper.py)2. 💥 VMEM OOM in Vision Tower Attention
scaled_dot_product_attention(SDPA) consumed excessive VMEM (encoder_only_flash_attention) based on dynamic memory heuristics.F.scaled_dot_product_attentionfor Qwen3 VL._chunked_sdpawhich estimates native VMEM usage (encoder_only_flash_attentionkernel fromtpu-inference.🔍 Code (
tpu_inference/models/vllm/experimental/qwen3_vl_patcher.py)3. ⏳ Stalls Due to Missing JIT Support & Precompilation
Qwen3_5MoeForConditionalGenerationandQwen3_5ForConditionalGenerationtoJITTABLE_ARCHS.maybe_precompile_vision_encoder_fnto iterate over common video frame counts[1, 2, 4, 8, 16]to fully warm up the JIT cache before requests arrive.🔍 Code (
tpu_inference/models/vllm/experimental/vision_tower_jit.py)4. 🗄️ Missing Multimodal Cache Integration
mm_receiver_cache.mm_receiver_cacheinTPUWorkerandDisaggEngineCoreProcbefore running model execution to reuse cached features.🔍 Code (
tpu_inference/worker/tpu_worker.py)Tests
Model server command
Benchmark command
Performance metrics
Checklist
Before submitting this PR, please make sure:
-[X] I have performed a self-review of my code.
-[X] I have necessary comments in my code, particularly in hard-to-understand areas.
-[X] I have made or will make corresponding changes to any relevant documentation.