Skip to content

fix(megatron): fix Megatron inference settings - #4775

Merged
yuki-97 merged 8 commits into
NVIDIA-NeMo:mainfrom
tdene:tde/nanov3_mxfp8_flashinfer_backend
Oct 22, 2026
Merged

yuki-97 merged 8 commits into
NVIDIA-NeMo:mainfrom
tdene:tde/nanov3_mxfp8_flashinfer_backend

Conversation

@tdene

@tdene tdene commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

What does this PR do ?

Updates default Megatron Inference settings.

Effects:

  • grpo-nanov3-30BA3B-4n4g-megatron_generation-noncolocated-mxfp8-rollouts has total_step_time drop from 23.6 to 17.5, while generation time drops from 15.6 to 9.3.

  • grpo-nanov3-30BA3B-4n4g-megatron_generation-noncolocated-mxfp8-rollouts-packed-refit has total_step_time drop from 24.5 to 18.3, while generation time drops from 15.6 to 9.2.

  • grpo-qwen2.5-math-1.5b-instruct-1n8g-megatron_generation-single-controller-async-lag4 drops exposed generation time from 2.45 to 1.68, and various other megatron_generation tests see similar improvements on Blackwell.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

@copy-pr-bot

copy-pr-bot Bot commented Oct 18, 2026

Copy link
Copy Markdown

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the Documentation Improvements or additions to documentation label Oct 18, 2026
@tdene tdene added the CI:L0 Run doctests and unit tests label Oct 18, 2026
cspades
cspades previously approved these changes Oct 18, 2026
@tdene
tdene changed the base branch from tde/nanov3_mxfp8_flashinfer_backend to main October 18, 2026 20:26
@tdene
tdene dismissed cspades’s stale review October 18, 2026 20:26

The base branch was changed.

Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
@tdene
tdene force-pushed the tde/nanov3_mxfp8_flashinfer_backend branch from 7713cd9 to a50dcd6 Compare October 18, 2026 22:32

@yuki-97 yuki-97 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.

Thanks for retuning the Megatron inference defaults and for retiring the stale buffer_guaranteed_fraction / unified_memory_level keys behind a startup error that names the replacement. The PR switches num_cuda_graphs to autotune, enables prefix caching by default, moves the nanov3 MXFP8 recipes to FlashInfer, and derives the fp32 router for inference-optimized MoE.

Great to see the before/after step and generation times for the two nanov3 MXFP8 recipes and the lag4 Qwen recipe posted in the description; the effect of the new defaults can be read off directly.

Scope: default/recipe/config changes, the new token-capacity check and the removed-key rejection, at a50dcd6

Checked:

  • MXFP8 grouped-GEMM validation removal: MCore's resolve_mxfp8_backend returns the same storage for torch, flashinfer and vllm, so dropping the NeMo-RL-side guard (and its omitted-key special case) is consistent with upstream.
  • Prefix caching default: MCore's DynamicInferenceEngine.resume() bumps _weight_epoch and re-salts the prefix cache, so enabling it does not reuse KV computed under pre-refit weights.
  • Removed-key rejection in outdated_config_checks.py:141: generalizing the single offload check into one table keeps the error text per key, and no YAML, doc or test in the repo still carries buffer_guaranteed_fraction or unified_memory_level, so no in-tree config can trip it.
  • Token-capacity check in megatron_worker.py:707: the decode-row bound matches the formula MCore's text generation controller uses to engage the bounded MXFP8 path, so the new error fires exactly when the capacity could never apply.

Remaining comments are on the router dtype in setup.py:1395, which is overridden to fp32 silently and should raise instead when it is not fp32.

Comment thread nemo_rl/models/megatron/setup.py Outdated
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
yuki-97
yuki-97 previously approved these changes Oct 20, 2026

@yuki-97 yuki-97 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.

changes LGTM, will need to wait #4802 get into main first, otherwise CI will fail.

@yuki-97
yuki-97 enabled auto-merge (squash) October 20, 2026 03:24
@yuki-97

yuki-97 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

/ok to test a511e2b

@tdene tdene added the cherry-pick-to-main Opt in: cherry-pick this PR's commit from the release branch back into main label Oct 20, 2026
@tdene
tdene changed the base branch from main to r0.8.0 October 20, 2026 13:10
@tdene
tdene dismissed yuki-97’s stale review October 20, 2026 13:10

The base branch was changed.

Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Oct 20, 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.

@github-actions github-actions Bot added the CI Relating to CI label Oct 20, 2026
@tdene tdene removed the cherry-pick-to-main Opt in: cherry-pick this PR's commit from the release branch back into main label Oct 20, 2026
@tdene
tdene changed the base branch from r0.8.0 to main October 20, 2026 13:20
@github-actions github-actions Bot removed the CI Relating to CI label Oct 20, 2026
@tdene

tdene commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test c3bd883

@yuki-97 yuki-97 added CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) and removed CI:L0 Run doctests and unit tests labels Oct 20, 2026
yuki-97
yuki-97 previously approved these changes Oct 20, 2026

@yuki-97 yuki-97 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.

changed to run with CI:Lfast, so that we can cover e2e runs.

tdene and others added 2 commits October 21, 2026 10:19
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
…e/RL

Brings in the GitHub-side merge of main (c3bd883).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
@tdene

tdene commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test dccc2a7

@yuki-97
yuki-97 merged commit 30e7db2 into NVIDIA-NeMo:main Oct 22, 2026
93 of 94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) Documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants