Conversation
Performance Comparison
|
|
The author of this PR, rui-ren, is not an activated member of this organization on Codecov. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
🏗️ Architecture Diff
No architecture changes detected. ✅ Legend: ⚪ No change · 🔵 Minor (attrs/inits) · 🟡 Moderate (nodes added/removed) · 🔴 Major (interface changed) |
|
@copilot can you please use this skill of Currently I am only working on the |
Reviewed the thinker work at d5a4ec9 using the
|
Pick up the partial Qwen2.5-Omni onboarding (#193) and bring it to a state where the file imports cleanly, the full pytest suite collects without errors, and the registry-completeness test passes. Concrete fixes: * Syntax: removed stray ")" at end of Qwen25OmniDecoderModel that broke every test that imported the module (65 collection errors on plain main). * Missing imports: RMSNorm, DecoderLayer, initialize_rope were referenced by Qwen25OmniDecoderModel but not imported. Also consolidated the OpBuilder import (was using the internal onnxscript._internal.builder module). * Typos in Qwen25OmniEmbeddingModel: - Used audio.image_token_id (image_token_id lives on VisionConfig). - self._image_token_ids plural typo where _image_token_id was meant. - "return input_embeds" referenced a non-existent local; renamed. - Forward arg "inputs_ids" -> "input_ids". * Composite class implemented: Qwen25OmniThinkerForConditionalGeneration was just `pass`. Now mirrors the gemma4 multi-encoder layout — owns decoder, embedding, vision_encoder, and audio_tower (named to match what SpeechLanguageTask looks up). forward() raises NotImplementedError because this is a multi-output split needing a dedicated task. preprocess_weights maps HF thinker.audio_tower.* / thinker.visual.* / thinker.model.* / thinker.lm_head.* onto the new module structure, drops Talker / codec-head keys, and applies the standard embed/lm_head weight tying. Test infrastructure: * Added qwen2_5_omni to _KNOWN_UNTESTED_MODEL_TYPES with a comment explaining the situation: the audio + vision + text 4-model split needs a dedicated task (like Gemma4Task); the existing speech-language task only drives audio + text. When that task is added in a follow-up, this entry should move into a dedicated graph test (mirroring test_gemma4_any_to_any_graph). Also cleaned up: unused n_window lookup, unused dataclasses import, trailing blank lines, and audio-attention docstring punctuation that ruff flagged. Tests: full tests/build_graph_test.py + src/mobius/ suite passes (2767 passed, 43 skipped). Ruff check + format clean on touched files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Justin Chu <11205048+justinchuby@users.noreply.github.com>
|
@copilot update to the latest and ensure the implementation is clean and correct |
Signed-off-by: GitHub <noreply@github.com>
Add a dedicated four-model task and align audio, vision, decoder, configuration, and weight routing with the current Hugging Face Thinker implementation. Cover the nested config and complete graph package with focused regression tests. Signed-off-by: GitHub <noreply@github.com>
There was a problem hiding this comment.
Pull request overview
This PR onboards the Qwen2.5-Omni (Thinker) architecture into mobius, implementing a four-model ONNX split (audio encoder, vision encoder, embedding, decoder) and wiring it into the registry/config extraction paths.
Changes:
- Added
Qwen25OmniThinkerForConditionalGeneration+Qwen25OmniTaskto build a 4-partModelPackage(audio_encoder,vision_encoder,embedding,decoder). - Implemented new Omni audio encoder components and a generic
Conv1dbuilding block. - Extended config extraction to pull Omni’s nested
thinker_config.vision_config(includingvideo_token_id) and registered the new model/task in the public APIs and registry.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/build_graph_test.py | Adds a graph-build test for the 4-model package and marks the model type as “specialized” for registry completeness. |
| src/mobius/tasks/_qwen25_omni.py | New task to build the Omni 4-model split (audio/vision/embedding/decoder). |
| src/mobius/tasks/init.py | Exposes Qwen25OmniTask and registers the task name qwen25-omni. |
| src/mobius/models/qwen25_omni.py | New Thinker model implementation (audio + vision + embedding fusion + decoder) and HF weight routing. |
| src/mobius/models/qwen25_omni_test.py | Unit tests for nested-config extraction and preprocess_weights() routing rules. |
| src/mobius/models/init.py | Exports Qwen25OmniThinkerForConditionalGeneration. |
| src/mobius/components/_qwen25_omni_audio.py | Adds Omni packed audio attention + encoder layer components. |
| src/mobius/components/_conv.py | Adds a reusable Conv1d component. |
| src/mobius/components/init.py | Exposes Omni audio components via the public components API. |
| src/mobius/_registry.py | Registers model type qwen2_5_omni with task qwen25-omni. |
| src/mobius/_configs/per_model/_qwen25_omni_vision.py | Adds a per-model hook to extract vision config from thinker_config. |
| src/mobius/_configs/per_model/init.py | Imports the new Omni vision extractor hook. |
| src/mobius/_configs/_sub_configs.py | Adds video_token_id to VisionConfig. |
| src/mobius/_configs/_extractors.py | Lifts video_token_id as a shared vision field. |
| src/mobius/_configs/_base.py | Adds top-level video_token_id and recognizes qwen2_5_omni_text for config extraction. |
| q = op.Unsqueeze(op.Transpose(q, perm=[1, 0, 2]), [0]) | ||
| k = op.Unsqueeze(op.Transpose(k, perm=[1, 0, 2]), [0]) | ||
| v = op.Unsqueeze(op.Transpose(v, perm=[1, 0, 2]), [0]) | ||
| attn_output = op.Attention( | ||
| q, |
| import onnx_ir as ir | ||
| from onnxscript import nn | ||
| from onnxscript._internal import builder | ||
|
|
| def build(self, module: nn.Module, config: ArchitectureConfig) -> ModelPackage: | ||
| self._validate_components(module) | ||
| models = { | ||
| "audio_encoder": self._build_audio(module.audio_encoder, config), | ||
| "vision_encoder": self._build_vision(module.vision_encoder, config), | ||
| "embedding": self._build_embedding(module.embedding, config), | ||
| "decoder": build_decoder_from_embeds(module.decoder, config, mrope=True), | ||
| } | ||
| return ModelPackage(models, config=config) |
Updated onto latest |
|
@copilot resolve the conflicts and feedback |
Resolve config and registration conflicts while retaining both model sets. Move the Thinker graph test to the split build_graph suite introduced on main. Signed-off-by: GitHub <noreply@github.com> Co-authored-by: rui-ren <15321482+rui-ren@users.noreply.github.com>
|
ruiren_microsoft seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Use public builder types and rank-3 audio attention, validate required encoders, and respect the configured audio input dtype. Match Hugging Face's final audio normalization epsilon and correct component documentation. Add shared tiny-config coverage and offline HF parity for packed audio, vision, batched modality fusion, and cached MRoPE decoding. Signed-off-by: GitHub <noreply@github.com> Co-authored-by: rui-ren <15321482+rui-ren@users.noreply.github.com>
Keep the composite parent for audio and vision extraction while choosing Thinker text config instead of the sibling Talker. Preserve serialized token-index aliases in raw-config fallback and cover both full-checkpoint paths plus unchanged Qwen3-TTS selection. Signed-off-by: GitHub <noreply@github.com> Co-authored-by: rui-ren <15321482+rui-ren@users.noreply.github.com>
Resolved in
Final checks: 2,259 passed, 53 skipped, 40 xfailed; lint, secret scans, and scoped review pass. Combined validation/CodeQL could not complete because its merge-diff generation repeatedly timed out. |
There was a problem hiding this comment.
🟡 Changes recommended
The audio ABI is not processor-native, and packed attention currently creates a quadratic mask.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/mobius/tasks/_qwen25_omni.py:63
- Use a float32 processor boundary here. The standard audio processor emits float32, while fp16/bf16 exports currently require callers to manufacture reduced-precision inputs (NumPy cannot directly supply bfloat16);
Qwen25OmniAudioEncoder.forwardalready casts once to the convolution weight dtype. Keep the graph input FLOAT and update the dtype assertion accordingly.
dtype=config.dtype,
- Files reviewed: 19/19 changed files
- Comments generated: 3
- Review effort level: Balanced
| input_features = builder.input( | ||
| "input_features", | ||
| dtype=config.dtype, | ||
| shape=[num_chunks, n_mels, chunk_len], | ||
| ) |
| attention_bias = op.Where( | ||
| same_segment, | ||
| op.CastLike(0.0, q), | ||
| op.CastLike(-1e9, q), | ||
| ) | ||
| attention_bias = op.Unsqueeze(attention_bias, [0, 1]) |
| _builder, "_load_transformers_config", lambda *args, **kwargs: (hf_config, raw_json) | ||
| ) | ||
|
|
||
| package = _builder.build_transformers_model("test/qwen25-omni", load_weights=False) |
titaiwangms
left a comment
titaiwangms
left a comment
There was a problem hiding this comment.
I completed a full review of the Qwen2.5-Omni onboarding at 8dd555c8.
The core audio/vision/text math is generally faithful to the Hugging Face
implementation, but I found several integration blockers.
Major findings
-
ORT GenAI export generates an unsupported Omni configuration
src/mobius/integrations/ort_genai/auto_export.py:1615-1785The package is detected as both VLM and speech, but Qwen2.5-Omni is not
registered in the Qwen-VL/model-type mappings. It therefore takes the
generic speech path, which identity-maps graph inputs such as
input_features,chunk_lengths, andpool_indices. The code itself notes
that ORT GenAI's speech input keys are a closed set and reject arbitrary
graph input names._write_audio_processor_config()also has no Omni branch.Until ORT GenAI has a defined Qwen2.5-Omni Thinker contract, please fail
explicitly for--runtime ort-genairather than emit a package that is
expected to fail at load time. -
The registration violates the repository's L2 coverage contract
src/mobius/_registry.py:967-970
tests/model_coverage_test.py:505-537qwen2_5_omnihas neither atest_model_idnor an entry in
_COVERAGE_SKIP.TestL2ConfigValidationwill therefore fail
deterministically. Add a pinnedtest_model_id, or add a documented skip
consistent with the other large audio/multimodal models. -
The four-component model does not declare
HF_COMPONENT_SOURCESsrc/mobius/models/qwen25_omni.py:425-526
src/mobius/_component_manifest.py:143-183Component inspection and component-aware quantization obtain source
ownership fromHF_COMPONENT_SOURCES. Without it, the four declared roles
have empty HF source paths andthinker.*tensors cannot be attributed to
the correct component.The mapping should cover at least:
audio_encoder:thinker.audio_towervision_encoder:thinker.visualembedding:thinker.model.embed_tokensdecoder:thinker.model.layers,thinker.model.norm,
thinker.model.rotary_emb, andthinker.lm_head
-
The onnx-genai workflow has no producer for
video_featuressrc/mobius/integrations/onnx_genai/workflow_metadata.py:6330-6365
src/mobius/tasks/_qwen25_omni.py:103-124produced_featurescontains the vision encoder'simage_featuresand the
audio outputs. The embedding graph additionally requiresvideo_features,
so workflow generation turns it into a required application request input
instead of invoking the shared vision tower for video or defining an
explicit packed image/video contract. The generated workflow therefore
does not provide an end-to-end raw-video path. -
The implementation duplicates existing Qwen-family architecture and task
contracts, and the copies have already driftedThe Omni-specific audio frontend is justified, but the PR separately
reimplements the decoder-from-embeddings loop, multimodal token replacement,
four-model task plumbing, andConv1d, despite existing equivalents in
Qwen2.5-VL, Qwen3-ASR,Qwen2VLMultimediaTask, and the component library.This is not only a maintainability concern: the duplicated paths are where
the missing component source metadata, incomplete video workflow, incorrect
generic ORT GenAI routing, and unchecked feature-width assumptions occur.Please retain the Omni-specific audio chunking/pooling implementation, but
reuse or extract shared primitives for:- decoder-from-embeddings with MRoPE;
- ordered audio/image/video placeholder replacement;
- Qwen-VL vision block/attention factories;
- audio+vision four-component task construction;
- the existing public
Conv1d.
Additional correctness issues
src/mobius/tasks/_qwen25_omni.py:103-116declares every feature input with
widthconfig.hidden_size, while the encoders produce
audio.output_dimandvision.out_hidden_size. Validate equality at build
time or use the actual encoder dimensions.Qwen25OmniEmbeddingModel._replace_tokens()does not validate that the
number of placeholder tokens equals the number of feature rows. Too few
rows produce an opaqueGatherbounds failure; excess rows are ignored.- The audio config exposes
activation_function, but the implementation
always emits GELU. Reject unsupported values or dispatch through the
configured activation. vc.fullatt_block_indexes or (7, 15, 23, 31)treats a valid empty list as
missing and silently changes an all-windowed configuration._conv.pyadds a secondConv1deven though the component library already
exposes one with the required behavior.
The audio convolution, pooling-index arithmetic, packed cu_seqlens,
sinusoidal positional embeddings, projection-bias asymmetry, fp16 clamp, and
Omni-specific Qwen2.5-VL vision parameter layout were checked against the
reference implementation and appear correct.
This was a static review. I did not execute the PR tests or perform downstream
ORT GenAI/onnx-genai runtime loading.
Adds Qwen2.5-Omni Thinker support with: