Repository navigation
[Feature] Support per-video audio masks for Qwen Omni - #4656
SamitHuang merged 1 commit into
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
b920345 to
7952cab
Compare
hsliuustc0106
left a comment
There was a problem hiding this comment.
Looks reasonable for review.
SamitHuang
left a comment
There was a problem hiding this comment.
This PR introduces a critical bug for Qwen2.5-Omni and breaks multi-modal caching for per-video masks. I have identified three major issues that need to be addressed before merging.
| return hf_mm_kwargs | ||
|
|
||
|
|
||
| class Qwen2_5OmniThinkerMultiModalProcessor( |
There was a problem hiding this comment.
This PR breaks Qwen2.5-Omni. Qwen2_5OmniThinkerMultiModalProcessor does not override _get_prompt_updates, so it uses the upstream vLLM implementation. The upstream implementation reads use_audio_in_video = hf_processor_mm_kwargs.get("use_audio_in_video", False). Since hf_processor_mm_kwargs is the unmodified user input (e.g. [True, False]), it evaluates to True, causing it to use get_replacement_qwen2_use_audio_in_video for all videos. This will lead to an IndexError: list index out of range when it tries to access audio_output_lengths for a video that doesn't have audio.
There was a problem hiding this comment.
Fixed. Qwen2.5 now overrides _get_prompt_updates and applies use_audio_in_video per video instead of treating a list mask as global true.
For [True, False], only the first video uses the interleaved audio/video replacement; the second video uses the normal video replacement and does not consume audio_output_lengths.
Added regression coverage in test_qwen2_5_prompt_updates_apply_audio_mask_per_video.
| use_audio_in_video = mm_kwargs.get("use_audio_in_video") | ||
| if _is_per_video_use_audio_in_video(use_audio_in_video): | ||
| num_videos = _num_videos_in_hf_mm_data(mm_data) | ||
| self._vllm_omni_per_video_use_audio_in_video = _normalize_use_audio_in_video( |
There was a problem hiding this comment.
Storing request-specific state on self (self._vllm_omni_per_video_use_audio_in_video) is not thread-safe if the processor is shared across concurrent requests. More importantly, this will crash on a multi-modal cache hit: when the video is fully cached, _call_hf_processor is still called for text tokenization but with an empty mm_data. num_videos will be 0, and _normalize_use_audio_in_video([True, False], 0) will raise a ValueError. You should extract the mask from inputs.hf_processor_mm_kwargs in _get_prompt_updates instead of mutating self.
There was a problem hiding this comment.
Fixed. Removed request-specific processor state (self._vllm_omni_per_video_use_audio_in_video) and now carry the mask through request-local hf_processor_mm_kwargs.
This avoids cross-request state and also avoids crashing when _call_hf_processor sees empty mm_data on multimodal cache hits.
Added coverage for empty-cache-data handling in test_coerce_use_audio_in_video_for_hf_processor_does_not_validate_against_empty_mm_data.
| return mm_kwargs | ||
|
|
||
| num_videos = _num_videos_in_hf_mm_data(mm_data) | ||
| video_use_audio_in_video = _normalize_use_audio_in_video(use_audio_in_video, num_videos) |
There was a problem hiding this comment.
This will crash on partial cache hits. When some videos are cached, mm_data only contains the missing (uncached) videos, so num_videos will be less than the total number of videos in the request. However, use_audio_in_video from mm_kwargs is the original list from the user request (length = total videos). _normalize_use_audio_in_video will raise a ValueError because the lengths don't match. You should avoid validating the length against mm_data here.
There was a problem hiding this comment.
Fixed. _coerce_use_audio_in_video_for_hf_processor no longer validates the per-video mask against cache-filtered mm_data.
For partial cache misses, the request-level mask is filtered to the uncached video items before missing-item processing. second_per_grid_ts is filtered the same way.
Added coverage in test_coerce_use_audio_in_video_for_hf_processor_does_not_validate_against_partial_mm_data and test_filter_video_use_audio_in_video_for_uncached_items_aligns_partial_cache_mask.
|
Thanks for the detailed review. Summary:
Validation:
|
|
Please fix the conflicts |
4aa3de0 to
7ebf076
Compare
7ebf076 to
f3bec74
Compare
|
Hi @SamitHuang , thanks for the checks, |
| if second_per_grid_ts is not None: | ||
| video_second_per_grid_t = float(second_per_grid_ts[item_idx]) | ||
| second_per_grid_ts = hf_processor_mm_kwargs.get("second_per_grid_ts", None) | ||
| if second_per_grid_ts: |
There was a problem hiding this comment.
This drops the old preference for the HF-computed second_per_grid_ts from out_mm_data and only reads request kwargs now, so presampled videos fall back to 2.0 and lose the temporal alignment the deleted comment guarded. Is that intentional?
There was a problem hiding this comment.
Fixed. For cached video items represented as None, we no longer hardcode use_audio_in_video=False when another video in the batch has an explicit mask.
Instead, missing/cached items fall back to the prompt-update based inference path and detect whether the video replacement contains audio tokens. This preserves the cached video's original audio-in-video classification on partial cache hits.
Added regression coverage in test_get_video_use_audio_in_video_falls_back_for_partial_cache_hit_with_explicit_mask.
| audio_in_video_item_idx += 1 | ||
|
|
||
| second_per_grid_ts = hf_processor_mm_kwargs.get("second_per_grid_ts", None) | ||
| if second_per_grid_ts: |
There was a problem hiding this comment.
if second_per_grid_ts: treats an empty list as absent and a multi-elem ndarray/tensor raises 'ambiguous truth value'. Go back to if second_per_grid_ts is not None:. Same idiom in the qwen3 copy.
There was a problem hiding this comment.
Fixed. Restored the previous preference for HF-computed second_per_grid_ts from out_mm_data, with request kwargs used only as a fallback.
This keeps presampled-video temporal alignment intact instead of falling back to the Qwen3 default 2.0.
Added Qwen3 regression coverage for preferring out_mm_data["second_per_grid_ts"] over request kwargs.
| has_use_audio_in_video = any(item is not None and "use_audio_in_video" in item for item in video_kwargs) | ||
| for item_idx, item in enumerate(video_kwargs): | ||
| if has_use_audio_in_video: | ||
| if item is None or "use_audio_in_video" not in item: |
There was a problem hiding this comment.
On a partial cache hit a cached video arrives as None here, and since has_use_audio_in_video is True for the batch it's forced to False, so a cached video that used audio gets misclassified. For the None items, fall back to the prompt-updates inference in the else branch instead of hardcoding False.
There was a problem hiding this comment.
Fixed. Replaced truthiness checks with explicit None checks and centralized second_per_grid_ts extraction.
The helper now handles list/tuple, torch.Tensor, and numpy.ndarray values without ambiguous truth-value errors, while preserving scalar behavior. Applied to both Qwen2.5 and Qwen3 paths.
Added regression coverage for tensor second_per_grid_ts values from kwargs.
6ca7bcc to
efad480
Compare
Signed-off-by: hbhflw2000 <417911774@qq.com>
05b1b82 to
ef486ad
Compare
Purpose
Fix mixed-video Qwen Omni requests where some videos include audio and others do not.
Previously,
mm_processor_kwargs["use_audio_in_video"]effectively behaved as a global bool. Passing a per-video list such as[true, false]could be misinterpreted by the HF processor as truthy for every video, which made audio/video placeholders and multimodal feature metadata diverge.This PR:
use_audio_in_video.OmniModelRunnerOutput.with_kv_conn_output_only.Backward compatibility:
use_audio_in_video=Trueanduse_audio_in_video=Falsecalls still use the original shared/global path.use_audio_in_videokeep the previous default behavior.ValueErrorinstead of silently producing misaligned placeholders.Test Plan
mm_processor_kwargs={"use_audio_in_video": [True, False]}.describe the story in the video, with audio info.vLLM Version:
0.22.0+cu126
vLLM-Omni Commit:
b920345
Test Result
python -m ruff check \ vllm_omni/model_executor/models/qwen2_5_omni/qwen2_5_omni_thinker.py \ vllm_omni/model_executor/models/qwen3_omni/qwen3_omni_moe_thinker.py \ vllm_omni/outputs.py \ tests/model_executor/models/qwen2_5_omni/test_per_video_audio_mask.py # All checks passed!python -m pytest tests/model_executor/models/qwen2_5_omni/test_per_video_audio_mask.py -q # 19 passed, 17 warnings in 3.88sFull Qwen3-Omni e2e passed with vLLM-Omni
Omnientrypoint and TP=4:Pre-submit check:
CONTRIBUTING.md.precheck-prquick workflow manually.[Bugfix].BEFORE SUBMITTING: read CONTRIBUTING.md and run the precheck-pr skill with the code agent for a self-check against project conventions.
(anything written below this line will be removed by GitHub Actions)