fix: Reconcile #2315 and #2612

[tdene]

What does this PR do ?

Sets up the GPU cluster with correct topology, making sure NVLS works properly.

Issues

#2850

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]

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.

[copy-pr-bot]

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.

[tdene]

/ok to test f20856b

[terrykong]

Thanks for reconciling the Megatron generation backend with the topology-aware placement from #2612 — focused, well-scoped change.

The grpo.py placement-group logic looks correct:

• Both MegatronGeneration.init_cluster_placement_groups(..., policy_config) call sites pass the full PolicyConfig (the method needs megatron_cfg, which only exists on policy_config, not generation_config).
• No harmful double-init: colocated megatron reuses the training policy (its __init__ early-returns before its internal PG init), so the eager call is the sole init; the non-colocated path calls it twice but _init_placement_groups early-returns when PGs already exist.
• gpus_per_instance = TP*PP*CP matches the model_parallel_size formula inside the megatron method, and the vLLM/SGLang else branch is unchanged.

Two inline asks (validation coverage for the nvls switch and the new megatron+topology placement path), plus a minor process note.

Process note (non-blocking): The PR description is the empty template, and the title references #2315, which is "docs: add SECURITY.md" — likely a typo for the PR/issue that introduced the Megatron generation backend. A one-line "what/why" and the correct issue references would help reviewers.

Generated by Claude Code

[terrykong]

@tdene just checking on this PR, is this still needed?

also @shanmugamr1992 for review

[tdene]

@tdene just checking on this PR, is this still needed?

also @shanmugamr1992 for review

This is still needed, and I will update it and pass CI by 07/14

[tdene]

This is still needed, and I will update it and pass CI by 07/14

Waiting until after the next NeMo RL release (which should happen this week?)

[tdene]

/ok to test ddaab4c

[tdene]

/ok to test d06f7a7

[tdene]

/ok to test d06f7a7

[tdene]

/ok to test 6af15ba

[tdene]

/ok to test 48edb3d

[terrykong]

Re-reviewed at 48edb3d0 after your push, with a team of six agents. The new functional test is a genuine improvement — it exercises segment_size + backend: megatron in CI, and the synthetic-Ray-resource setup with anti-fallback grep guards is the right way to build it. I've replied on the June thread it addresses.

Two things to act on:

• The new megatron branch reads training parallelism where generation parallelism applies (non-colocated only). Traces for both paths inline — the colocated one is correct, the non-colocated one lays out placement groups from the pre-merge config and the corrective call is a no-op. Silent, and inert on today's numbers, but it's the path this PR exists to enable.
• The new unit test is collected by zero CI lanes — one-line fix, but as it stands it can never catch a regression.

Also one genuine question (not a finding) about whether TP*PP*CP is the right NVLink-domain span for MoE, and evidence on the still-open nvls thread.

Two things I want to flag rather than bury. An earlier draft asked you to guard a KeyError: 'sglang' in the dispatch block — that was wrong and is dropped, since gpus_per_server isn't a key in the SGLang schema at all, so the branch is unreachable. (Separately, and not yours to fix: that also makes SGLang's topology-constraint path dead code; the real field is sglang_server_config.num_gpus_per_engine.) And an earlier draft overstated the EP/ETP point as a defect — I could not build a shipped config that hits it, so it's demoted to a question.

Caveat: I could not run the test suite or the repo linters — uv.lock targets linux only, so the env doesn't resolve on my host. Every claim comes from reading source, with permalinks so you can check the reasoning rather than take my word for it.

Generated by Claude Code

[tdene]

/ok to test 90a202a

[tdene]

/ok to test 2fa6990

[tdene]

/ok to test 3afd33a

[terrykong]

/ok to test 3afd33a

[terrykong]

/ok to test 73663dd

[tdene]

/ok to test eb17aa8