Skip to content

Propagate RL runtime config updates to module configs - #6423

Merged
Phlip79 merged 1 commit into
NVIDIA:mainfrom
Phlip79:philip/propagate-rl-config-updates
Aug 11, 2026
Merged

Propagate RL runtime config updates to module configs#6423
Phlip79 merged 1 commit into
NVIDIA:mainfrom
Phlip79:philip/propagate-rl-config-updates

Conversation

@Phlip79

@Phlip79 Phlip79 commented Aug 10, 2026

Copy link
Copy Markdown
Member
  • I, the PR author, have personally reviewed every line of this PR.

What does this PR do?

Design doc (internal-only).

Removes RL runtime config updates' assumption that every module aliases the root model config.

get_logprobs and megatron_rl_inference_mode currently update only model.config. Models may contain distinct module config objects, so those root-only writes can leave module runtime state stale.

This change:

  • adds set_model_config_attribute, which updates the root config and every distinct child-module config that defines the attribute;
  • deduplicates shared configs by identity and traverses standard module wrappers;
  • uses the helper for flash_decode, cuda_graph_impl, cuda_graph_modules, and inference_cuda_graph_scope in RL utilities; and
  • keeps unrelated config-setting call sites outside this PR.

These four attributes are model-wide runtime controls: this helper intentionally assigns the same value to every applicable config. This PR does not introduce layer-specific values for them.

For models that share one config object, identity deduplication preserves the existing behavior. For models with distinct configs, the change fixes propagation of these model-wide controls.

This PR is independently mergeable into main and is intended to merge before #6313. It does not modify #6313 or depend on its hybrid layer config changes.

Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 10, 2026

Copy link
Copy Markdown

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.

@Phlip79

Phlip79 commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

/ok to test de68147

@asolergi-nv

Copy link
Copy Markdown
Contributor

/claude review

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Verified the wrapper-descent loop handles the real production shapes (DDP/Float16Module/GPTModel roots are already nn.Module, so modules() covers the whole tree), the identity dedup preserves existing behavior for models sharing one config, and the restore path keeps the prior args.* semantics. Test coverage looks right: dedup/idempotency and skip-on-missing-attribute in test_utils.py, plus shared/distinct config parametrization across dense/MoE for megatron_rl_inference_mode and a get_logprobs test that checks the value both during forward and after restore.

Non-blocking note: the helper assigns the same list object to every config for cuda_graph_modules. I checked for in-place mutation of that attribute and the only site is TransformerConfig.__post_init__ (append), which runs only at construction, so no aliasing is observable today — and the docstring already documents the shared-value intent. Just worth keeping in mind if a runtime in-place mutation of that list is ever added.

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the Final Review PR is in the "final review" stage label Aug 11, 2026
@Phlip79
Phlip79 enabled auto-merge August 11, 2026 20:25
@svcnvidia-nemo-ci svcnvidia-nemo-ci added Approved All necessary approvals have been made and removed Final Review PR is in the "final review" stage labels Aug 11, 2026
@Phlip79
Phlip79 added this pull request to the merge queue Aug 11, 2026
@svcnvidia-nemo-ci

Copy link
Copy Markdown
Contributor

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/31533473528

Merged via the queue into NVIDIA:main with commit da18ed6 Aug 11, 2026
99 of 101 checks passed
@Phlip79
Phlip79 deleted the philip/propagate-rl-config-updates branch August 11, 2026 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Approved All necessary approvals have been made complexity: low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants