refactor(multimodal): move audio-processor selection onto ModelProcessorSpec - #1913
Conversation
…ssorSpec Audio-processor selection previously required the model-family spec name (e.g. "qwen3_asr") to be registered in two independent places -- the ModelRegistry spec AND a string-keyed AudioProcessorRegistry -- kept in sync by a free string with no compile-time guarantee, failing at runtime on any mismatch. Move audio-processor construction onto the spec via a new ModelProcessorSpec::audio_processor(model_config, preprocessor_config) method (default None; overridden by Qwen3AsrSpec and Qwen3OmniSpec to build the Qwen3 audio processor). The ModelRegistry lookup that already owns a model's prompt/placeholder logic now also owns its audio preprocessor, so the separate AudioProcessorRegistry and its stringly-typed create(spec.name(), ..) coupling are deleted. The call site in preprocess_modality now takes the resolved spec and calls spec.audio_processor(..) directly. Behavior is preserved: qwen3_asr/qwen3_omni resolve to the same Qwen3AudioProcessor as before, and the error message is unchanged. Vision selection is intentionally left on VisionProcessorRegistry: its substring matching is an independent matcher whose divergences from the spec (e.g. a bare "llava" model_type resolves a spec but no vision processor) are load-bearing for current behavior, so folding it into the spec would change outcomes. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
📝 WalkthroughWalkthroughAudio processor selection moved from a shared registry into ChangesAudio processor selection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MultimodalPlan
participant ModelProcessorSpec
participant Qwen3AudioProcessor
MultimodalPlan->>ModelProcessorSpec: pass spec to preprocess_modality
MultimodalPlan->>ModelProcessorSpec: request audio_processor
ModelProcessorSpec->>Qwen3AudioProcessor: construct processor
Qwen3AudioProcessor-->>MultimodalPlan: preprocess audio clips
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
There was a problem hiding this comment.
Code Review
This pull request refactors the audio preprocessor selection logic by removing the centralized AudioProcessorRegistry and instead delegating the creation of audio preprocessors directly to the ModelProcessorSpec trait implementations (such as Qwen3AsrSpec and Qwen3OmniSpec). This simplifies the design by making the model spec the single source of truth for audio processor selection. I have no feedback to provide as there are no review comments.
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.
…sorSpec (smg-project/smg#1913) Signed-off-by: Simo Lin <linsimo.mark@gmail.com> Signed-off-by: Bugen Zhao <i@bugenzhao.com>
…sorSpec (smg-project/smg#1913) Signed-off-by: Simo Lin <linsimo.mark@gmail.com> Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Description
Problem
#1905 left two overlapping model→audio-processor mechanisms:
ModelRegistry(family →ModelProcessorSpec) and a separateAudioProcessorRegistrykeyed by the spec-name string ("qwen3_asr"/"qwen3_omni"). That string must be registered in two independent places and kept in sync; a mismatch is a runtime error with no compile-time guarantee.Solution
Move audio-processor selection onto the spec:
ModelProcessorSpec::audio_processor(model_config, preprocessor_config) -> Option<Box<dyn AudioPreProcessor>>(defaultNone). The spec that owns a model's prompt/placeholder logic now owns its audio preprocessor, andAudioProcessorRegistryis deleted. Construction is identical (Qwen3AudioProcessor::from_configs(...)), so behavior is preserved. Vision keeps its ownVisionProcessorRegistryon purpose — folding it in would change behavior (specs match broadly, the vision registry narrowly).Changes
registry/traits.rs:audio_processortrait method (defaultNone).registry/{qwen3_asr,qwen3_omni}.rs: override to buildQwen3AudioProcessor.audio/processor.rs: deleteAudioProcessorRegistry/AudioProcessorFactory(keep theAudioPreProcessortrait).multimodal/{config,process}.rs: drop the registry; callspec.audio_processor(...).llavano-audio test.Test Plan
cargo test -p llm-multimodalandcargo test -p smg --lib routers::grpc::multimodalpass; audio construction is byte-identical to before (registry deletion only changes selection wiring, not preprocessing output).Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses