Pass the config container to pretrain in the multimodal example - #6857
Open
huthvincent wants to merge 1 commit into
Open
Pass the config container to pretrain in the multimodal example#6857huthvincent wants to merge 1 commit into
huthvincent wants to merge 1 commit into
Conversation
Contributor
Author
|
@NVIDIA/mcore-oncall — same ask as on #6859: Ready, but Two added lines in |
huthvincent
force-pushed
the
fix/B003-multimodal-train-cfg-container
branch
from
August 27, 2026 14:45
fffd9d9 to
863a3df
Compare
`examples/multimodal/train.py` cannot start. Its `__main__` block calls `pretrain_cfg_container_from_args(args)` without importing the symbol, so the line raises `NameError`; and the `full_config` it builds is never passed, so every positional argument to `pretrain()` binds one slot early — `cfg_container` receives the dataloader provider, `train_valid_test_dataset_provider` receives `ModelType.encoder_or_decoder`, and so on. `pretrain` dereferences its first argument as `cfg_container.logger.log_progress`, so there is no legacy order to fall back on. Both halves arrived with the ModelBuilder migration in NVIDIA#5516, which applied them correctly to `pretrain_vlm.py`, `examples/mimo/train.py` and `examples/bert/pretrain_bert.py`. This brings the multimodal entry point in line with those three; a reviewer raised the same point on NVIDIA#5516 (NVIDIA#5516 (comment)) and it was marked resolved, but only the `args_defaults`/`extra_args_provider` half landed. `examples/multimodal/pretrain_mistral_clip.sh`, documented in `examples/multimodal/README.md`, runs this file under torchrun, so the documented LLaVA pretraining example fails immediately after argument parsing on any config. Signed-off-by: Rui Zhu <rui.zhu.rz399@yale.edu>
huthvincent
force-pushed
the
fix/B003-multimodal-train-cfg-container
branch
from
August 31, 2026 00:46
863a3df to
de693b7
Compare
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
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?
Makes
examples/multimodal/train.pystart again. It currently cannot.Two halves of one incomplete migration, both in the
__main__block:pretrain_cfg_container_from_args(args), but the module's only import from that package isfrom megatron.training.arguments import parse_and_validate_args. The symbol lives inmegatron.training.argument_utils, which this file never imports, and there is no star-import — so the line raisesNameError.full_configit builds is never passed.pretrain()now begins(cfg_container, train_valid_test_dataset_provider, model_type, forward_step_func, model_provider=None, ...), and the call here begins with the dataloader provider, so every positional argument binds one slot early:cfg_containergets the dataloader provider,train_valid_test_dataset_providergetsModelType.encoder_or_decoder,model_typegetsforward_step.pretraindereferences its first argument immediately ascfg_container.logger.log_progress, so there is no legacy order to fall back on.The ModelBuilder migration in #5516 applied both halves correctly to
pretrain_vlm.py,examples/mimo/train.pyandexamples/bert/pretrain_bert.py; this brings the multimodal entry point in line with those three.pretrain_vlm.py:500-502is the exact shape used here.A reviewer raised this on #5516 — #5516 (comment) — and the thread was closed as "resolved in af7f1b6". That sha is not in the repository (the PR was squash-merged), and the merged result contains only the
args_defaults/extra_args_providerhalf of the fix; the import and the first positional argument are still missing onmainand ondev.Issue tracking
For PRs from open-source community contributors:
Linked issue: none — 2-line bug fix. Related to #5516 (comment linked above).
Contribution process
Pre-checks
tests/importsexamples/multimodal/train.py, which is why the migration could land without breaking anything visible. Happy to add a smoke test if you want one.megatron/coreandtests/), and the new import is placed whereisortwould put it (argument_utilssorts beforearguments).How to reproduce, and how we checked it
examples/multimodal/pretrain_mistral_clip.sh:133runstorchrun --nproc_per_node 8 examples/multimodal/train.py ${OPTIONS}, andexamples/multimodal/README.md:89documents running that script. The failure is at module__main__, before any GPU work, so no GPU or data is needed to see it.Checked on 8xL4 / torch 2.12.0+cu130: an AST pass collected every name bound at module scope (no star-imports present), confirmed
pretrain_cfg_container_from_argsis not among them, readinspect.signature(pretrain)as beginning['cfg_container', 'train_valid_test_dataset_provider', 'model_type', 'forward_step_func', 'model_provider'], and executed the file's real__main__block with every bound name stubbed — which raisesNameError: name 'pretrain_cfg_container_from_args' is not defined.What we did not do: we did not run the LLaVA example end to end, so this is verified to remove the
NameErrorand the argument shift, not to demonstrate that multimodal pretraining converges after the ModelBuilder migration. Nothing was benchmarked and no performance claim is made.The strongest objection to this change
This is an
examples/script that no CI job executes, and it has been broken identically ondevfor hundreds of commits — so the two-line patch fixes the entry point without showing that the documented example works after it. Ifexamples/multimodal/train.pyis superseded bypretrain_vlm.py, the right change is deleting the file or the README pointer rather than fixing it, and we would rather be told that than guess.