feat: configurable GDPO per-reward weights and multi-reward NeMo Gym bridge - #2680
Conversation
ZhiyuLi-Nvidia
left a comment
There was a problem hiding this comment.
Generally looks good to me. Only 2 comments here.
|
/ok to test cf64525 |
|
Also could you rebase on to main branch after resolving conflicts? |
…bridge Add reward_weights to the GDPO advantage estimator so components aggregate as A = sum_n w_n * A_n (defaults to equal weights; wrong length raises). Extend the NeMo Gym bridge to surface per-component rewards (reward1..rewardN) from a verifier that returns reward_components, mirroring the native multi-reward rollout path, so GDPO can train on NeMo Gym environments. Includes unit tests and docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anjali Shah <anjshah@nvidia.com>
…ard env Name the ready-made multi-reward reference env (NVIDIA-NeMo/Gym) so users know exactly what to set env.nemo_gym to for an end-to-end GDPO multi-reward run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anjali Shah <anjshah@nvidia.com>
40fc0fa to
fe17cee
Compare
yes done! |
Thanks @ZhiyuLi-Nvidia. Resolved conflicts and rebased. Please merge, thanks. |
|
Found DCO failure. Could you finally |
…ate reward == sum(components) Addresses review on NVIDIA-NeMo#2680: - Stop silently overwriting final_batch["total_reward"] with the component sum. Keep the verifier's scalar `reward`. When reward_components is present the contract is reward == sum(components), so overwriting is a no-op in the correct case and only masks a misconfigured verifier otherwise. Validate the contract instead and raise on a real mismatch. - Expand the sorted() comment to explain it is a canonical ordering (stable reward{n} -> component mapping for the GDPO per-component baseline and reward_weights), not just deduplication. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anjali Shah <anjshah@nvidia.com>
fe17cee to
cb7f6b5
Compare
|
/ok to test cb7f6b5 |
- Move the reward/component-sum consistency check into validate_reward_components_match_scalar() in nemo_gym.py, next to extract_reward_components, so it is unit-testable without the full rollout machinery. run_async_nemo_gym_rollout now calls the helper. - Add test_validate_reward_components_match_scalar covering the matching, float-tolerance, single-reward-skip, and mismatch-raises cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anjali Shah <anjshah@nvidia.com>
cb7f6b5 to
de41394
Compare
|
/ok to test de41394 |
|
/ok to test de41394 |
yuki-97
left a comment
There was a problem hiding this comment.
thanks @anjalibshah for supporting the two features! left some comments.
…eward2)
main's get_gdpo_reward_component_keys() selects batch keys by
`k.startswith("reward/")`, but the NeMo Gym bridge emitted positional
`reward1, reward2, ...`. Those never match, so GDPO saw zero components and
raised "GDPO requires multiple reward components". Emit `reward/<name>`
instead, matching the native multi-reward path.
- rollouts.py: emit final_batch[f"reward/{name}"] (component name carries
identity; get_gdpo_reward_component_keys sorts, so no positional index).
- test_grpo.py: reward_weights test now uses reward/correctness, reward/format.
- Reword reward_weights ordering from "reward1, reward2, ..." to "alphabetical
by component name" across advantage_estimator.py, grpo.py, grpo.md, and
gdpo_multireward.yaml; update nemo_gym.py docstring.
- Remove dead private-fork link (gym-multireward-scope.md) from the recipe yaml.
Addresses review feedback on NVIDIA-NeMo#2680.
Signed-off-by: Anjali Shah <anjshah@nvidia.com>
|
/ok to test fce0393 |
Guards the reward_weights aggregation path added to GDPOAdvantageEstimator (per review on #2680). Adds a DTensor GDPO recipe over the native math_multi_reward env (correctness / format / integer components), Qwen2.5-1.5B on GSM8K, 1n8g, with reward_weights=[1.0, 0.5, 0.25]. Follows the nightly pattern from #1866: - examples/configs/recipes/llm/gdpo-qwen2.5-1.5b-1n8g-fsdp2-reward-weights.yaml - tests/test_suites/llm/gdpo-qwen2.5-1.5b-1n8g-fsdp2-reward-weights.sh - register in tests/test_suites/nightly.txt - add "gdpo" -> gdpo_math_1B.yaml to ALGO_MAPPING_TO_BASE_YAML so the recipe name-prefix check passes. Signed-off-by: Anjali Shah <anjshah@nvidia.com>
Guards the reward_weights aggregation path added to GDPOAdvantageEstimator (per review on NVIDIA-NeMo#2680). Adds a DTensor GDPO recipe over the native math_multi_reward env (correctness / format / integer components), Qwen2.5-1.5B on GSM8K, 1n8g, with reward_weights=[1.0, 0.5, 0.25]. Follows the nightly pattern from NVIDIA-NeMo#1866: - examples/configs/recipes/llm/gdpo-qwen2.5-1.5b-1n8g-fsdp2-reward-weights.yaml - tests/test_suites/llm/gdpo-qwen2.5-1.5b-1n8g-fsdp2-reward-weights.sh - register in tests/test_suites/nightly.txt - add "gdpo" -> gdpo_math_1B.yaml to ALGO_MAPPING_TO_BASE_YAML so the recipe name-prefix check passes. Signed-off-by: Anjali Shah <anjshah@nvidia.com>
fce0393 to
9143bf2
Compare
|
/ok to test 9143bf2 |
terrykong
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on the review feedback — both follow-up commits were verified locally at head fce0393c:
- d00b431 fully resolves the
reward/<name>key-scheme issue: the previously-failingtest_gdpo_advantage_estimator_reward_weightsnow passes, a repo-wide search finds no remainingreward1/reward2references, all ordering docs/comments/error messages are reworded consistently, and the dead skill link is gone. Nice, thorough fix. - fce0393 adds a well-plumbed native-path nightly (recipe resolution, naming registration, and alphabetical weight→component mapping all verified). Two follow-ups inline: the recipe yaml currently fails the
minimize-checkpre-commit hook (CI blocker, one-command fix), and the Gym bridge block itself still has no test coverage since the nightly runs the nativemath_multi_rewardpath.
This review was produced by a 5-agent team (RL expert, test agent, bug finder, comment reviewer, devil's advocate); all findings were independently verified, and all other pre-commit hooks (ruff, ruff-format, pyrefly) pass at head.
Relationship to #2681 (same author): no code dependency, but the demo script there feeds reward1/reward2 keys into GDPOAdvantageEstimator, so with the key scheme now settled as reward/<name> it will need the matching update.
Generated by Claude Code
|
merged. @anjalibshah agreed offline to address the reviews in #2681 regarding: |
…-experiment skill Per review on #2681: experiment-specific content doesn't fit the repo's skill convention (existing skills are general infra/process workflows). Remove the skills/nemo-rl-gdpo-experiment/ directory and fold the genuinely additive pieces into the existing GDPO section of docs/guides/grpo.md: - matched GDPO-vs-GRPO comparison workflow (points at the examples/ control config and CPU collapse demo, both unchanged) - what to measure (per-reward convergence, per-prompt advantage spread) - practical notes: reward_scaling applies per component; final batch normalization always runs regardless of normalize_rewards Deliberately excludes reward_weights guidance to avoid overlap with #2680, which documents that option. Env-authoring contract is left in docs/guides/environments.md where it already lives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anjali Shah <anjshah@nvidia.com>
`spinup_server` is not a field Gym has ever read. It is absent from nemo_gym/server_utils.py at both the pin it was written against (d67ad6611, via #2680) and the current pin (c3bac9631), and nothing on the config MRO overrides pydantic's default extra="ignore" -- the only model_config in the chain is ConfigDict(arbitrary_types_allowed=True) at server_utils.py:295 -- so the key was silently discarded on every load. What actually decides whether a server is launched is the presence of an entrypoint (cli/env.py:407, unchanged since d67ad6611:188). These thin clients carry one, so they do spin up; the dry run simply returns early at server_utils.py:698 before serving. The conclusion in the previous comment was right -- validation is unconditional -- but the reason given described a mechanism that does not exist, which made a dead key read as load-bearing. Drop the six occurrences and reword the comment to the real mechanism. Upstream Gym's own resources_servers/jailbreak_detection/configs/safety_judge_model.yaml defines the same server without the key, and it has no reader anywhere in this repo. No behaviour change: tests/unit/environments/test_nemo_gym_prefetch_configs.py still reports 7 passed, 2 skipped against the edited configs, which is the expected result if the key was genuinely inert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
`spinup_server` is not a field Gym has ever read. It is absent from nemo_gym/server_utils.py at both the pin it was written against (d67ad6611, via #2680) and the current pin (c3bac9631), and nothing on the config MRO overrides pydantic's default extra="ignore" -- the only model_config in the chain is ConfigDict(arbitrary_types_allowed=True) at server_utils.py:295 -- so the key was silently discarded on every load. What actually decides whether a server is launched is the presence of an entrypoint (cli/env.py:407, unchanged since d67ad6611:188). These thin clients carry one, so they do spin up; the dry run simply returns early at server_utils.py:698 before serving. The conclusion in the previous comment was right -- validation is unconditional -- but the reason given described a mechanism that does not exist, which made a dead key read as load-bearing. Drop the six occurrences and reword the comment to the real mechanism. Upstream Gym's own resources_servers/jailbreak_detection/configs/safety_judge_model.yaml defines the same server without the key, and it has no reader anywhere in this repo. No behaviour change: tests/unit/environments/test_nemo_gym_prefetch_configs.py still reports 7 passed, 2 skipped against the edited configs, which is the expected result if the key was genuinely inert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
`spinup_server` is not a field Gym has ever read. It is absent from nemo_gym/server_utils.py at both the pin it was written against (d67ad6611, via #2680) and the current pin (c3bac9631), and nothing on the config MRO overrides pydantic's default extra="ignore" -- the only model_config in the chain is ConfigDict(arbitrary_types_allowed=True) at server_utils.py:295 -- so the key was silently discarded on every load. What actually decides whether a server is launched is the presence of an entrypoint (cli/env.py:407, unchanged since d67ad6611:188). These thin clients carry one, so they do spin up; the dry run simply returns early at server_utils.py:698 before serving. The conclusion in the previous comment was right -- validation is unconditional -- but the reason given described a mechanism that does not exist, which made a dead key read as load-bearing. Drop the six occurrences and reword the comment to the real mechanism. Upstream Gym's own resources_servers/jailbreak_detection/configs/safety_judge_model.yaml defines the same server without the key, and it has no reader anywhere in this repo. No behaviour change: tests/unit/environments/test_nemo_gym_prefetch_configs.py still reports 7 passed, 2 skipped against the edited configs, which is the expected result if the key was genuinely inert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
Summary
Two GDPO (arXiv:2601.05242) capability additions on top of the existing GDPO support (#2069).
1. Configurable per-reward weights. GDPO's aggregation
A = Σ wₙ·Aₙis now tunable viagrpo.adv_estimator.reward_weights(one entry per component, ordered to matchreward1, reward2, …). Defaults to equal weights (all1.0) for backward compatibility; a wrong-length list raisesValueError. Files:nemo_rl/algorithms/advantage_estimator.py,nemo_rl/algorithms/grpo.py,docs/guides/grpo.md.2. Multi-reward NeMo Gym bridge.
run_async_nemo_gym_rolloutnow surfaces per-component rewards (reward1…rewardN) when a Gym verifier returnsreward_components, mirroring the native multi-reward rollout path. Single-reward environments are unaffected (fall back to the scalarreward). Files:nemo_rl/environments/nemo_gym.py(newextract_reward_components),nemo_rl/experience/rollouts.py,examples/nemo_gym/gdpo_multireward.yaml.Dependency
The bridge consumes the
reward_componentsfield added in NVIDIA-NeMo/Gym#1525 — that should land first. Until it does, the bridge code is inert (single-reward fall-back), so this PR is safe to merge independently.Test plan
Validated in the NeMo-RL container (8×H100):
tests/unit/algorithms/test_grpo.py::test_gdpo_advantage_estimator_reward_weights— PASSEDtests/unit/environments/test_nemo_gym.py::test_extract_reward_components— PASSED