Conversation
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
|
/ok to test 3535f01 |
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
|
/ok to test 440d4b7 |
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
|
/ok to test 115ed78 |
Signed-off-by: larkzhang-nv <larkz@nvidia.com>
|
/ok to test 4637622 |
jQizhang
left a comment
There was a problem hiding this comment.
Team review: PR #4143 — MiniMax-M3 GRPO training support
Reviewed by 6 agents (rl-expert, test-agent, bug-finder, design-reviewer, comment-reviewer, devil's-advocate). 2 non-blocking suggestions, 2 informational/minor items below.
No existing PR comments needed a response. Linters (pre-commit run --all-files) pass clean.
Generated by Claude Code
| if isinstance(value, str) and value.startswith("torch."): | ||
| optimizer_kwargs[key] = getattr(torch, value.removeprefix("torch.")) | ||
| if not _has_optimizer_fp32_master(optimizer_cls, optimizer_kwargs): | ||
| load_dtype = torch.float32 |
There was a problem hiding this comment.
nemo_rl/models/automodel/setup.py:755-767
1 action item.
TL;DR — this silently changes stored param dtype (fp32→bf16) for 5+ pre-existing production recipes, not just the new MiniMax recipe, with no nightly evidence it's safe.
PR-introduced, not a numerics-correctness bug. Model weight storage dtype used to be unconditionally torch.float32; now it's runtime_config.dtype (e.g. bf16) whenever _has_optimizer_fp32_master is true — true for grpo-qwen3.5-35ba3b-2n8g-automodel-ep16.yaml, grpo-nemotron3-super-120BA12B-16n8g-automodel-ep8.v2.yaml, grpo-glm47-flash-4n8g-automodel.yaml, vlm_grpo-gemma4-e4b-geo3k-1n8g-automodel.yaml, and dapo-nanov3.5-30BA3B-4n8g-automodel.yaml.
Compute precision itself is unaffected: FSDP2's MixedPrecisionPolicy(param_dtype=dtype, ...) in setup_distributed already casts the all-gathered shard to bf16 for forward/backward regardless of resident storage dtype, so this only changes stored-param dtype / memory footprint (previously fp32 storage made TE FusedAdam's separate fp32 master copy redundant; now it isn't). Undisclosed in the PR description; no nightly evidence for the affected pre-existing recipes.
AI-1
Action: confirm via nightly (a) at least one affected pre-existing recipe (e.g. grpo-qwen3.5-35ba3b-2n8g-automodel-ep16) still converges post-change, and (b) DCP checkpoint save/resume compatibility across the storage-dtype change (resuming an old fp32-saved checkpoint under the new bf16-storage code, or vice versa).
| backend and BF16 vLLM generation. | ||
|
|
||
| > [!IMPORTANT] | ||
| > **Status: Functionally Ready.** The reference recipe has been validated with |
There was a problem hiding this comment.
docs/guides/models/minimax/minimax-m3.md:7
1 action item.
PR-introduced. The "Status: Functionally Ready" label rests on a single 100-step run (~0.72 val accuracy), while this same doc's Known Limitations section says long-run convergence is unvalidated — and there's no nightly driver to re-validate it going forward (see the test-suite registration comment on the recipe YAML).
AI-1
Action: either downgrade the status label (e.g. "Experimental"/"Preview") until a nightly driver exists, or add the nightly registration — don't ship "Functionally Ready" without an automated re-validation path.
| _patch_vllm_tool_parser_namespace_tool(patch_logger) | ||
| _patch_vllm_radio_layerscale_loader(patch_logger) | ||
| _patch_vllm_glm_decoder_sequence_parallel_moe(patch_logger) | ||
| _patch_vllm_minimax_m3_topk_buffer_layout(patch_logger) |
There was a problem hiding this comment.
nemo_rl/models/generation/vllm/patches.py:829
No action needed — suggestion only, not blocking.
Pre-existing pattern, this PR continues it. _patch_vllm_* is now 8 functions, hand-registered across two call sites (ensure_vllm_source_compat calls 4, _apply_vllm_patches calls 7 — an intentional subset/superset, not full duplication). A 9th patch author could add their function's body but forget one of the two registration sites — e.g. a diagnostics-only path that calls only ensure_vllm_source_compat would silently run unpatched.
Follow-up
A small registry (list[VllmPatch(name, apply, early_import_safe)] + a single loop per call site) would remove the risk of a call-site desync. Worth tracking given the file's growth rate — no tracking issue filed yet.
| return tokenizer | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "use_te, optimizer_kwargs, expected_dtype", |
There was a problem hiding this comment.
tests/unit/models/automodel/test_automodel_setup.py:919
No action needed — minor suggestion.
PR-introduced, minor. The new parametrized cases for _has_optimizer_fp32_master never exercise the except ImportError: return False branch or the optimizer_cls is not FusedAdam branch — both short-circuit before those paths are reached.
A small direct unit test calling _has_optimizer_fp32_master in isolation (no sys.modules monkeypatching needed) for these two branches would close the gap more cheaply than routing through the full setup_model_and_optimizer integration test.
What does this PR do?
Add MiniMax-M3 GRPO training support with the AutoModel backend and BF16 vLLM generation.
Issues
No linked NeMo-RL issue. The vLLM compatibility patch references upstream issue vllm-project/vllm#48603.
Usage
From a 32-node allocation with 8 GPUs per node:
Before your PR is "Ready for review"
Additional Information
git diff --checkpassed against the locally recorded main merge base.