Forward rotary_base and rope_scaling_factor to LLaVAModel in the VLM builders - #6586
Merged
Merged
Conversation
Both VLM call sites pass language_rope_scaling but never language_rope_scaling_factor, so LLaVAModel falls back to its default of 8.0 and --rope-scaling-factor is silently ignored when training a VLM. LLaVAModel already accepts the parameter and forwards it to the language model, so only the call sites need updating. Reported for the GPT path in NVIDIA#6305, which notes that the VLM call sites are a separate surface not covered by its linked PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Aditya Ghai <adityaghailbdrp1@gmail.com>
pretrain_vlm.py never passes language_rotary_base, so LLaVAModel falls back to its default of 10000 and --rotary-base is silently ignored. The sibling call site in examples/multimodal/model.py already passes it. Unlike RoPE scaling, rotary_base is not opt-in: it is used by every RoPE model on this path, and it must match the pretrained checkpoint (Llama 3.x uses 500000). Fixes NVIDIA#6585 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Aditya Ghai <adityaghailbdrp1@gmail.com>
adityaghai07
marked this pull request as ready for review
August 17, 2026 17:37
adityaghai07
marked this pull request as draft
August 18, 2026 07:32
adityaghai07
marked this pull request as ready for review
August 18, 2026 07:32
maanug-nv
approved these changes
Aug 18, 2026
Contributor
|
/ok to test 187a759 |
Phlip79
approved these changes
Aug 18, 2026
Phlip79
enabled auto-merge
August 18, 2026 22:20
cuichenx
approved these changes
Aug 20, 2026
svcnvidia-nemo-ci
added
the
waiting-on-maintainers
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/32497337271 |
svcnvidia-nemo-ci
removed
the
waiting-on-maintainers
devnkong
pushed a commit
to devnkong/Megatron-LM
that referenced
this pull request
Aug 22, 2026
…builders (NVIDIA#6586) Signed-off-by: Aditya Ghai <adityaghailbdrp1@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Kezhi Kong <kezhik@kezhik-mlt.client.nvidia.com>
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
6 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
Two RoPE arguments are parsed but never forwarded to
LLaVAModel, so they are silently ignored on the VLM path.LLaVAModelalready accepts both and forwards them to the language model — only the call sites needed updating.pretrain_vlm.pyexamples/multimodal/model.pylanguage_rotary_baselanguage_rope_scaling_factorrope_scaling_factor(commit 1) —--rope-scaling-factornever reaches the model, which keepsLLaVAModel's default of8.0. This is the VLM half of #6305, which states thatpretrain_vlm.pyandexamples/multimodal/model.py"have the same issue on the LLaVA path... a separate call surface and not covered by the linked PR". Complements #6306, which covers the GPT/post-training paths.rotary_base(commit 2, #6585) —--rotary-basenever reaches the model, which keeps the default of10000. Unlike RoPE scaling this is not opt-in: it applies to every RoPE model on this path and must match the pretrained checkpoint (Llama 3.x uses500000). The sibling call site inexamples/multimodal/model.py:218already passes it, which is what makes this an oversight rather than an intentional difference.Both values are forwarded into the language model (
llava_model.py:254-256→GPTModel→RotaryEmbedding); the vision encoder never sees them. There is no fallback via the config — baseTransformerConfighas norotary_basefield (the only one intransformer_config.pybelongs toMLATransformerConfig, andgpt_model.py:166excludes multi-latent attention from this branch), so the keyword argument is the only path.History
Both appear to be plumbing that was added to the model but never wired at the call site:
617dc63c0("Make rotary base configurable in LlavaModel", 2024-06-27) addedlanguage_rotary_basetollava_model.pyonly —+2lines in one file, no call site updated.63be779b4later updatedexamples/multimodal/model.py;pretrain_vlm.pynever was.8c98d2def("llama3.2 support", 2025-01-31) addedlanguage_rope_scaling_factortollava_model.pyand updated zero call sites.Testing
These call sites are not currently unit tested — no test in the repo imports
pretrain_vlm.model_provider, and exercising it requires an initializedtorch.distributedprocess group (pretrain_vlm.py:89) plus a fully-populated args namespace, so a mock-based test would be fragile. The change is a three-line argument forwarding, verified by inspection against the sibling call site and against the equivalent GPT-path fix in #6306. Happy to add a test if maintainers would like one, and glad to follow whatever approach #6306 settles on.🤖 Generated with Claude Code