feat: Add support for Qwen 2.5-Omni model bridge - #2296
Conversation
📝 WalkthroughWalkthroughThis PR introduces the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Qwen25OmniBridge
participant ConfigExtractor
participant Qwen25VLModelProvider
participant MappingRegistry
Client->>Qwen25OmniBridge: provider_bridge(hf_pretrained)
Qwen25OmniBridge->>ConfigExtractor: extract thinker_config & text_config
ConfigExtractor-->>Qwen25OmniBridge: configs (or error if missing)
Qwen25OmniBridge->>Qwen25OmniBridge: validate configs present
Qwen25OmniBridge->>Qwen25OmniBridge: derive mRoPE & rope_theta
Qwen25OmniBridge->>Qwen25OmniBridge: populate vision/token IDs<br/>& dtype settings
Qwen25OmniBridge->>Qwen25VLModelProvider: construct with all params
Qwen25VLModelProvider-->>Qwen25OmniBridge: provider instance
Qwen25OmniBridge-->>Client: configured Qwen25VLModelProvider
Client->>Qwen25OmniBridge: mapping_registry()
Qwen25OmniBridge->>MappingRegistry: construct with layer & multimodal mappings
MappingRegistry-->>Client: MegatronMappingRegistry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/megatron/bridge/models/qwen_vl/qwen25_omni_bridge.py`:
- Around line 33-39: Update the class docstring to correctly reference the
bridge target Qwen25VLModel instead of "Megatron-Core GPTModel formats"; locate
the docstring for the Qwen 2.5-Omni Bridge (the class decorated with the target
Qwen25VLModel) and replace the inaccurate phrase so it describes conversion
between HuggingFace Qwen2_5OmniForConditionalGeneration and Qwen25VLModel
formats, including weight mappings and configuration translation for multimodal
models.
- Around line 107-109: The current assignment uses "or" which treats 0 as falsy
and can drop a legitimate token id; change the logic for image_token_id and
video_token_id to prefer thinker_config.image_token_id / image_token_index only
when the first is None (e.g., read getattr(..., None) into a variable and if
that variable is None then use getattr(..._index, <default>)); update references
to image_token_id and video_token_id accordingly. Also address the long
commented-out audio_token_id line: move the explanatory note into a separate
shorter comment above the commented-out declaration (or shorten it) so the line
stays under 119 chars and clearly documents why audio_token_id is commented out.
🧹 Nitpick comments (4)
src/megatron/bridge/models/qwen_vl/qwen25_omni_bridge.py (1)
86-103: Consider extracting rope parameter resolution into a helper.The
mrope_sectionandrope_thetaextraction blocks share the same pattern (trytext_configattr → tryrope_parametersdict → tryrope_parametersobject attr → default). A small helper like_get_rope_param(text_config, key, default)would reduce the nesting and duplication.tests/unit_tests/models/qwen_vl/test_qwen25_omni_bridge.py (3)
358-514: Extract the repeated mapping-names collection into a helper.The same ~10-line block that gathers
mapping_namesfromregistry.mappingsis copy-pasted across 8 test methods (test_mapping_registry_contains_required_mappings,test_mapping_registry_thinker_model_mappings,test_mapping_registry_visual_params, etc.). Extract it into a helper function or fixture to reduce duplication.Example helper extraction
+def _collect_mapping_names(registry): + """Collect all Megatron and HF param names from the registry mappings.""" + names = [] + for mapping in registry.mappings: + if hasattr(mapping, "megatron_param"): + names.append(str(mapping.megatron_param)) + hf = getattr(mapping, "hf_param", None) + if isinstance(hf, dict): + names.extend(str(v) for v in hf.values()) + elif isinstance(hf, str): + names.append(hf) + return names + + class TestQwen25OmniBridgeMappingRegistry: """Test mapping_registry method functionality.""" ... def test_mapping_registry_contains_required_mappings(self, qwen25_omni_bridge): """Test mapping_registry contains all required parameter mappings.""" registry = qwen25_omni_bridge.mapping_registry() - - # Extract mappings - registry should contain mappings for common parameters - mappings = registry.mappings - assert len(mappings) > 0 - - # Check that we have mappings for embeddings, output layer, layernorms - mapping_names = [] - for mapping in mappings: - # Collect Megatron param pattern - if hasattr(mapping, "megatron_param"): - mapping_names.append(str(getattr(mapping, "megatron_param"))) - # Collect HF param pattern(s) - hf = getattr(mapping, "hf_param", None) - if isinstance(hf, dict): - mapping_names.extend([str(v) for v in hf.values()]) - elif isinstance(hf, str): - mapping_names.append(hf) + assert len(registry.mappings) > 0 + mapping_names = _collect_mapping_names(registry) # Should contain word embeddings mapping (thinker.model.embed_tokens) has_embeddings = any("embed_tokens" in name or "word_embeddings" in name for name in mapping_names)Then apply the same pattern to the other 7 test methods.
28-45: Mock auto-attributes may leak into tests as unexpected values.
Mock()auto-creates attributes on access, sohasattr(text_config, "rope_parameters")returnsTrueeven thoughrope_parameterswas never explicitly set. This means the bridge'smrope_sectionextraction path will execute against a nested Mock chain, resulting in a Mock object (not a list orNone) being assigned tomrope_sectionin tests that don't explicitly overriderope_parameters.The targeted mRoPE tests handle this via
delattr, but the basic config tests silently pass a Mock asmrope_sectionto the provider. Consider usingspec=on the Mock or explicitly initializingtext_config.rope_parameters = Nonein the fixture to make the default behavior deterministic.Proposed fix
`@pytest.fixture` def mock_text_config(): """Create a mock text_config for Qwen2.5-Omni.""" text_config = Mock() text_config.num_hidden_layers = 32 text_config.hidden_size = 4096 text_config.intermediate_size = 11008 text_config.num_attention_heads = 32 text_config.num_key_value_heads = 32 text_config.initializer_range = 0.02 text_config.rms_norm_eps = 1e-6 text_config.vocab_size = 151936 text_config.max_position_embeddings = 4096 text_config.rope_theta = 1000000.0 text_config.tie_word_embeddings = False text_config.bos_token_id = 151643 text_config.eos_token_id = 151645 + text_config.rope_parameters = None return text_config
85-86: Missingpytest.markcategorization on test classes.Per coding guidelines, tests should be categorized with
pytest.mark(e.g.,@pytest.mark.unit). None of the test classes in this file have markers.As per coding guidelines: "Use 'pytest.mark' to categorize tests (unit, integration, system)."
| """ | ||
| Megatron Bridge for Qwen 2.5-Omni Conditional Generation. | ||
|
|
||
| This bridge handles the conversion between HuggingFace Qwen2_5OmniForConditionalGeneration | ||
| and Megatron-Core GPTModel formats, including weight mappings and | ||
| configuration translation for multimodal models. | ||
| """ |
There was a problem hiding this comment.
Inaccurate docstring: references GPTModel instead of Qwen25VLModel.
The class docstring mentions "Megatron-Core GPTModel formats" but the bridge target (as declared in the decorator on line 31) is Qwen25VLModel.
Proposed fix
"""
Megatron Bridge for Qwen 2.5-Omni Conditional Generation.
This bridge handles the conversion between HuggingFace Qwen2_5OmniForConditionalGeneration
- and Megatron-Core GPTModel formats, including weight mappings and
+ and Megatron-Core Qwen25VLModel formats, including weight mappings and
configuration translation for multimodal models.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """ | |
| Megatron Bridge for Qwen 2.5-Omni Conditional Generation. | |
| This bridge handles the conversion between HuggingFace Qwen2_5OmniForConditionalGeneration | |
| and Megatron-Core GPTModel formats, including weight mappings and | |
| configuration translation for multimodal models. | |
| """ | |
| """ | |
| Megatron Bridge for Qwen 2.5-Omni Conditional Generation. | |
| This bridge handles the conversion between HuggingFace Qwen2_5OmniForConditionalGeneration | |
| and Megatron-Core Qwen25VLModel formats, including weight mappings and | |
| configuration translation for multimodal models. | |
| """ |
🤖 Prompt for AI Agents
In `@src/megatron/bridge/models/qwen_vl/qwen25_omni_bridge.py` around lines 33 -
39, Update the class docstring to correctly reference the bridge target
Qwen25VLModel instead of "Megatron-Core GPTModel formats"; locate the docstring
for the Qwen 2.5-Omni Bridge (the class decorated with the target Qwen25VLModel)
and replace the inaccurate phrase so it describes conversion between HuggingFace
Qwen2_5OmniForConditionalGeneration and Qwen25VLModel formats, including weight
mappings and configuration translation for multimodal models.
| image_token_id = getattr(thinker_config, "image_token_id", None) or getattr(thinker_config, "image_token_index", 151655) | ||
| video_token_id = getattr(thinker_config, "video_token_id", None) or getattr(thinker_config, "video_token_index", 151656) | ||
| # audio_token_id = getattr(thinker_config, "audio_token_id", None) or getattr(thinker_config, "audio_token_index", 151646) # audio_token_id is not used by Qwen25VLModelProvider, which only supports image and video tokens |
There was a problem hiding this comment.
Falsy token ID (e.g. 0) would silently fall through to the fallback.
The or operator treats 0 as falsy, so if image_token_id or video_token_id is legitimately 0, it would be ignored in favor of the *_token_index fallback or the hardcoded default. While token ID 0 is unlikely for Qwen special tokens, using is None checks would be more robust.
Also, line 109 (commented-out audio_token_id) far exceeds the 119-character line limit. Per coding guidelines, commented-out code should include a comment describing why it's commented out — the inline note is there but the line is too long. Consider moving the explanation to a standalone comment line above.
Proposed fix
- image_token_id = getattr(thinker_config, "image_token_id", None) or getattr(thinker_config, "image_token_index", 151655)
- video_token_id = getattr(thinker_config, "video_token_id", None) or getattr(thinker_config, "video_token_index", 151656)
- # audio_token_id = getattr(thinker_config, "audio_token_id", None) or getattr(thinker_config, "audio_token_index", 151646) # audio_token_id is not used by Qwen25VLModelProvider, which only supports image and video tokens
+ image_token_id = getattr(thinker_config, "image_token_id", None)
+ if image_token_id is None:
+ image_token_id = getattr(thinker_config, "image_token_index", 151655)
+ video_token_id = getattr(thinker_config, "video_token_id", None)
+ if video_token_id is None:
+ video_token_id = getattr(thinker_config, "video_token_index", 151656)
+ # audio_token_id is not used by Qwen25VLModelProvider,
+ # which only supports image and video tokens.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| image_token_id = getattr(thinker_config, "image_token_id", None) or getattr(thinker_config, "image_token_index", 151655) | |
| video_token_id = getattr(thinker_config, "video_token_id", None) or getattr(thinker_config, "video_token_index", 151656) | |
| # audio_token_id = getattr(thinker_config, "audio_token_id", None) or getattr(thinker_config, "audio_token_index", 151646) # audio_token_id is not used by Qwen25VLModelProvider, which only supports image and video tokens | |
| image_token_id = getattr(thinker_config, "image_token_id", None) | |
| if image_token_id is None: | |
| image_token_id = getattr(thinker_config, "image_token_index", 151655) | |
| video_token_id = getattr(thinker_config, "video_token_id", None) | |
| if video_token_id is None: | |
| video_token_id = getattr(thinker_config, "video_token_index", 151656) | |
| # audio_token_id is not used by Qwen25VLModelProvider, | |
| # which only supports image and video tokens. |
🤖 Prompt for AI Agents
In `@src/megatron/bridge/models/qwen_vl/qwen25_omni_bridge.py` around lines 107 -
109, The current assignment uses "or" which treats 0 as falsy and can drop a
legitimate token id; change the logic for image_token_id and video_token_id to
prefer thinker_config.image_token_id / image_token_index only when the first is
None (e.g., read getattr(..., None) into a variable and if that variable is None
then use getattr(..._index, <default>)); update references to image_token_id and
video_token_id accordingly. Also address the long commented-out audio_token_id
line: move the explanatory note into a separate shorter comment above the
commented-out declaration (or shorten it) so the line stays under 119 chars and
clearly documents why audio_token_id is commented out.
|
@martinzhang03 : thanks for contribution! It all looks good, but we are having a refactor on-going right now. Do you think you can rebase on top of https://github.com/NVIDIA-NeMo/Megatron-Bridge/pull/2250/changes? |
|
@martinzhang03 Do you have any plans to support the Qwen Omni audio modality? If not, I would be happy to further integrate the audio modality based on your PR. |
Hi @yaoyu-33 Thanks for the feedback. I'm currently working on rebasing my branch on top of PR #2250 and will ping you once its complete. |
Hi @yuekaizhang Thanks. I don't have immediate plans to implement the audio modality. I would definitely welcome your help in further integration once the PR is stabilized. Please feel free to build upon this work. |
7d5ebd5 to
7302588
Compare
Done. |
|
Hi @yaoyu-33 I have updated the PR to include full Sequence Parallelism (SP) support for the Omni model. This addition complements the base model registration by providing the high-performance training infra needed for industrial scale-up. I've also updated the PR description with the technical details. |
Signed-off-by: Lianglipeng <lianglipeng@didiglobal.com> Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: Lianglipeng <lianglipeng@didiglobal.com> Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: Lianglipeng <lianglipeng@didiglobal.com> Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
Signed-off-by: hbhflw2000 <417911774@qq.com>
… initializer in bridge file. add unit test files Signed-off-by: root <martinzhang0314@gmail.com>
…p support Signed-off-by: martinzhang03 <martinzhang0314@gmail.com>
- Export all Qwen2.5-VL finetune recipes plus qwen25_omni_7b_* from megatron.bridge.recipes.qwen_vl (and top-level recipes via star import). - Update finetune_qwen_vl.py: Omni in docstring, --recipe help, example command, and log label Qwen2.5-Omni vs Qwen2.5-VL. - Document Qwen2.5-Omni in docs/models/vlm/qwen2.5-vl.md (bridge, recipes, TP/SP defaults, HF card links). - Note Qwen2.5-Omni in Qwen25VLCommonKwargs TypedDict docstring. - Add test_qwen25_omni_recipes.py smoke tests with AutoBridge mocked. Made-with: Cursor
…ker/token2wav), with conversion smoke, roundtrip checks, functional and launch coverage, and VLM docs. Omni bridge moves out of qwen_vl into the dedicated package.
… collectives) - torchrun_main: write TORCHELASTIC error JSON; avoid os._exit for diagnostics - hf_megatron_roundtrip: RuntimeError when WORLD_SIZE unset - qwen25_omni_provider: local GPT layers + persist_layer_norm alignment - rope: distributed CP group fallback - conversion test: sys.executable, log_dir, NCCL socket defaults, error.json dump - model_bridge/peft_bridge: skip all_gather_object when PP group size is 1 - auto_bridge: barrier only world_size>1; NCCL barrier with device_ids - download_unit_tests_dataset: optional GH_TOKEN / clearer errors - pyproject/uv.lock: sync deps as needed Made-with: Cursor
9c929a9 to
8fff03e
Compare
|
https://github.com/NVIDIA-NeMo/Megatron-Bridge/tree/main/src/megatron/bridge/models/qwen_omni we added qwen25 omni in another pr. |
Summary
This PR adds Qwen2.5-Omni support to Megatron-Bridge: HF thinker / talker / token2wav alignment, conversion bridge, recipes, docs, and tests.
Stacking / rebase note: This branch is built on top of community PR #2831 — Qwen3-Omni (Omni3 multimodal layout, tests, and shared
qwen_omnidirection). Qwen2.5-Omni work extends that foundation rather than duplicating parallel scaffolding.Head branch workflow: Development continued on
feat/omni-sp(branched from the original PR headfeat/omni-support). The fork branchfeat/omni-supportwas updated to matchfeat/omni-spso PR #2296 continues to use the same head branch name while including all latest commits.What changed (high level)
Qwen2.5-Omni package and integration
megatron.bridge.models.qwen_omni: dedicated package for Qwen2.5-Omni (provider, bridge, modeling for thinker/talker/token2wav), with registration and exports aligned with the Omni3 layout from [model] Add initial Qwen3-Omni support in Megatron-Bridge #2831.persist_layer_normaligned with torch norm when using local layers (replace(..., persist_layer_norm=False)on the non-TE path).torchrun_mainwrites TorchElastic-compatibleerror.jsonwhere possible (including non-Exceptionexits), prefers normal process exit over hardos._exitfor better diagnostics; conversion script avoidssys.exitfor the “not launched under torchrun” path in favor ofRuntimeErrorso@recordcan capture failures.Functional test and subprocess harness
tests/functional_tests/models/qwen_omni/test_qwen25_omni_conversion.py: runstorch.distributed.runwithsys.executable,--log_dir+-r 3, nocoverage run --parallel-mode(avoids opaqueerror_file: <N/A>); printserror.jsonon failure; sets conservativeNCCL_IB_DISABLE/NCCL_NETdefaults for single-node functional runs on hosts with broken IB / gIB plugin expectations.Conversion / distributed edge cases (single rank / PP=1)
model_bridge/peft_bridge: skiptorch.distributed.all_gather_objectwhen pipeline-parallel group size is 1 (avoids observed crashes on some PyTorch/NCCL stacks).auto_bridge.save_hf_weights:dist.barrier()only whenworld_size > 1; for NCCL multi-rank collectives, usebarrier(device_ids=[torch.cuda.current_device()])where appropriate to avoid device-guessing warnings and segfaults seen on T4-class single-node runs.Misc
thinker_configserialization viato_dict(),spk_dict.ptfor audio-off reload, tokenizer sidecar files as needed.GH_TOKEN, clearer failure behavior).Tests run (local, all passed)
Commands and results (GPU VM,
uv run --no-sync pytest):pytest -v tests/functional_tests/models/qwen_omni/test_qwen25_omni_conversion.pypytest -v tests/unit_tests/models/qwen_omni/modeling_qwen25_omni/test_omni_model.pypytest -v tests/unit_tests/recipes/qwen_vl/test_qwen25_omni_recipes.pyRelationship to PR #2831 (Omni3)
If #2831 is updated before merge, this branch may need a follow-up rebase to stay aligned.
Future work / follow-ups
MEGATRON_BRIDGE_ENABLE_COVERAGE=1) once the default debug-first subprocess harness is stable.Maintenance
This PR description will be edited again if additional commits land on the head branch (e.g. review feedback, rebase onto updated
mainor updated #2831).