Skip to content

fix(megatron): bind gradient callbacks to runtime model config - #4116

Open
DanialTaheri wants to merge 1 commit into
super-v3.5-posttrainingfrom
smohsenitahe/fix-runtime-gradient-callbacks-super35
Open

DanialTaheri wants to merge 1 commit into
super-v3.5-posttrainingfrom
smohsenitahe/fix-runtime-gradient-callbacks-super35

Conversation

@DanialTaheri

Copy link
Copy Markdown
Contributor

What does this PR do?

Bind NeMo-RL's Megatron runtime callbacks to the constructed model's configuration, not the original provider configuration.

finalize_megatron_setup() invokes Bridge's _update_model_config_funcs() directly. Providers such as Nemotron Omni can copy their configuration during model construction, so installing callbacks on megatron_cfg.model leaves the runtime configuration read by the MCore scheduler without them. Use get_model_config(model) to unwrap the model and bind callbacks to the configuration actually used for training.

The regression tests cover shared and copied configurations, nested model wrappers, gradient/parameter overlap enabled and disabled, and training versus forward-only execution. They check gradient-finalizer dispatch once after three training microbatches and no dispatch during forward-only execution.

Issues

Related: NVIDIA-NeMo/Megatron-Bridge#6067

Bridge's PR fixes its own setup call site; NeMo-RL has a separate call site and needs this companion change. The callback installer itself is unchanged, so this NeMo-RL change works with the target branch's existing Bridge pin. No dependency bump is included.

Usage

No configuration changes are required. The corrected callback binding is applied during Megatron policy setup.

Before your PR is "Ready for review"

  • Read and followed the contributor guidelines; commit is signed off.
  • Added regression coverage to TestFinalizeMegatronSetup.
  • Full native unit and functional suites passed locally: not run; this host lacks the GPU/Megatron runtime. Requesting CI verification.
  • No documentation change is needed for this internal bug fix.

Additional Information

  • A CPU source-level harness passed 10 checks against the target branch's unchanged Bridge and MCore pins: the existing setup test, eight parameterized callback/scheduler cases, and an old-binding negative control.
  • The harness executes the production setup, installer, unwrapping and scheduler functions with mocked DDP and forward/backward math. This validates callback wiring, not numerical distributed gradient equality or training convergence.
  • This ports only the NeMo-RL code and tests from the local combined fix. It excludes the local-only Bridge backport gitlink, image preprocessing, packed-MTP changes, experiment launchers and reward/generation settings.

Use get_model_config(model) when installing runtime callbacks from NeMo-RL's own setup path. Providers such as Nemotron Omni may copy their configuration during construction, so the MCore scheduler must receive callbacks on the constructed model's configuration.

Cover copied/shared configs, nested wrappers, overlap on/off, and training versus forward-only finalizer dispatch. Port the NeMo-RL changes from df6b2d7d3102048d85ac6afead41dd4813352e59 without the local Bridge backport gitlink or other experiment changes. The existing Bridge callback installer supports this fix unchanged.

Signed-off-by: smohsenitahe <smohsenitahe@nvidia.com>
@DanialTaheri
DanialTaheri requested review from a team as code owners September 12, 2026 15:36
@copy-pr-bot

copy-pr-bot Bot commented Sep 12, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@DanialTaheri DanialTaheri added the CI:L0 Run doctests and unit tests label Sep 12, 2026
@DanialTaheri

Copy link
Copy Markdown
Contributor Author

/ok to test ebfb72e

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L0 Run doctests and unit tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant