examples/multimodal_dev: docs + Victarry review fixes + vision patch merger parity test - #3
Conversation
Reviewer's GuideAdds HF→Megatron-FSDP DTensor checkpoint conversion docs for multimodal_dev, tightens configuration safety/semantics for Qwen3.5-VL scripts and model (PP/EP/MoE, dataset registry, GELU parity, MRoPE perf note), and introduces a numerical parity test that validates Qwen35VLPatchMerger against the HuggingFace reference implementation. Sequence diagram for Qwen35VLPatchMerger HF parity testsequenceDiagram
participant PyTest as PyTest
participant HFMerger as Qwen3VLVisionPatchMerger
participant LocalMerger as Qwen35VLPatchMerger
PyTest->>HFMerger: load_state_dict(hf_state_dict)
PyTest->>LocalMerger: load_state_dict(hf_state_dict)
PyTest->>HFMerger: forward(hidden_states)
HFMerger-->>PyTest: hf_logits
PyTest->>LocalMerger: forward(hidden_states)
LocalMerger-->>PyTest: local_logits
PyTest->>PyTest: assert torch.allclose(hf_logits, local_logits, atol)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the checkpoint conversion README snippet, the Git instructions are pinned to a personal fork/feature branch (
wplf feat/qwen35-vl-example); consider referencing an upstream branch or specific commit hash (and/or calling out that this is temporary) so the instructions remain valid as your personal branches evolve. - The parity test script mutates
sys.pathto inject the repo root; consider relying on package-relative imports (or running viapython -m ...) instead of manualsys.pathsurgery to make the test more robust to different execution environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the checkpoint conversion README snippet, the Git instructions are pinned to a personal fork/feature branch (`wplf feat/qwen35-vl-example`); consider referencing an upstream branch or specific commit hash (and/or calling out that this is temporary) so the instructions remain valid as your personal branches evolve.
- The parity test script mutates `sys.path` to inject the repo root; consider relying on package-relative imports (or running via `python -m ...`) instead of manual `sys.path` surgery to make the test more robust to different execution environments.
## Individual Comments
### Comment 1
<location path="examples/multimodal_dev/tests/test_vision_patch_merger_parity.py" line_range="57-60" />
<code_context>
+HIDDEN_SIZE = 1152
+OUT_HIDDEN_SIZE = 3584
+SPATIAL_MERGE_SIZE = 2
+NUM_PATCHES = 64 # must be divisible by spatial_merge_size ** 2
+
+ATOL_FP32 = 1e-4
</code_context>
<issue_to_address>
**suggestion (testing):** Turn the implicit divisibility requirement into an assertion to guard future changes.
The test currently only documents that `NUM_PATCHES` must be divisible by `SPATIAL_MERGE_SIZE ** 2` but doesn’t enforce it. Please add an assertion like `assert NUM_PATCHES % (SPATIAL_MERGE_SIZE ** 2) == 0` so future constant changes that violate this invariant fail clearly rather than causing confusing layout/parity issues.
```suggestion
SPATIAL_MERGE_SIZE = 2
NUM_PATCHES = 64 # must be divisible by spatial_merge_size ** 2
assert NUM_PATCHES % (SPATIAL_MERGE_SIZE**2) == 0
ATOL_FP32 = 1e-4
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| SPATIAL_MERGE_SIZE = 2 | ||
| NUM_PATCHES = 64 # must be divisible by spatial_merge_size ** 2 | ||
|
|
||
| ATOL_FP32 = 1e-4 |
There was a problem hiding this comment.
suggestion (testing): Turn the implicit divisibility requirement into an assertion to guard future changes.
The test currently only documents that NUM_PATCHES must be divisible by SPATIAL_MERGE_SIZE ** 2 but doesn’t enforce it. Please add an assertion like assert NUM_PATCHES % (SPATIAL_MERGE_SIZE ** 2) == 0 so future constant changes that violate this invariant fail clearly rather than causing confusing layout/parity issues.
| SPATIAL_MERGE_SIZE = 2 | |
| NUM_PATCHES = 64 # must be divisible by spatial_merge_size ** 2 | |
| ATOL_FP32 = 1e-4 | |
| SPATIAL_MERGE_SIZE = 2 | |
| NUM_PATCHES = 64 # must be divisible by spatial_merge_size ** 2 | |
| assert NUM_PATCHES % (SPATIAL_MERGE_SIZE**2) == 0 | |
| ATOL_FP32 = 1e-4 |
Document the HF -> Megatron-FSDP DTensor conversion path needed before pretraining from pretrained weights: setup (clone Bridge, pin its 3rdparty/Megatron-LM submodule to this branch), the `torchrun convert_checkpoints_fsdp.py import` command with EP=8 default topology, expected output layout, and the open Bridge dependency (NVIDIA-NeMo/Megatron-Bridge#3987) to skip the post-save tokenizer build that otherwise crashes on this branch.
Resolves the inline comments from @Victarry's PR review on NVIDIA#4751. * vision_encoder.py — patch merger GELU was `approximate='tanh'` while the in-code NOTE acknowledged HF uses `approximate='none'`. Switched to `approximate='none'` to match the official Qwen3VLVisionPatchMerger numerics for HF -> Megatron checkpoint parity. * pretrain_multimodal.py — added an explicit guard against `--pipeline-model-parallel-size > 1`. The model_provider builds the full model on every rank and ignores pre_process / post_process stage flags, so PP>1 would silently break Megatron's pipeline-parallel contract. Fail fast instead. * scripts/run_qwen35_vl.sh — three fixes: 1. `EP` now defaults to 1 (was 2). MoE variants must opt in via the environment override. 2. After the variant case block, fail fast if `NUM_EXPERTS=0 && EP>1` so a dense run such as `MODEL_VARIANT=9b ./run_qwen35_vl.sh` no longer trips Megatron's arg validation downstream. 3. `--moe-router-force-load-balancing` was unconditionally added to GPT_MODEL_ARGS (and therefore enabled even when no MoE args were emitted). It is now gated behind `FORCE_LOAD_BALANCING=1`, defaults off, and is appended to MOE_ARGS only when MoE is active. Real finetuning runs no longer freeze router routing decisions by default. * data/{vlm_dataset.py -> cord_v2.py} + models/__init__.py — renamed the CORD-V2-specific module from the generic-sounding `vlm_dataset.py` to `cord_v2.py`, updated the model registry path string accordingly, and added an "Adding another VLM dataset" section to the module docstring documenting the per-dataset module + `MODEL_REGISTRY["..."]["dataset_providers"]` registration pattern. * models/qwen35_vl/mrope.py — added a performance note on the `_build_sample_mrope_positions` helper documenting the `.tolist()` / `.item()` GPU<->CPU sync points and CUDA-graph incompatibility, and the precompute-in-collate / cache-by-shape follow-up plan. Behavior preserved here pending a follow-up data pipeline change. The other tests-import comment (test_thd_*.py importing `_pack_batch`) is already addressed on this branch: the helper is now named `pack_or_pad_batch` and the tests import that symbol.
New test ``tests/test_vision_patch_merger_parity.py`` verifies the Megatron patch merger against an inlined verbatim copy of HuggingFace ``Qwen3VLVisionPatchMerger`` (``use_postshuffle_norm=False`` branch from ``transformers/src/transformers/models/qwen3_vl/modeling_qwen3_vl.py``). The HF reference is inlined so the test has no runtime dependency on the ``transformers`` package. The test copies HF state-dict tensors into the Megatron module (TP=1, 1:1 mapping), runs both on the same random input, and asserts ``torch.testing.assert_close`` on the logits in fp32 and bf16: [torch.float32] shape=(16, 3584) max_abs_diff=2.551e-05 (atol=1e-4) [torch.bfloat16] shape=(16, 3584) max_abs_diff=3.906e-03 (atol=5e-2) The fp32 residual is structural (TE LayerNorm vs nn.LayerNorm use different fused reduction orders) and the bf16 figure is at the arithmetic floor for a two-layer MLP. This pins the GELU ``approximate='none'`` fix (commit 8aace7b) against future regressions. Run with:: torchrun --nproc_per_node=1 \\ examples/multimodal_dev/tests/test_vision_patch_merger_parity.py
2130f42 to
59923c4
Compare
Cherry-picked from
feat/qwen35-vl-exampleontojinliangl/qwen35-vl-hybridep-deploy-pr4715so the review diff is scopedto just three commits of
examples/multimodal_dev/work:075fe8935docs(examples/multimodal_dev): add checkpoint conversion guide to README— new## Checkpoint Conversion (HF → Megatron-FSDP DTensor)section (Setup / Convert / Output / Bridge dependency)4e853ea47fix(examples/multimodal_dev): address Victarry review feedback— resolves the 6 inline review comments on upstream PR NVIDIA#4751: PatchMerger GELUtanh→none(HF parity), PP>1 fail-fast inpretrain_multimodal.py, EP default2 → 1+NUM_EXPERTS=0 && EP>1fail-fast inrun_qwen35_vl.sh,--moe-router-force-load-balancinggated behindFORCE_LOAD_BALANCING=1,vlm_dataset.py → cord_v2.pyrename + registry path update + extensibility docstring, MRoPE precompute TODO note2130f4210test(examples/multimodal_dev): add Qwen35VLPatchMerger HF parity test— newtests/test_vision_patch_merger_parity.py. Inlines HFQwen3VLVisionPatchMerger, copies state-dict 1:1 (TP=1), asserts logits parity. Runs on H100: fp32 max-abs2.55e-5(atol=1e-4), bf16 max-abs3.91e-3(atol=5e-2); test passesTotal: 9 files / +325 -10 (incl. 1 rename).
Verified on cw: parity test job 12224061 COMPLETED 0:0 in 43s.
Summary by Sourcery
Document checkpoint conversion from HuggingFace to Megatron-FSDP DTensor, tighten multimodal training/launch configuration safety, and add a numerical parity test for the Qwen35VLPatchMerger vision module against the HuggingFace reference.
Enhancements:
Documentation:
Tests:
Chores: