Repository navigation
Conversation
get_model_for_inference() and tools/run_text_generation_server.py pass modelopt_gpt_hybrid_builder straight to get_model(). The builder takes args as its first positional parameter, but get_model() calls its provider with keyword arguments only (pre_process, post_process, config, pg_collection), so with ModelOpt enabled both entry points raise "missing 1 required positional argument: 'args'" before any model is built. The regression came in with NVIDIA#5169, which replaced partial(model_provider, builder) by the bare builder. Bind args with functools.partial at both call sites, which is what model_provider() did for this builder before. Add CPU tests that drive both entry points with a stand-in builder of the same signature and a stand-in get_model that calls the provider the way get_model() does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yan Xu <yxu1@nvidia.com>
The comments above the ModelOpt get_model() calls in get_model_for_inference() and tools/run_text_generation_server.py say that get_model() runs the ModelOpt-checkpoint auto-detection. It does nothing there: both entry points take this branch only when args.modelopt_enabled is already set, and maybe_enable_modelopt() returns at once in that case. Remove the claim and fold the note about binding args into the remaining comment. Since NVIDIA#5169 moved the builder choice ahead of get_model(), these entry points also no longer pick the ModelOpt builder for a ModelOpt checkpoint served without --modelopt-enabled. Restoring that is a separate behaviour change and is left to its own fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yan Xu <yxu1@nvidia.com>
On current main (NVIDIA#8391), get_model_for_inference() reads get_run_config() before it picks a builder, so the first test fails with "run config is not initialized" before it reaches the ModelOpt branch. Use the run_config fixture, which sets a run config for the test and exists on this base as well. Both tests also asserted that the builder receives pg_collection=None. Once the text generation server passes the registered process groups to get_model(), that holds only while nothing is registered. Record the groups the get_model() stand-in receives and check that the builder gets exactly those. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yan Xu <yxu1@nvidia.com>
The get_model() stand-in passed pg_collection=None straight through, while the real get_model() swaps None for the global groups first. On main, a wrapper that dropped pg_collection would therefore still pass. The stand-in now swaps None for a distinct object, so the builder must receive exactly the groups get_model() built with. The tests compared the bound args by value, which a copy of the Namespace also satisfies; they now also check identity. The stand-in builder copies the real builder's signature by hand, so add a test that compares the two where ModelOpt is installed and the real module imports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yan Xu <yxu1@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. |
This was referenced Oct 30, 2026
Connor-XY
added a commit
to Connor-XY/Megatron-LM
that referenced
this pull request
Nov 1, 2026
The inference entry points that this branch makes pass the registered collection to get_model() also call the ModelOpt builder, which needs args bound because get_model() calls the provider with keyword arguments only. Merge that change so both land together. In megatron/inference/utils.py and tools/run_text_generation_server.py the ModelOpt path keeps the merged change's comment and calls get_model() with partial(modelopt_gpt_hybrid_builder, args), wrap_with_ddp=False and the registered pg_collection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yan Xu <yxu1@nvidia.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yan Xu <yxu1@nvidia.com>
This was referenced Nov 3, 2026
Contributor
Author
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.
Why. With
--modelopt-enabled,get_model_for_inference()inmegatron/inference/utils.pyandtools/run_text_generation_server.pypassmodelopt_gpt_hybrid_builderstraight toget_model(). The builder takesargsas its first positional parameter, butget_model()calls its provider with keyword arguments only. Every ModelOpt run through these entry points fails withTypeError: modelopt_gpt_hybrid_builder() missing 1 required positional argument: 'args'. That includestools/run_dynamic_text_generation_server.py, which reachesget_model_for_inference()throughget_dynamic_inference_engine(). The regression came with #5169, which replacedpartial(model_provider, builder)with the bare builder.What.
argswithfunctools.partialat both call sites. That is whatmodel_provider()did for this builder, and whatexamples/post_training/modeloptstill do.get_model()runs the ModelOpt checkpoint auto-detection. Both entry points take this branch only when--modelopt-enabledis already set, andmaybe_enable_modelopt()returns at once in that case.Tests. New
tests/unit_tests/post_training/test_modelopt_inference_builder.pydrives both entry points with a stand-in builder that has the real builder's signature and a stand-inget_model()that, like the real one, calls the provider with keyword arguments only and replaces aNonecollection with its own groups. The builder must receive the sameargsobject and exactly the groupsget_model()built with. A third test compares the stand-in's signature with the real builder where ModelOpt is installed. In a CPU run before the 2026-11-01 sync, the two binding tests fail on main with theTypeErrorabove and pass here. The rest oftests/unit_tests/post_traininggives the same results on both. The signature test skipped in that CPU run because ModelOpt is not installed there. In the integration run on 4 GPUs (main 77391e7 with all #6307 PRs merged), all 3 tests in the new file pass and none skip; main has no such file. In the integration run, this PR's test files show no regressions against main. No end-to-end run with--modelopt-enabledwas done.On main after #8391, the first commit's test fails without the third commit, so please squash on merge.
Compatibility. No API change and no new warnings. Not fixed here: since #5169 these entry points no longer pick the ModelOpt builder on their own for a ModelOpt checkpoint served without
--modelopt-enabled. That behavior change needs its own PR.Synced with main 77391e7 on 2026-11-01 by a merge commit with no conflicts, so existing commits and review links stay valid.
Part of #6307.
🤖 Generated with Claude Code