Repository navigation
chore(dtensor): move remaining DTensor v1 configs to v2 - #4196
Conversation
|
/ok to test f201ac1 |
|
/ok to test 49a5f85 |
49a5f85 to
bb23261
Compare
|
/ok to test 85dc155 |
f1deffd to
75e3298
Compare
75e3298 to
6bc8577
Compare
|
/ok to test 6bc8577 |
yuki-97
left a comment
There was a problem hiding this comment.
Team review of the v1 to v2 config flip: 4 agents (RL/config expert, bug-finder, test reviewer, devil's advocate) plus a leader verification pass.
The mechanical core of this PR checks out. Verified independently:
defaults:inheritance is an OmegaConf deep merge (nemo_rl/utils/config.py:46), so the partialcheckpoint:overrides on the deepscaler recipes keep the parent'ssave_consolidated/single_rank_consolidation/consolidation_timeout_minutes.- All 7
tests/unit/reference_configs/pairs passtest_config_v2.py::test_reference_configs_up_to_date(missing=[] different=[]re-run by hand); comments and key order are irrelevant to that test, and no other twin needed updating. - Every config that resolves to
_v2: trueandenabled: truecarries a legalmodel_save_format, so nothing hits thebuild_checkpoint_configValueError. - The
torch_savevssafetensorssplit is correct: the only 4 liveconvert_dcp_to_hfcallers are the deepscaler scripts, and all of their recipes inherittorch_savethroughdefaults:. - Per-config v2 feature compatibility was checked against the pinned Automodel submodule and came back clean — the gemma3 tp=8 parallel plan (Automodel carries an identical
_parallelize_gemma3registered forGemma3ForConditionalGeneration), the RMscore.head path,context_parallel_size: 4,cpu_offload, and the dynamo path.
Two comments below, neither blocking. Several candidate findings were investigated and dropped as unfounded, including a suspected convert_dcp_to_hf breakage across the flipped configs and a suspected v1 residue in the vlm_mpo family.
One status note rather than a finding: no functional test has run on 6bc8577 yet — the green checks are lint/release/semantic, and the test approval queue entry is still waiting.
Generated by Claude Code
6bc8577 to
84dcc5f
Compare
|
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. |
|
/ok to test 84dcc5f |
|
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. |
3bfd34d to
b243070
Compare
terrykong
left a comment
There was a problem hiding this comment.
The migration itself is complete and correct, and that is most of this PR.
A recursive walk over every dtensor_cfg in examples/**, tests/unit/reference_configs/ and research/ — 417 blocks — finds nothing that resolves to _v2: false, and no enabled v2 block missing a legal model_save_format. The torch_save / safetensors split is right: every deepscaler recipe whose .sh runs convert_dcp_to_hf.py resolves to torch_save, which it needs, since it reads a DCP .metadata file that safetensors does not write. All 7 reference-config pairs were compared key by key: no missing keys, no differences, and the dpo/rm twin updates were required. The key deletions all resolve correctly through their parents, and no doc tells users to set _v2: false.
The stack order is right, too: flip the values here so CI stays green, then turn on the guard in #4207. There is no commit in the stack where the repo's own configs fail its own check.
One thing worth knowing before merge: no unit or functional lane has run on the current head b243070a. The last /ok to test named 84dcc5f, which was force-pushed away, so only the DCO, copyright, secrets, semantic and submodule checks are green. test_all_config_dtensor_selects_v2 has never executed in CI on this head. An /ok to test b243070a would cover that.
The findings below come from 7 agents plus a leader verification pass. The first adversarial pass went silent and was stopped; a second independent adversarial pass then reviewed the full set, and several candidate findings were dropped as unfounded.
Generated by Claude Code
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com> ci: drop the step-20 logprob-error check from the dapo cp2 recipe Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com>
b243070 to
e92bfea
Compare
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com>
|
/ok to test 9546acb |
Brings #4198 (remove the DTensor v1 policy worker), #4207, #4196, #4143 (MiniMax-M3 GRPO support), #4233, #4232 and #3408 (flow_grpo research). Conflicts and resolution: - pyproject.toml: main removed the `fsdp` extra together with the v1 worker; this branch had given that extra its own torch 2.13 pin. The extra is dropped here as well (with its `[tool.uv] conflicts` entries), and the per-extra torch comment no longer lists it. - uv.lock: regenerated from this branch's lock; the only change is the addition of #3408's flow-grpo research dependencies (albumentations, paddleocr, scikit-image, ...). No existing package moved, and nothing resolves below main except the deliberate numpy / llguidance / flash-attn pins. Also removed, not conflicting: `_patch_vllm_minimax_m3_topk_buffer_layout` (#4143) and its tests. It backported vllm-project/vllm@d1a8ba63 (token-major vs head-major `topk_indices_buffer`) onto vLLM 0.26.0 and its docstring asks for removal at vLLM >= 0.27; vLLM 0.29.0's `minimax_m3/common/indexer.py` already carries the head-major `buf_htk` layout, so on this branch the patch could only ever log "source shape not found" and its installed-vLLM anchor tests would fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
What does this PR do ?
Moves every remaining DTensor v1 config in the repo onto the v2 (Automodel) backend, so that the v1 worker can be deleted in a follow-up PR (#4198).
policy.dtensor_cfg._v2defaults toFalse(nemo_rl/models/policy/lm_policy.py), so a config thatnever mentions
_v2silently runs the v1DTensorPolicyWorker. Between that default and 6 filespinning
_v2: falseexplicitly, 20 resolveddtensor_cfgblocks were still on v1.What changed
8 root files were edited; everything else inherits via
defaults:.checkpoint.model_save_formatwas added wherever it was missing ornull—build_checkpoint_config(nemo_rl/models/automodel/checkpoint.py) requires it on the v2 path.Exemplar configs (covered by
CI:L1functional tests)examples/configs/dpo.yamlexamples/configs/rm.yamlexamples/configs/grpo_math_1B_dynamo.yamlRoots for the nightly / release recipes
examples/configs/recipes/llm/grpo-deepscaler-1.5b-8K.yamlexamples/configs/recipes/llm/grpo-gspo-deepscaler-1.5b-8K.yamlexamples/configs/recipes/llm/grpo-gemma3-27b-it-8n8g-fsdp2tp8-actckpt-long.yamlexamples/configs/recipes/llm/dapo-qwen2.5-7b.v2.yamlexamples/nemo_gym/grpo_workplace_assistant_nemotron_nano_v2_9b.yamlInherited via
defaults:— not edited, but flipped to v2recipes/llm/grpo-deepscaler-1.5b-16K.yaml(nightly)recipes/llm/grpo-deepscaler-1.5b-24K.yaml(nightly)recipes/llm/grpo-deepscaler-1.5b-8K-tq_simple.yaml(nightly)recipes/llm/grpo-deepscaler-1.5b-1n4g-8K.yaml(nightly_gb200)recipes/llm/grpo-gspo-deepscaler-1.5b-8K-tq_simple.yaml(nightly)recipes/llm/grpo-gemma3-27b-it-8n4g-fsdp2tp4-actckpt-long.yaml(release_gb200)recipes/llm/dapo-qwen2.5-7b-16n4g-fsdp2cp2.v2.yaml(release_gb200)recipes/llm/dpo-llama3.2-1b-instruct-1n8g-fsdp2tp1.v2.yaml(nightly)recipes/llm/dpo-mistral-nemo-instruct-2407-1n8g-fsdp2tp8-actckpt-long.yaml(nightly)recipes/llm/dpo-llama3.2-1b-instruct-1n4g-fsdp2tp1.v2.yaml(nightly_gb200)recipes/llm/dpo-llama3.1-8b-instruct-4n8g-fsdp2tp4.yaml(release)recipes/llm/dpo-llama3.1-8b-tulu3-1n8g-fsdp2tp1.yaml(release)recipes/llm/gdpo-qwen2.5-1.5b-1n8g-gym-multireward.yaml(nightly)Also in the diff
tests/test_suites/llm/dapo-qwen2.5-7b.v2.shanddapo-qwen2.5-7b-16n4g-fsdp2cp2.v2.sh— drop the flaky terminal-steptoken_mult_prob_error["20"] < 1.05check; themedian(...) < 1.1bound in the same file stays.tests/unit/test_config_validation.py—test_all_config_dtensor_selects_v2walks every config withdefaults:resolved and fails on_v2: false, or on a missing_v2whendtensor_cfgis enabled.tests/unit/reference_configs/{dpo,rm}.yaml— updated to match the exemplar edits above.Verification
Only the recipes below were run (roots, or the ones most likely to be affected), all pass -- the other nightly / release recipes were not run:
grpo-deepscaler-1.5b-8K(root)grpo-deepscaler-1.5b-16K(inherited)grpo-gspo-deepscaler-1.5b-8K(root)grpo-gemma3-27b-it-8n8g-fsdp2tp8-actckpt-long(root)dapo-qwen2.5-7b.v2(root)gdpo-qwen2.5-1.5b-1n8g-gym-multireward(inherited)🤖 Generated with Claude Code