[recipe] feat: Add Qwen3.5 LLM-only SFT recipes with skip_megatron_pa… - #3037
Closed
cuichenx wants to merge 16 commits into
Closed
[recipe] feat: Add Qwen3.5 LLM-only SFT recipes with skip_megatron_pa…#3037cuichenx wants to merge 16 commits into
cuichenx wants to merge 16 commits into
Conversation
…ram_globs Add skip_megatron_param_globs to load_weights_hf_to_megatron and AutoBridge, allowing selective parameter skipping during HF→Megatron weight loading via fnmatch patterns. Use this mechanism to create Qwen3.5 LLM-only SFT recipes that load language-model weights from Qwen3.5-VL checkpoints while skipping the vision tower (*vision_model*). Recipes cover 800M through 397B-A17B model sizes. Includes shell wrapper (llm_sft.sh), README docs, and unit tests. Signed-off-by: Chen Cui <chcui@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
- llm_sft.sh: REPO_ROOT navigated 3 levels up instead of 4, landing at examples/ instead of the repository root. - qwen35_llm.py: pack_sequences_in_batch=True conflicted with the traditional packed-sequence mode (MBS=1) inherited from _sft_common(). Changed to False to match the VL recipe pattern. Also fixed 35B-A3B micro_batch_size from 2 to 1 for consistency. - test_megatron_param_skip_globs.py: mock.Mock() does not support the context-manager protocol required by hide_teacher_model(); switched to mock.MagicMock(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Chen Cui <chcui@nvidia.com>
yaoyu-33
added
feature
Resolutions: - src/megatron/bridge/models/conversion/auto_bridge.py: keep both (skip_megatron_param_globs param + unquantized_state_dict capture) - src/megatron/bridge/models/conversion/model_bridge.py: keep both (megatron_param_matches_skip_globs helper + _HFNameSuffixMapping FP8 wrapper) - src/megatron/bridge/recipes/qwen_vl/__init__.py: keep both __all__ blocks (qwen35_llm_*_sft_config + qwen35_vl_*_pretrain_mock_config) Signed-off-by: Chen Cui <chcui@nvidia.com>
…skip test T1.4 (gap to fill from verification plan): test_stream_weights_hf_to_megatron_respects_skip_globs in tests/unit_tests/models/test_megatron_param_skip_globs.py — verifies stream_weights_hf_to_megatron honours skip_megatron_param_globs using the same mock-task approach as the load variant (T1.3). T2.2 (Tier-2 GPU functional test): tests/functional_tests/models/qwen_vl/test_qwen35_llm_sft_skip_vision.py — verifies with a real Qwen3.5-0.8B checkpoint that the stream yields no vision_model.* parameters when skip globs are active, and that language params are still present (skip is not over-eager). No distributed init required; Megatron modules are mocked. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Chen Cui <chcui@nvidia.com>
…-only recipe Verifies that after the first training step with qwen35_llm_800m_sft_config: - All vision tower parameters have requires_grad=False (frozen) - Language model parameters are trainable - Peak GPU memory is reported Uses an isolated temp checkpoint directory to avoid resuming from prior runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Chen Cui <chcui@nvidia.com>
… recipe T2.6 (weight parity): verifies that DirectMapping LM params (embedding, final layernorm) are bitwise-identical to the HF source weights after loading via qwen35_llm_800m_sft_config, confirming skip_megatron_param_globs is not over-eager. T2.7 (checkpoint resume): runs 3 training steps, saves checkpoint, resumes for 2 more steps, and asserts the resumed run starts at step 3 (not 0) with all 5 losses finite. T2.8 (TP/PP smoke): runs 2 training steps under TP=2 and PP=2 parallelism (set via MODE env var). The callback captures losses only on the last pipeline stage to handle the PP case where intermediate stages have no loss value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Chen Cui <chcui@nvidia.com>
cuichenx
marked this pull request as ready for review
May 5, 2026 01:34
Contributor
Author
|
/ok to test 6d59e64 |
Contributor
Light Code ReviewMissing CI launch script: The PR description lists tests/functional_tests/launch_scripts/h100/active/L1_Launch_qwen35_llm_sft.sh in the changelog, but this file is not in the diff. The 5 functional test scripts under tests/functional_tests/models/qwen_vl/test_qwen35_llm_*.py will not run in CI without a corresponding launch script. Please add it or remove it from the changelog. Minor: The README examples and llm_sft.sh use uv run torchrun rather than uv run python -m torch.distributed.run, which is the project convention per CLAUDE.md. Not blocking, but worth aligning for consistency. Code: The core skip_megatron_param_globs implementation in model_bridge.py and threading through auto_bridge.py looks correct. The megatron_param_matches_skip_globs helper properly checks both global and local param names against fnmatch patterns, and the skip is applied in both load_weights_hf_to_megatron and stream_weights_hf_to_megatron at the right point (after the megatron_module is None guard). Unit test coverage for the helper and both code paths is solid. Suggested test cases: No perf tests impacted. |
…ch script Moves the T2.2/T2.5/T2.6/T2.7/T2.8 functional test files for the qwen35_llm_only recipe from the stale tests/functional_tests/models/qwen_vl/ location to tests/functional_tests/test_groups/models/qwen_vl/ (post-refactor layout) and adds a CI launch script that runs all five tests: T2.2 skip_megatron_param_globs excludes vision-tower weights (pytest) T2.5 Optimizer state: vision tower frozen, LM params trainable T2.6 LM weight parity: Megatron weights bitwise-identical to HF source T2.7 Checkpoint save-and-resume (Phase 2 starts at step 3) T2.8 TP=2 and PP=2 smoke tests with finite decreasing losses All five tests pass locally on H100. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Chen Cui <chcui@nvidia.com>
yaoyu-33
reviewed
May 6, 2026
Add init_vision_model flag (default True) to Qwen35VLModelProvider and Qwen35VLMoEModelProvider. When False, the provider passes add_encoder=False to Qwen3VLModel so the vision tower is never instantiated — vision params are not allocated on GPU and never enter the conversion task list. The qwen35_llm_*_sft_config recipes now set init_vision_model=False and no longer rely on skip_megatron_param_globs. The language model is the standard GPTModel (Qwen3VLGPTModel, a GPTModel subclass with mRoPE) wrapped under model.language_model, so existing language_model.* mappings continue to work. Verified on Qwen/Qwen3.5-0.8B (H100, 1 GPU): 0 vision params, 0 vision buffers, 230 LM trainable params, peak 17.248 GB. All five smoke tests pass (T2.2/T2.5/T2.6/T2.7 single-GPU, T2.8 TP=2 and PP=2). Signed-off-by: Chen Cui <chcui@nvidia.com>
Contributor
Author
|
/ok to test 833db9a |
Contributor
Author
|
will remove skip_megatron_param_globs |
The skip_megatron_param_globs mechanism was added to AutoBridge and
MegatronModelBridge to support LLM-only loading from Qwen3.5-VL
checkpoints, but the qwen35_llm recipe was subsequently refactored to
use add_encoder=False on the provider instead. With no production
caller — and no in-tree use case for selectively skipping arbitrary
Megatron params during HF -> Megatron load — the kwarg, the
megatron_param_matches_skip_globs helper, and its dedicated unit tests
become orphan API surface.
Reverts:
- skip_megatron_param_globs kwarg from
AutoBridge.{load_hf_weights, to_megatron_model, to_megatron_provider}
- skip_megatron_param_globs kwarg from
MegatronModelBridge.{load,stream}_weights_hf_to_megatron
- megatron_param_matches_skip_globs helper in model_bridge.py
- tests/unit_tests/models/test_megatron_param_skip_globs.py (deleted)
- the four skip_megatron_param_globs= entries in test_auto_bridge.py
mock signatures
If selective skipping is ever needed, reintroduce alongside the
production caller. Default values were None (no-op), so no public-API
break for downstream consumers.
Signed-off-by: Chen Cui <chcui@nvidia.com>
…n_model -> add_encoder Match the sibling Qwen3VLModelProvider, which already uses ``add_encoder: bool = True`` for the same concept. Two names for one knob within a single model family was needless divergence; the field forwards directly to Qwen3VLModel's ``add_encoder`` argument either way. Updated: - Qwen35VLModelProvider and Qwen35VLMoEModelProvider field rename - recipe override in qwen35_llm.py (cfg.model.add_encoder = False) - recipe docstring - unit-test fake provider mock and final assertion - functional-test docstrings (T2.2 / T2.5 / T2.6 / T2.7) - L1 launch script comment - README for examples/models/vlm/qwen35_vl/ No behavior change. Signed-off-by: Chen Cui <chcui@nvidia.com>
The prior T2.7 test only asserted that the training loop's iteration counter resumed at PHASE1_ITERS, which is not sufficient to distinguish "checkpoint loaded" from "iteration counter loaded but HF pre_wrap_hook silently re-seeded the weights". The recipe registers the HF loader as a pre-wrap hook, so a regression that flips load ordering would not be caught by the counter check alone. Add a perturb-then-restore weight-identity check: - Phase 1: at the last on_train_step_end before save (step PHASE1_ITERS), overwrite the first trainable parameter's first element with a sentinel (-1.2345e6) well outside any plausible training trajectory. The save that immediately follows captures the perturbed state. - Phase 2: at on_train_start (before any optimizer step), read the same parameter back and assert it carries the sentinel within a tight relative tolerance. If the HF pre_wrap_hook silently overrides the checkpoint loader, the observed value would be near HF init (~0) instead of the sentinel — the test fails clearly. The iteration-counter check is kept as a secondary signal. Signed-off-by: Chen Cui <chcui@nvidia.com>
The prior T2.6 test only checked two DirectMapping params (embed_tokens
weight, final_layernorm weight). For MoE recipes (35B-A3B and larger),
the higher-risk failure mode is silent breakage in the QKV or gate_up
fusion path — a row-/column-order bug there would not be caught by the
direct-mapping checks alone.
Add L2-norm parity checks for layer-0:
- ``linear_qkv.weight`` ↔ {q_proj, k_proj, v_proj}.weight (QKV fusion)
- ``linear_fc1.weight`` ↔ {gate_proj, up_proj}.weight (gate-up fusion)
L2 norm is invariant under any internal concatenation/permutation order,
so the check catches the failure modes that matter (wrong layer loaded,
scaling drift, fused param left at random init) without re-implementing
the bridge's fusion layout in test code. Tolerance 1e-3 relative —
above bf16 round-trip + summation order noise, well below the gap a
random-init or wrong-layer regression would produce.
Signed-off-by: Chen Cui <chcui@nvidia.com>
The prior test captured peak_mem_gb and printed it but never asserted on it. Without a ceiling the test is a regression detector for vision params being PRESENT in the named parameter list, but not for vision params (or their optimizer state) being silently ALLOCATED on the device — the reference 17.3 GB usage for Qwen3.5-0.8B would not catch e.g. a vision-tower-init-without-grad regression. Add a 25 GB ceiling (env-overridable via PEAK_MEM_GB_CEILING). The reference measurement was ~17.3 GB on H100; the ceiling leaves headroom for kernel/dtype drift but still trips well before the full Qwen3.5-VL footprint (~30+ GB on the same setup). Refactor the failure path to accumulate into a single ``failed`` list so multiple regressions (vision params + memory blow-up) surface together instead of stopping at the first. Signed-off-by: Chen Cui <chcui@nvidia.com>
With ``add_encoder=False`` the vision tower is never instantiated, so setting ``freeze_vision_model = True`` and ``freeze_vision_projection = True`` on the provider was misleading dead code — there is no vision module for the freeze hook to apply to. Remove the two assignments and the corresponding stale assertions in the unit test (those were asserting the fake-mock default, not recipe behavior). The meaningful guard — that the vision tower is in fact absent — is already covered by ``assert cfg.model.add_encoder is False``. Signed-off-by: Chen Cui <chcui@nvidia.com>
Contributor
Author
|
Addresses the open punch list from review:
All six commits signed-off; pre-commit (ruff + format) clean. |
cuichenx
marked this pull request as ready for review
May 11, 2026 18:48
cuichenx
added
the
ready-to-merge
Contributor
Author
|
/ok to test b9ed11f |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -0,0 +1,57 @@ | |||
| #!/usr/bin/env bash | |||
Contributor
There was a problem hiding this comment.
Nit: This file has mode 100644 (not executable), but it has a shebang and the README tells users to run it directly (./examples/models/vlm/qwen35_vl/llm_sft.sh). Consider chmod +x.
|
|
||
| from megatron.bridge import AutoBridge | ||
| from megatron.bridge.recipes.common import _sft_common | ||
| from megatron.bridge.recipes.qwen_vl.qwen35_vl import ( |
Contributor
There was a problem hiding this comment.
The to_megatron_provider(load_weights=True) call here means weights are loaded eagerly during recipe construction. This is correct for training (the provider streams HF weights at model-build time), but worth a brief docstring note since other recipes typically use load_weights=False and rely on checkpoint.load instead. Makes it clear this is intentional for the HF-direct-load path.
Contributor
|
Light Code Review. Adds 8 Qwen3.5 LLM-only SFT recipes using add_encoder=False to skip vision tower instantiation with thorough test coverage for the 800M variant. Findings: (1) PR description/diff mismatch - body describes skip_megatron_param_globs kwargs and PEFT variants not in the diff. Actual mechanism is add_encoder=False. Consider updating PR description. (2) llm_sft.sh not executable - mode 100644 but README says to run directly. (3) load_weights=True in recipe looks intentional but differs from other recipes. Test coverage is good for 800M. MoE variants lack functional tests. Suggested test cases: No perf tests impacted. |
cuichenx
removed
the
ready-to-merge
yaoyu-33
added
the
needs-review
Contributor
Author
|
superseded by #3769 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds
skip_megatron_param_globstoload_weights_hf_to_megatronandstream_weights_hf_to_megatrononMegatronModelBridge, allowing selective parameter skipping during HF→Megatron weight loading via fnmatch patterns.Uses this mechanism to ship 8 Qwen3.5 LLM-only SFT recipes (
qwen35_llm_{800m,2b,4b,9b,27b,35b_a3b,122b_a10b,397b_a17b}_sft_config) that load language-model weights from Qwen3.5-VL checkpoints while skipping the vision tower (*vision_model*). Includes LoRA/PEFT variants for the dense sizes.Changelog
MegatronModelBridge.load_weights_hf_to_megatron: addskip_megatron_param_globskwargMegatronModelBridge.stream_weights_hf_to_megatron: addskip_megatron_param_globskwargAutoBridge.to_megatron_provider: threadskip_megatron_param_globsthrough to the providermegatron_param_matches_skip_globs: new helper (fnmatch on global + local param name)src/megatron/bridge/recipes/qwen_vl/qwen35_llm.py: 8 LLM-only SFT + PEFT configstests/functional_tests/launch_scripts/h100/active/L1_Launch_models_qwen35_llm.sh: CI launch script (L1 tier, runs T2.2/T2.5–T2.8)tests/functional_tests/test_groups/models/qwen_vl/: 5 functional test scripts (see below)Tests run on H100 (Qwen/Qwen3.5-0.8B)
Tier 1 — unit tests (no GPU)
test_megatron_param_skip_globs.pyload_weightspathtest_megatron_param_skip_globs.pystream_weightspath respects skip globstest_megatron_param_skip_globs.pyTier 2 — functional tests (GPU, real HF checkpoint)
test_qwen35_llm_sft_skip_vision.pystream_weightsyields no*vision_model*tasks; LM tasks presenttest_qwen35_llm_optimizer_state.pytest_qwen35_llm_weight_parity.pyembed_tokensandfinal_layernormweights bitwise-equal to HF sourcetest_qwen35_llm_resume.pytest_qwen35_llm_tp_pp.pytest_qwen35_llm_tp_pp.py