feat(multimodal): support Qwen3 ASR and Omni - #1905
Conversation
📝 WalkthroughWalkthroughThis PR adds end-to-end audio multimodal support, Qwen3 audio preprocessing, media-plan-driven gateway processing, Qwen3-ASR transcription routing, TokenSpeed audio handling, Qwen3-Omni video processing, and Qwen2 vocabulary/merges tokenizer loading. ChangesMultimodal audio and encoder contracts
Protocol, tokenizer, and servicer integration
Gateway multimodal pipeline
ASR routing and output validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GrpcRouter
participant MediaPlan
participant AsyncMultiModalTracker
participant MediaConnector
participant AudioProcessorRegistry
participant TokenSpeedAssembly
Client->>GrpcRouter: Submit chat request with audio content
GrpcRouter->>MediaPlan: Build ordered media plan
MediaPlan-->>GrpcRouter: Media parts and modality counts
GrpcRouter->>AsyncMultiModalTracker: Enqueue audio
AsyncMultiModalTracker->>MediaConnector: Fetch and decode audio
MediaConnector-->>AsyncMultiModalTracker: AudioClip
GrpcRouter->>AudioProcessorRegistry: Create model-specific processor
AudioProcessorRegistry-->>GrpcRouter: Qwen3AudioProcessor
GrpcRouter->>TokenSpeedAssembly: Assemble audio encoder inputs
TokenSpeedAssembly-->>Client: Multimodal response
sequenceDiagram
participant Client
participant GrpcRouter
participant Qwen3AsrRequestBuilder
participant ChatPipeline
participant OutputParser
Client->>GrpcRouter: Submit audio transcription request
GrpcRouter->>Qwen3AsrRequestBuilder: Build ASR chat request
Qwen3AsrRequestBuilder-->>GrpcRouter: ChatCompletionRequest
GrpcRouter->>ChatPipeline: Execute chat request
ChatPipeline-->>GrpcRouter: Generated response
GrpcRouter->>OutputParser: Parse transcription text
OutputParser-->>Client: JSON or text transcription
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
tokenizer
Signed-off-by: chenht2022 <chenht2022@gmail.com>
af1adc9 to
5602d7a
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive audio preprocessing and decoding support to the multimodal pipeline, specifically targeting Qwen3-ASR and Qwen3-Omni models. It adds audio decoding via Symphonia with an FFmpeg fallback, implements a Whisper-compatible log-mel frontend using RustFFT, and refactors the gateway and gRPC servicers to handle multiple modalities concurrently. The review feedback focuses on performance optimizations: specifically, reusing the interleaved vector allocation outside the decoding loop in decode.rs, and pre-planning the FFT once before the batch loop in qwen3_audio.rs to avoid redundant, expensive planning operations.
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.
| let mut mono = Vec::new(); | ||
| loop { |
There was a problem hiding this comment.
To avoid allocating a new vector on every iteration of the decoding loop, declare interleaved outside the loop.
| let mut mono = Vec::new(); | |
| loop { | |
| let mut mono = Vec::new(); | |
| let mut interleaved = Vec::new(); | |
| loop { |
References
- Avoid heap allocations (such as allocating a new
Vec) in hot or periodic paths.
| "failed to reserve {additional_samples} decoded audio samples: {error}" | ||
| )) | ||
| })?; | ||
| let mut interleaved = vec![0.0_f32; interleaved_samples]; |
There was a problem hiding this comment.
Reuse the interleaved vector allocated outside the loop by resizing it instead of re-allocating on every packet.
| let mut interleaved = vec![0.0_f32; interleaved_samples]; | |
| interleaved.resize(interleaved_samples, 0.0); |
References
- Avoid heap allocations (such as allocating a new
Vec) in hot or periodic paths.
| } | ||
|
|
||
| let batch_size = waveforms.len(); | ||
| let feature_values = batch_size |
There was a problem hiding this comment.
Planning the FFT is an expensive operation. To avoid re-planning it for every single audio clip in the batch, initialize the FftPlanner and plan the forward FFT once before the loop, then pass it to whisper_log_mel.
let batch_size = waveforms.len();
let mut planner = FftPlanner::<f32>::new();
let fft = planner.plan_fft_forward(self.params.n_fft);| .min(max_frames); | ||
| let mut padded = waveform; | ||
| padded.resize(max_samples, self.params.padding_value); | ||
| let features = whisper_log_mel(&padded, max_frames, &self.params)?; |
There was a problem hiding this comment.
Pass the pre-planned FFT to whisper_log_mel.
| let features = whisper_log_mel(&padded, max_frames, &self.params)?; | |
| let features = whisper_log_mel(&padded, max_frames, &self.params, &fft)?; |
| fn whisper_log_mel( | ||
| samples: &[f32], | ||
| frame_count: usize, | ||
| params: &Qwen3AudioParams, | ||
| ) -> Result<Array2<f32>, TransformError> { |
There was a problem hiding this comment.
Update the signature of whisper_log_mel to accept the pre-planned FFT.
| fn whisper_log_mel( | |
| samples: &[f32], | |
| frame_count: usize, | |
| params: &Qwen3AudioParams, | |
| ) -> Result<Array2<f32>, TransformError> { | |
| fn whisper_log_mel( | |
| samples: &[f32], | |
| frame_count: usize, | |
| params: &Qwen3AudioParams, | |
| fft: &Arc<dyn rustfft::Fft<f32>>, | |
| ) -> Result<Array2<f32>, TransformError> { |
| let mut planner = FftPlanner::<f32>::new(); | ||
| let fft = planner.plan_fft_forward(params.n_fft); |
There was a problem hiding this comment.
Remove the local FFT planning since it is now passed as an argument.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| error!( | ||
| function = "MessagePreparationStage::execute", | ||
| "Multimodal content detected but multimodal components not initialized" | ||
| model = %model_id, |
There was a problem hiding this comment.
🟡 Nit: This returns error::internal_error (HTTP 500), but the identical call in chat/preparation.rs returns error::bad_request (HTTP 400). Since prepare_placeholder_tokens can fail due to user-facing reasons (unsupported modality for the model, modality limit exceeded, placeholder token missing from vocab), the Messages API path should also surface this as a 400 so callers get an actionable error instead of a confusing 500.
| model = %model_id, | |
| error::bad_request( | |
| "invalid_multimodal_request", | |
| format!("Invalid multimodal request: {e}"), | |
| ) |
Signed-off-by: chenht2022 <chenht2022@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70d6b34dc8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let feature_length = original_samples | ||
| .div_ceil(self.params.hop_length) | ||
| .min(max_frames); |
There was a problem hiding this comment.
When a request batches multiple audio clips and a shorter clip's sample length is not an exact multiple of hop_length, this rounds its valid feature frames up while max_frames (and the Whisper STFT path described above) use floor division. That marks one padded frame as real and can inflate audio_feature_lengths/prompt token counts at chunk boundaries (for example around n_window * 2 frames), so the expanded audio placeholders no longer match the actual valid encoder output. Use the same floor calculation as max_frames for each item.
Useful? React with 👍 / 👎.
| error::internal_error( | ||
| "multimodal_placeholder_resolution_failed", | ||
| format!("Failed to resolve multimodal placeholder token: {e}"), | ||
| ) |
There was a problem hiding this comment.
For Messages API requests that contain media but target a non-multimodal model, an unsupported modality, or too many media items, prepare_placeholder_tokens returns an input-validation error; mapping it to internal_error turns those client mistakes into 500s instead of 4xx responses. The chat path maps the same preparation failure to bad_request, so this should do the same to avoid false server errors/retries for invalid multimodal Messages requests.
Useful? React with 👍 / 👎.
Signed-off-by: chenht2022 <chenht2022@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_gateway/src/routers/grpc/utils/chat_utils.rs (1)
220-316: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread a placeholder into the
Nonepath The Go bindings still callprocess_chat_messages(..., None), so any multimodalChatCompletionRequestsent through those entrypoints now fails instead of rendering a prompt. Pass the configured placeholder tokens there too, or keep the wrapper tolerant when no placeholders are supplied.🤖 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 `@model_gateway/src/routers/grpc/utils/chat_utils.rs` around lines 220 - 316, Ensure the Go-binding entrypoints calling process_chat_messages no longer pass None for multimodal requests: thread the configured PlaceholderTokens through to transform_content_field. If those callers cannot provide tokens, update the None path to preserve rendering compatibility instead of returning a missing-placeholder error, while retaining placeholder substitution when tokens are configured.crates/multimodal/src/vision/processors/qwen_vl_base.rs (1)
544-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment on
smart_resize_videono longer reflects the dual-mode (TotalVolume/PerFrame) budget logic.The doc comment states the pixel budget is unconditionally "applied to the full sampled video volume (
T * H * W)", but the body now branches onself.config.video_resize_mode, applying the budget per-frame forQwenVideoResizeMode::PerFrame. Worth updating so future readers don't assume single-mode behavior.📝 Proposed doc update
/// Smart resize for Qwen3-style video processors. /// - /// Unlike image resize, the pixel budget is applied to the full sampled - /// video volume (`T * H * W`), matching HuggingFace's Qwen3 video - /// processor. + /// Unlike image resize, the pixel budget is applied at video granularity. + /// With `QwenVideoResizeMode::TotalVolume` the budget covers the full + /// sampled volume (`T * H * W`), matching HuggingFace's Qwen3 video + /// processor. With `QwenVideoResizeMode::PerFrame` the budget applies to + /// a single frame's `H * W`, independent of frame count. pub fn smart_resize_video(🤖 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 `@crates/multimodal/src/vision/processors/qwen_vl_base.rs` around lines 544 - 611, Update the doc comment for smart_resize_video to describe both video_resize_mode behaviors: TotalVolume applies the pixel budget to the padded full video volume, while PerFrame applies it independently to a single frame. Remove the unconditional claim that the budget always uses T * H * W.
🤖 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 `@crates/multimodal/Cargo.toml`:
- Around line 32-33: Update the symphonia dependency declaration in Cargo.toml
to remove features = ["all"] and enable only the explicit feature(s) required
for the existing WAV handling path. Preserve FFmpeg as the fallback and do not
add unrelated codec or container features.
In `@crates/multimodal/src/audio/transforms.rs`:
- Around line 66-152: The audio test suite lacks coverage for the
bandlimited_resample path. Add a golden-vector test using a non-16 kHz input
clip that exercises bandlimited_resample, and assert the complete expected
output and length to detect kernel/indexing or output-length regressions.
In `@crates/multimodal/src/media.rs`:
- Around line 348-374: Update fetch_http_audio and the matching HTTP media
fetcher to enforce the configured maximum payload size before buffering: reject
responses whose Content-Length exceeds the limit and stream body chunks while
tracking cumulative bytes, returning the existing appropriate error once the
limit is crossed. Replace unbounded resp.bytes() buffering with the bounded
collection path, while preserving normal decoding for responses within the
limit.
In `@crates/multimodal/src/vision/processors/qwen3_omni_vision.rs`:
- Around line 81-95: Remove the duplicate
from_shared_preprocessor_config_for_video implementation and delegate video
shared-config handling to from_preprocessor_config. Update
with_video_preprocessor_config to use that delegation directly, preserving the
existing image limit fallbacks, video defaults, and other with_limits arguments.
- Around line 143-150: Extract the duplicated six-field structural override
check from has_structural_overrides and
Qwen2VLProcessor::with_preprocessor_config into one shared helper on the common
configuration or processor-base type. Update both call sites to use that helper,
preserving the current fields and behavior so future additions require changing
only one implementation.
In `@crates/multimodal/src/vision/processors/qwen3_vl.rs`:
- Around line 91-93: Update Qwen3VLProcessor initialization in new, with_config,
and from_preprocessor_config so video_min_pixels and video_max_pixels use
dedicated video defaults or configuration overrides rather than
DEFAULT_MIN_PIXELS and DEFAULT_MAX_PIXELS. Keep image bounds assigned only to
the image fields, and preserve QwenVLProcessorBase::smart_resize_video reading
the independent video limits.
---
Outside diff comments:
In `@crates/multimodal/src/vision/processors/qwen_vl_base.rs`:
- Around line 544-611: Update the doc comment for smart_resize_video to describe
both video_resize_mode behaviors: TotalVolume applies the pixel budget to the
padded full video volume, while PerFrame applies it independently to a single
frame. Remove the unconditional claim that the budget always uses T * H * W.
In `@model_gateway/src/routers/grpc/utils/chat_utils.rs`:
- Around line 220-316: Ensure the Go-binding entrypoints calling
process_chat_messages no longer pass None for multimodal requests: thread the
configured PlaceholderTokens through to transform_content_field. If those
callers cannot provide tokens, update the None path to preserve rendering
compatibility instead of returning a missing-placeholder error, while retaining
placeholder substitution when tokens are configured.
🪄 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: 1a31903a-22c4-4088-aaa1-4193f3843f9b
📒 Files selected for processing (66)
crates/grpc_client/proto/tokenspeed_encoder.protocrates/grpc_client/proto/tokenspeed_scheduler.protocrates/multimodal/Cargo.tomlcrates/multimodal/src/audio/decode.rscrates/multimodal/src/audio/mod.rscrates/multimodal/src/audio/processor.rscrates/multimodal/src/audio/processors/mod.rscrates/multimodal/src/audio/processors/qwen3_audio.rscrates/multimodal/src/audio/transforms.rscrates/multimodal/src/encoder_inputs.rscrates/multimodal/src/error.rscrates/multimodal/src/hasher.rscrates/multimodal/src/lib.rscrates/multimodal/src/media.rscrates/multimodal/src/registry/kimi_k25.rscrates/multimodal/src/registry/llama4.rscrates/multimodal/src/registry/llava.rscrates/multimodal/src/registry/mod.rscrates/multimodal/src/registry/phi3_v.rscrates/multimodal/src/registry/qwen3_asr.rscrates/multimodal/src/registry/qwen3_omni.rscrates/multimodal/src/registry/qwen3_vl.rscrates/multimodal/src/registry/qwen_vl.rscrates/multimodal/src/registry/traits.rscrates/multimodal/src/tracker.rscrates/multimodal/src/types.rscrates/multimodal/src/vision/mod.rscrates/multimodal/src/vision/processor.rscrates/multimodal/src/vision/processors/kimi_k25.rscrates/multimodal/src/vision/processors/llava.rscrates/multimodal/src/vision/processors/mod.rscrates/multimodal/src/vision/processors/qwen2_vl.rscrates/multimodal/src/vision/processors/qwen3_omni_vision.rscrates/multimodal/src/vision/processors/qwen3_vl.rscrates/multimodal/src/vision/processors/qwen_vl_base.rscrates/multimodal/src/vision/transforms.rscrates/multimodal/tests/multimodal_tracker_test.rscrates/protocols/src/chat.rscrates/protocols/src/common.rscrates/tokenizer/src/factory.rscrates/tokenizer/src/huggingface.rscrates/tokenizer/tests/qwen2_vocab_merges.rscrates/tokenizer/tests/qwen3_asr_bpe_parity.rsgrpc_servicer/smg_grpc_servicer/tokenspeed/encoder_servicer.pygrpc_servicer/smg_grpc_servicer/tokenspeed/servicer.pymodel_gateway/src/routers/grpc/common/stages/worker_selection.rsmodel_gateway/src/routers/grpc/epd_encode.rsmodel_gateway/src/routers/grpc/harmony/builder.rsmodel_gateway/src/routers/grpc/mod.rsmodel_gateway/src/routers/grpc/multimodal/assemble.rsmodel_gateway/src/routers/grpc/multimodal/config.rsmodel_gateway/src/routers/grpc/multimodal/detect.rsmodel_gateway/src/routers/grpc/multimodal/mod.rsmodel_gateway/src/routers/grpc/multimodal/plan.rsmodel_gateway/src/routers/grpc/multimodal/process.rsmodel_gateway/src/routers/grpc/multimodal/transport.rsmodel_gateway/src/routers/grpc/pd_router.rsmodel_gateway/src/routers/grpc/proto_wrapper.rsmodel_gateway/src/routers/grpc/regular/stages/chat/preparation.rsmodel_gateway/src/routers/grpc/regular/stages/chat/request_building.rsmodel_gateway/src/routers/grpc/regular/stages/messages/preparation.rsmodel_gateway/src/routers/grpc/regular/stages/messages/request_building.rsmodel_gateway/src/routers/grpc/router.rsmodel_gateway/src/routers/grpc/utils/chat_utils.rsmodel_gateway/src/routers/grpc/utils/message_utils.rsmodel_gateway/src/routers/grpc/utils/mod.rs
💤 Files with no reviewable changes (2)
- model_gateway/src/routers/grpc/regular/stages/messages/request_building.rs
- model_gateway/src/routers/grpc/regular/stages/chat/request_building.rs
| rustfft = "6.4" | ||
| symphonia = { version = "0.6", default-features = false, features = ["all"] } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine which Symphonia codecs/formats are actually exercised before FFmpeg fallback.
rg -n 'symphonia' crates/multimodal/src/audio/decode.rs -A5
rg -n 'Codec|CodecType|register_all|get_codecs|get_formats|probe' crates/multimodal/src/audio/decode.rsRepository: lightseekorg/smg
Length of output: 4955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Cargo.toml excerpt\n'
sed -n '1,80p' crates/multimodal/Cargo.toml
printf '\n## Symphonia decode implementation\n'
sed -n '84,170p' crates/multimodal/src/audio/decode.rs
printf '\n## Audio tests\n'
sed -n '470,540p' crates/multimodal/src/audio/decode.rsRepository: lightseekorg/smg
Length of output: 7805
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
p = Path("crates/multimodal/src/audio/decode.rs")
text = p.read_text()
# print only lines with Symphonia hint/codecs references and tests names for quick scan
for i, line in enumerate(text.splitlines(), 1):
if "get_probe()" in line or "get_codecs()" in line or "Hint" in line or "TrackType" in line or "wav_i16_mono" in line or "symphonia_decodes_wav_to_mono_f32" in line:
print(f"{i}: {line}")
PYRepository: lightseekorg/smg
Length of output: 704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("crates/multimodal/src/audio/decode.rs")
for start, end in [(84,170), (470,540)]:
print(f"\n### lines {start}-{end}")
for i, line in enumerate(p.read_text().splitlines(), 1):
if start <= i <= end:
print(f"{i:4}: {line}")
PYRepository: lightseekorg/smg
Length of output: 6750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Audio dispatch and suffix helpers\n'
sed -n '1,220p' crates/multimodal/src/audio/decode.rs
printf '\n## Search for ffmpeg/symphonia backend selection\n'
rg -n 'audio_temp_suffix|SMG_AUDIO_DECODE_BACKEND|ffmpeg fallback|decode_audio_with_ffmpeg|decode_audio_with_symphonia' crates/multimodal/src/audio/decode.rsRepository: lightseekorg/smg
Length of output: 9407
Narrow Symphonia's feature set features = ["all"] pulls in every Symphonia codec/container reader, but this path only has WAV coverage and FFmpeg already handles the fallback path. If Symphonia only needs a smaller in-process subset, list those features explicitly instead of enabling everything.
🤖 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 `@crates/multimodal/Cargo.toml` around lines 32 - 33, Update the symphonia
dependency declaration in Cargo.toml to remove features = ["all"] and enable
only the explicit feature(s) required for the existing WAV handling path.
Preserve FFmpeg as the fallback and do not add unrelated codec or container
features.
| /// Match torchaudio's default `functional.resample`: band-limited sinc | ||
| /// interpolation with a Hann window, filter width 6, and rolloff 0.99. | ||
| pub(super) fn bandlimited_resample( | ||
| samples: &[f32], | ||
| src_sample_rate: usize, | ||
| dst_sample_rate: usize, | ||
| ) -> Result<Vec<f32>, TransformError> { | ||
| const LOWPASS_FILTER_WIDTH: f32 = 6.0; | ||
| const ROLLOFF: f32 = 0.99; | ||
|
|
||
| if src_sample_rate == 0 || dst_sample_rate == 0 { | ||
| return Err(TransformError::ShapeError( | ||
| "audio resampling rates must be positive".to_string(), | ||
| )); | ||
| } | ||
| if samples.is_empty() || src_sample_rate == dst_sample_rate { | ||
| return Ok(samples.to_vec()); | ||
| } | ||
|
|
||
| let gcd = greatest_common_divisor(src_sample_rate, dst_sample_rate); | ||
| let orig_freq = src_sample_rate / gcd; | ||
| let new_freq = dst_sample_rate / gcd; | ||
| let base_freq = orig_freq.min(new_freq) as f32 * ROLLOFF; | ||
| let width = (LOWPASS_FILTER_WIDTH * orig_freq as f32 / base_freq).ceil() as usize; | ||
| let kernel_len = width | ||
| .checked_mul(2) | ||
| .and_then(|value| value.checked_add(orig_freq)) | ||
| .ok_or_else(|| TransformError::ShapeError("audio resample kernel is too large".into()))?; | ||
| let kernel_values = new_freq | ||
| .checked_mul(kernel_len) | ||
| .ok_or_else(|| TransformError::ShapeError("audio resample kernel size overflow".into()))?; | ||
| let mut kernels = Vec::new(); | ||
| kernels.try_reserve_exact(kernel_values).map_err(|error| { | ||
| TransformError::ShapeError(format!("failed to allocate audio resample kernel: {error}")) | ||
| })?; | ||
|
|
||
| let orig_freq_f32 = orig_freq as f32; | ||
| let new_freq_f32 = new_freq as f32; | ||
| let scale = base_freq / orig_freq_f32; | ||
| for phase in 0..new_freq { | ||
| for kernel_index in 0..kernel_len { | ||
| let idx = (kernel_index as f32 - width as f32) / orig_freq_f32; | ||
| let mut t = (idx - phase as f32 / new_freq_f32) * base_freq; | ||
| t = t.clamp(-LOWPASS_FILTER_WIDTH, LOWPASS_FILTER_WIDTH); | ||
| let window = (t * PI_F32 / LOWPASS_FILTER_WIDTH / 2.0).cos().powi(2); | ||
| let radians = t * PI_F32; | ||
| let sinc = if radians == 0.0 { | ||
| 1.0 | ||
| } else { | ||
| radians.sin() / radians | ||
| }; | ||
| kernels.push(sinc * window * scale); | ||
| } | ||
| } | ||
|
|
||
| let target_len_u128 = (samples.len() as u128 * new_freq as u128).div_ceil(orig_freq as u128); | ||
| let target_len = usize::try_from(target_len_u128).map_err(|_| { | ||
| TransformError::ShapeError("resampled audio length exceeds usize".to_string()) | ||
| })?; | ||
| let mut output = Vec::new(); | ||
| output.try_reserve_exact(target_len).map_err(|error| { | ||
| TransformError::ShapeError(format!("failed to allocate resampled audio: {error}")) | ||
| })?; | ||
|
|
||
| for block in 0..samples.len().div_ceil(orig_freq) { | ||
| let input_start = block * orig_freq; | ||
| for phase in 0..new_freq { | ||
| if output.len() == target_len { | ||
| return Ok(output); | ||
| } | ||
| let kernel = &kernels[phase * kernel_len..(phase + 1) * kernel_len]; | ||
| let mut value = 0.0_f32; | ||
| for (kernel_index, &coefficient) in kernel.iter().enumerate() { | ||
| let padded_index = input_start + kernel_index; | ||
| if padded_index >= width { | ||
| let sample_index = padded_index - width; | ||
| if let Some(&sample) = samples.get(sample_index) { | ||
| value = sample.mul_add(coefficient, value); | ||
| } | ||
| } | ||
| } | ||
| output.push(value); | ||
| } | ||
| } | ||
|
|
||
| Ok(output) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
torchaudio functional.resample sinc interpolation output length formula
💡 Result:
In torchaudio.functional.resample, the output length is calculated based on the ratio of the new frequency to the original frequency. Specifically, if the input waveform has a length of T, the output length is defined by the formula: Output Length = ceil(T * new_freq / orig_freq) This follows the standard definition of resampling in digital signal processing, where the number of samples scales linearly with the sampling rate ratio. The implementation uses bandlimited sinc interpolation, which treats the signal as a continuous function and samples it at the new rate, resulting in this length conversion [1][2][3].
Citations:
- 1: https://github.com/pytorch/audio/blob/main/src/torchaudio/functional/functional.py
- 2: https://docs.pytorch.org/audio/main/generated/torchaudio.functional.resample.html
- 3: https://docs.pytorch.org/audio/master/generated/torchaudio.functional.resample.html
🏁 Script executed:
git ls-files crates/multimodal/src/audio/transforms.rs crates/multimodal | sed -n '1,120p'
rg -n "bandlimited_resample|resample|16k|16000|qwen3_audio" crates/multimodal -g '*.{rs,toml}'
python3 - <<'PY'
from pathlib import Path
p = Path('crates/multimodal/src/audio/transforms.rs')
print(p.exists(), p.stat().st_size if p.exists() else None)
PYRepository: lightseekorg/smg
Length of output: 8797
🏁 Script executed:
sed -n '1,220p' crates/multimodal/src/audio/processors/qwen3_audio.rs
printf '\n---\n'
sed -n '220,640p' crates/multimodal/src/audio/processors/qwen3_audio.rs
printf '\n---\n'
sed -n '1,220p' crates/multimodal/src/audio/transforms.rsRepository: lightseekorg/smg
Length of output: 27557
Add a test for the resampling path. The current audio tests only cover 16 kHz input, so bandlimited_resample never runs. A golden-vector case for a non-16 kHz clip would catch kernel/indexing or output-length drift.
🤖 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 `@crates/multimodal/src/audio/transforms.rs` around lines 66 - 152, The audio
test suite lacks coverage for the bandlimited_resample path. Add a golden-vector
test using a non-16 kHz input clip that exercises bandlimited_resample, and
assert the complete expected output and length to detect kernel/indexing or
output-length regressions.
| fn has_structural_overrides(config: &PreProcessorConfig) -> bool { | ||
| config.patch_size.is_some() | ||
| || config.merge_size.is_some() | ||
| || config.min_pixels.is_some() | ||
| || config.max_pixels.is_some() | ||
| || config.temporal_patch_size.is_some() | ||
| || config.size.is_some() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
has_structural_overrides duplicates the same override check in qwen2_vl.rs.
Qwen2VLProcessor::with_preprocessor_config (in qwen2_vl.rs) inlines an identical 6-field override check. Consider hoisting this into a shared helper (e.g. on PreProcessorConfig or QwenVLProcessorBase) to avoid drift if a new structural field is added later.
🤖 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 `@crates/multimodal/src/vision/processors/qwen3_omni_vision.rs` around lines
143 - 150, Extract the duplicated six-field structural override check from
has_structural_overrides and Qwen2VLProcessor::with_preprocessor_config into one
shared helper on the common configuration or processor-base type. Update both
call sites to use that helper, preserving the current fields and behavior so
future additions require changing only one implementation.
| video_min_pixels: DEFAULT_MIN_PIXELS, | ||
| video_max_pixels: DEFAULT_MAX_PIXELS, | ||
| video_resize_mode: QwenVideoResizeMode::TotalVolume, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and map the processor implementations.
git ls-files | rg 'crates/.*/qwen3_vl\.rs$|qwen_vl_base\.rs$|qwen3_omni.*\.rs$|process\.rs$'
echo '--- qwen3_vl.rs outline ---'
ast-grep outline crates/multimodal/src/vision/processors/qwen3_vl.rs --view expanded || true
echo '--- qwen_vl_base.rs outline ---'
ast-grep outline crates/multimodal/src/vision/processors/qwen_vl_base.rs --view expanded || true
echo '--- qwen3_omni files outline ---'
for f in $(git ls-files | rg 'qwen3_omni.*\.rs$'); do
echo "FILE: $f"
ast-grep outline "$f" --view expanded || true
doneRepository: lightseekorg/smg
Length of output: 10958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact constructors and video preprocessing logic with line numbers.
sed -n '1,240p' crates/multimodal/src/vision/processors/qwen3_vl.rs
echo '---'
sed -n '1,260p' crates/multimodal/src/vision/processors/qwen_vl_base.rsRepository: lightseekorg/smg
Length of output: 15239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the video-related constructors and any dedicated video config paths.
sed -n '1,240p' crates/multimodal/src/vision/processors/qwen3_vl.rs
echo '---'
sed -n '1,260p' crates/multimodal/src/vision/processors/qwen_vl_base.rs
echo '---'
for f in $(git ls-files | rg 'qwen3_omni.*\.rs$'); do
echo "FILE: $f"
sed -n '1,260p' "$f"
echo '---'
doneRepository: lightseekorg/smg
Length of output: 33308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the exact lines and the downstream usage of the fields.
nl -ba crates/multimodal/src/vision/processors/qwen3_vl.rs | sed -n '1,260p'
echo '---'
nl -ba crates/multimodal/src/vision/processors/qwen_vl_base.rs | sed -n '1,320p'
echo '---'
rg -n "video_min_pixels|video_max_pixels|video_resize_mode|from_video_preprocessor_config|with_video_preprocessor_config|smart_resize_video|preprocess_video" crates/multimodal/src/vision/processorsRepository: lightseekorg/smg
Length of output: 194
🌐 Web query:
Qwen3-VL video_min_pixels video_max_pixels smart_resize_video Qwen3 VL config
💡 Result:
In Qwen3-VL, video processing parameters are used to manage the input resolution and token budget for video data during training and inference [1][2][3]. Key Parameters: - video_max_pixels: Defines the maximum total pixel count allowed for an entire video [3]. For a video with T frames, height H, and width W, the product T × H × W must not exceed this value [3]. Common default configurations or examples include 1664 * 28 * 28 or 1024 * 28 * 28 [1][2]. - video_min_pixels: Sets the minimum allowable pixel count for the video, similarly calculated as the total budget across all frames [3]. Examples include 256 * 28 * 28 [1][2]. - smart_resize_video: This refers to a processing logic (often implemented in supporting utilities or custom pipelines like those found in multimodal wrappers) that handles the resizing and patchification of video frames to fit the model's patch-based vision encoder requirements [4][5]. It ensures that input video frames are resized and structured correctly—maintaining spatial-temporal integrity—before being processed by the vision transformer [4][5]. Configuration Context: These parameters are typically managed within DataArguments in training scripts or through the processor's configuration dictionary [1][2][3]. Because Qwen3-VL processes video as a sequence of patches, setting these limits is critical for managing GPU memory consumption and preventing Out-Of-Memory (OOM) errors [6][3]. If the total number of tokens (derived from pixels and patches) exceeds model capacity, performance or stability may be compromised [3]. For detailed implementation logic, developers are often directed to the qwen_vl_utils/vision_process.py utility or equivalent processor definitions [3].
Citations:
- 1: https://www.mintlify.com/QwenLM/Qwen3-VL/fine-tuning/training-script
- 2: https://github.com/QwenLM/Qwen3-VL/blob/main/qwen-vl-finetune/qwenvl/train/argument.py
- 3: https://github.com/QwenLM/Qwen3-VL?tab=readme-ov-file
- 4: refactor(multimodal): Multimodal ABI generalization #1602
- 5: https://github.com/lightseekorg/smg/blob/b1120b13/crates/multimodal/src/vision/processors/qwen_vl_base.rs
- 6: Fintuning qestion QwenLM/Qwen3-VL#1924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the video planning and preprocessing code paths that consume the video config.
sed -n '360,620p' crates/multimodal/src/vision/processors/qwen_vl_base.rs
echo '---'
sed -n '1200,1305p' crates/multimodal/src/vision/processors/qwen_vl_base.rs
echo '---'
sed -n '630,690p' crates/multimodal/src/vision/processors/qwen3_vl.rs
echo '---'
sed -n '230,290p' crates/multimodal/src/vision/processors/qwen3_vl.rsRepository: lightseekorg/smg
Length of output: 18013
Keep Qwen3-VL video limits independent from image limits
QwenVLProcessorBase::smart_resize_video reads video_min_pixels/video_max_pixels separately, but Qwen3VLProcessor always copies the image bounds into those fields in new, with_config, and from_preprocessor_config (lines 91-93, 116-118, 141-149). That removes video-specific budgeting for this processor; add a video-specific defaults/override path instead.
🤖 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 `@crates/multimodal/src/vision/processors/qwen3_vl.rs` around lines 91 - 93,
Update Qwen3VLProcessor initialization in new, with_config, and
from_preprocessor_config so video_min_pixels and video_max_pixels use dedicated
video defaults or configuration overrides rather than DEFAULT_MIN_PIXELS and
DEFAULT_MAX_PIXELS. Keep image bounds assigned only to the image fields, and
preserve QwenVLProcessorBase::smart_resize_video reading the independent video
limits.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a5af02f3f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| messages.push(ChatMessage::System { | ||
| content: MessageContent::Text(prompt.to_string()), |
There was a problem hiding this comment.
When /v1/audio/transcriptions callers pass prompt, this copies that user-controlled text directly into a Qwen3-ASR system turn. The Qwen3-ASR prompt format uses ChatML controls and the <asr_text> delimiter, and upstream vLLM strips those tokens for this builder to prevent reconstruction attacks (see https://docs.vllm.ai/en/stable/api/vllm/model_executor/models/qwen3_asr/#_sanitize_transcription_user_text), so a prompt containing <|im_end|> or <asr_text> can change the rendered prompt/assistant prefix and corrupt or steer the parsed transcription. Sanitize the prompt before inserting it.
Useful? React with 👍 / 👎.
vLLM 0.25 eagerly imports torchcodec, which dlopens FFmpeg shared libraries at import; the e2e runner image ships none, so every vLLM worker died importing vllm. Install distro FFmpeg in the vLLM setup and add an import canary so a missing-lib regression fails at install rather than mid-test. Signed-off-by: lightseek-bot <243258330+lightseek-bot@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 58-63: Update the dependency installation flow in
scripts/ci_install_vllm.sh to explicitly install a Torch-compatible torchcodec
before the existing `python3 -c` import canary, or raise the vLLM lower bound to
a release whose metadata requires torchcodec. Ensure the canary continues
validating imports of torch, torchcodec, and vllm on a clean runner.
🪄 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: faf774db-c744-47c3-9163-19cfa746c6ae
📒 Files selected for processing (1)
scripts/ci_install_vllm.sh
| # Import canary: fail here (not mid-e2e) if vLLM's eager torchcodec import | ||
| # can't find the FFmpeg shared libs installed above (torch first so its | ||
| # bundled CUDA libraries are loaded) | ||
| python3 -c "import torch, torchcodec, vllm" | ||
| echo "vllm/torchcodec import canary OK" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from importlib.metadata import requires
deps = requires("vllm") or []
assert any(dep.lower().startswith("torchcodec") for dep in deps), (
"Resolved vLLM does not declare torchcodec; install it explicitly."
)
import torchcodec
print("torchcodec dependency/import verified")
PYRepository: lightseekorg/smg
Length of output: 1085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section of the script
cat -n scripts/ci_install_vllm.sh | sed -n '1,140p'Repository: lightseekorg/smg
Length of output: 5555
🌐 Web query:
vllm 0.22.1 PyPI JSON requires_dist torchcodec
💡 Result:
vLLM version 0.22.1 does not strictly require torchcodec as a core dependency for installation, although it is utilized as an optional backend for video decoding features [1]. While vLLM has integrated torchcodec to replace older backends like decord for video processing [2][1], it is generally managed as an optional or platform-specific component rather than a hard requirement for all installations [3][4]. For instance, scripts exist to facilitate its installation specifically for ROCm compatibility [3], and it has been removed from certain audio-related dependency lists in related forks [4]. PyPI metadata for vllm 0.22.1 lists numerous dependencies [5], but torchcodec is not mandated for basic LLM inference functionality [6]. Users requiring video decoding features should ensure their environment meets the requirements for torchcodec, which may include manual installation depending on the platform [3][2].
Citations:
- 1: Add TorchCodec as a video decoding backend vllm-project/vllm#46609
- 2: Replace
decordwithtorchcodecvllm-project/vllm#15022 - 3: https://github.com/vllm-project/vllm/blob/main/tools/install_torchcodec_rocm.sh
- 4: tenstorrent/vllm@6590a3e
- 5: https://deps.dev/pypi/vllm/0.22.1
- 6: https://pypi.org/project/vllm/
🌐 Web query:
vllm 0.24.0 PyPI JSON requires_dist torchcodec
💡 Result:
In vLLM version 0.24.0, torchcodec is utilized as an optional video decoding backend [1]. While torchcodec is integrated into the framework to support efficient video decoding [2][1], it is not listed as a mandatory dependency in the core PyPI package installation requirements (install_requires) [3]. The project previously moved to adopt torchcodec as a replacement for decord [4], but it is treated as an optional component that users may choose to install for specific functionality. Consequently, you will not find torchcodec in the requires_dist field of the vLLM 0.24.0 PyPI metadata [3]. Because torchcodec itself lacks a formal dependency constraint on PyTorch in its own PyPI metadata [5], it is typically managed as an optional environment dependency rather than a core package requirement [3]. Documentation and utility functions within vLLM, such as vllm.utils.import_utils.check_torchcodec_available, are provided to handle cases where this optional package is missing or not installed [6].
Citations:
- 1: https://github.com/vllm-project/vllm/releases
- 2: Add TorchCodec as a video decoding backend vllm-project/vllm#46609
- 3: https://github.com/vllm-project/vllm/blob/0d310ffbebe588972fb57b84b3ce564c0222ef4e/setup.py
- 4: Replace
decordwithtorchcodecvllm-project/vllm#15022 - 5: https://github.com/huggingface/skills/blob/main/skills/huggingface-zerogpu/references/cuda-and-deps.md
- 6: https://docs.vllm.ai/en/latest/api/vllm/utils/import_utils/
Install torchcodec before the import canary. The current vllm>=0.22.1 pin still allows releases whose metadata does not pull in torchcodec, so this can fail on a clean runner with ModuleNotFoundError. Install a Torch-compatible torchcodec explicitly, or raise the lower bound to a release that includes it in requires_dist.
🤖 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 58 - 63, Update the dependency
installation flow in scripts/ci_install_vllm.sh to explicitly install a
Torch-compatible torchcodec before the existing `python3 -c` import canary, or
raise the vLLM lower bound to a release whose metadata requires torchcodec.
Ensure the canary continues validating imports of torch, torchcodec, and vllm on
a clean runner.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08e4d4f8af
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Modality::Video => Self::replacements( | ||
| metadata, | ||
| preprocessed, | ||
| modality, | ||
| "video_token_id", | ||
| VIDEO_PAD_TOKEN, | ||
| ), |
There was a problem hiding this comment.
For Qwen3-Omni video inputs, this falls through to the generic replacement helper, which emits one flat run of <|video_pad|> tokens and leaves structural_prefix at 0. The Qwen3-VL spec handles the same video placeholder contract by using video_grid_thw to split multi-frame clips into per-frame vision blocks and by including the template-owned leading <|vision_start|> in the reported range (crates/multimodal/src/registry/qwen3_vl.rs:250-272), because the backend M-RoPE pass scans that range for frame markers. With a normal Omni video where video_grid_thw[0] > 1 (and even for backends that need the leading marker on single-frame videos), the placeholder metadata lacks those markers, so video position encoding can fail or be assigned to the wrong range; the Omni video branch should mirror the Qwen3-VL video replacement path.
Useful? React with 👍 / 👎.
Signed-off-by: chenht2022 <chenht2022@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/grpc/utils/chat_utils.rs (1)
252-269: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConvert media-only content to an empty string when anchors are absent.
With
placeholder_tokens == None, Line 253 skips every media part. For media-only input, both vectors remain empty and Line 266 leaves the original parts array in aStringtemplate instead of omitting media.Proposed fix
let mut media_parts: Vec<String> = Vec::new(); let mut text_parts: Vec<String> = Vec::new(); + let mut has_supported_media = false; for part in content_array { @@ Some(type_name @ ("image_url" | "video_url" | "audio_url" | "input_audio")) => { let modality = modality_for_chat_part(type_name).ok_or_else(|| { format!("unsupported media content part type: {type_name}") })?; + has_supported_media = true; let Some(tokens) = placeholder_tokens else { continue; }; @@ - if !media_parts.is_empty() || !text_parts.is_empty() { + if has_supported_media || !media_parts.is_empty() || !text_parts.is_empty() { let ordered: Vec<String> = media_parts.into_iter().chain(text_parts).collect(); *content_value = Value::String(ordered.join("\n")); }Add a media-only
Noneregression test expecting"".🤖 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 `@model_gateway/src/routers/grpc/utils/chat_utils.rs` around lines 252 - 269, Update the content conversion logic around placeholder_tokens and the media_parts/text_parts aggregation so media-only input with placeholder_tokens == None produces Value::String("") rather than preserving the original parts array; retain existing placeholder handling when tokens are available and add a regression test covering the media-only None case.
🤖 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 `@model_gateway/src/routers/grpc/router.rs`:
- Around line 100-107: Replace the iterative loop in sanitize_qwen3_asr_prompt
with a linear-time, stack-based single-pass sanitizer that removes all
chat/control tokens and ASR_TEXT_TAG occurrences without repeatedly rescanning
or reallocating the full prompt. Preserve the current sanitization result for
nested and repeated token inputs.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/utils/chat_utils.rs`:
- Around line 252-269: Update the content conversion logic around
placeholder_tokens and the media_parts/text_parts aggregation so media-only
input with placeholder_tokens == None produces Value::String("") rather than
preserving the original parts array; retain existing placeholder handling when
tokens are available and add a regression test covering the media-only None
case.
🪄 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: d2f3d7e4-9dc8-4ef0-b7b4-cc9cc66d8fbb
📒 Files selected for processing (9)
crates/multimodal/src/audio/decode.rscrates/multimodal/src/audio/processors/qwen3_audio.rscrates/multimodal/src/error.rscrates/multimodal/src/media.rscrates/multimodal/src/vision/processors/qwen3_omni_vision.rscrates/multimodal/src/vision/processors/qwen_vl_base.rsmodel_gateway/src/routers/grpc/regular/stages/messages/preparation.rsmodel_gateway/src/routers/grpc/router.rsmodel_gateway/src/routers/grpc/utils/chat_utils.rs
| fn sanitize_qwen3_asr_prompt(mut text: String) -> String { | ||
| loop { | ||
| let sanitized = strip_chatml_like_tokens(&text).replace(ASR_TEXT_TAG, ""); | ||
| if sanitized == text { | ||
| return text; | ||
| } | ||
| text = sanitized; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make prompt-control sanitization linear.
This repeatedly rescans and reallocates the complete prompt. Nested input such as "<|a".repeat(n) + "|>".repeat(n) removes one layer per pass, yielding O(n²) work on the request task. Use a stack-based single-pass sanitizer, or reject/cap pathological nesting.
🤖 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 `@model_gateway/src/routers/grpc/router.rs` around lines 100 - 107, Replace the
iterative loop in sanitize_qwen3_asr_prompt with a linear-time, stack-based
single-pass sanitizer that removes all chat/control tokens and ASR_TEXT_TAG
occurrences without repeatedly rescanning or reallocating the full prompt.
Preserve the current sanitization result for nested and repeated token inputs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d56bd7597f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let data = data.trim(); | ||
| let decoded = BASE64_STANDARD.decode(data)?; |
There was a problem hiding this comment.
When chat input_audio or the transcription adapter supplies audio, it reaches this data-URL path and decodes the entire base64 payload without applying audio_max_input_bytes(), unlike HTTP audio URLs which use collect_http_body_with_limit. A large inline audio request therefore allocates the base64 string plus decoded bytes and then enters the decoder before any input-size rejection, making the advertised audio input limit ineffective for the main inline-audio path.
Useful? React with 👍 / 👎.
| || ["model", "model_path", "tokenizer", "tokenizer_path"] | ||
| .iter() | ||
| .filter_map(|key| metadata.spec.labels.get(*key)) | ||
| .any(|value| is_qwen3_asr_identifier(value)) |
There was a problem hiding this comment.
For a Qwen3-ASR worker served under an alias or local path that does not literally contain qwen3-asr, worker discovery still reports the reliable model_type=qwen3_asr label, but this check only inspects path-like labels. In that deployment /v1/audio/transcriptions is rejected as unsupported before the chat pipeline runs, even though the registry and processor would handle the model; include model_type (and similar config labels) in this predicate.
Useful? React with 👍 / 👎.
Add an e2e test for POST /v1/audio/transcriptions against a TokenSpeed Qwen3-ASR worker, driven through the standard api_client (OpenAI SDK) / model fixtures like the other chat_completions e2e tests. Covers a whole-file transcription, the text response_format, and 400 rejection of an unsupported language. Runs in the existing e2e-1gpu-chat (tokenspeed) lane via the engine marker. Refs #1905 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
3 participants
Description
Problem
SMG's multimodal pipeline supports image and video inputs, but does not provide first-class audio preprocessing or model contracts. As a result, Qwen3-ASR and the Qwen3-Omni Thinker cannot be served through the existing multimodal request path.
Several shared stages, including media planning, placeholder expansion, batching, hashing, and TokenSpeed transport, also assume vision-oriented inputs and need to handle multiple modalities consistently.
Solution
Add audio as a first-class multimodal input and generalize the shared preprocessing and transport pipeline across image, video, and audio.
The implementation adds audio decoding and log-Mel preprocessing, registers Qwen3-ASR and Qwen3-Omni processors, and preserves authored media ordering through batching and itemized TokenSpeed transport.
Qwen3-Omni support is limited to the Thinker with text output (
return_audio=false). Talker, Code2Wav, audio-in-video, timestamps, generated audio, and audio EPD are out of scope.Changes
Test Plan
The following commands pass:
cargo test -p llm-multimodal --libcargo test -p smg --lib routers::grpc::multimodalcargo test -p smg --lib routers::grpc::router::testscargo test -p llm-tokenizer --test qwen2_vocab_mergescargo test -p openai-protocolcargo check -p smgcargo fmt --all -- --checkSummary by CodeRabbit
New Features
Bug Fixes