Repository navigation
docs: add multi-reward verification - #1665
Conversation
|
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. |
cwing-nvidia
changed the title
|
🌿 Preview your docs: https://nvidia-preview-cwing-docs-multireward-tutorial.docs.buildwithfern.com/nemo/gym Here are the markdown pages you've updated: |
|
TODO: update the comments in app.py for documentation consistency |
ffrujeri
left a comment
ffrujeri
left a comment
There was a problem hiding this comment.
Thanks for this — a clear, well-cross-referenced walkthrough of the multi-reward pattern. A team review (docs-only, 3 files) verified the load-bearing technical claims against the code: reward = sum(components), the Pydantic field-drop rationale for subclassing BaseVerifyRequest, and the "aggregate metrics only surfaces top-level numeric fields, not nested dicts" behavior (confirmed in nemo_gym/reward_profile.py). All internal links, the GDPO arXiv reference, and the code snippets check out.
Two small, non-blocking suggestions are left inline. One housekeeping note: the branch is currently behind main (draft, auto-sync disabled), so a routine rebase before merge would be good.
Generated by Claude Code
| `reward_components` and normalizes each component independently, rather than collapsing | ||
| them into one number. A GRPO baseline instead reads the summed `reward` and therefore | ||
| cannot distinguish rollouts with the same total but different composition — the advantage | ||
| collapse GDPO is designed to fix. How `reward_components` reaches the trainer depends on |
There was a problem hiding this comment.
The README/tutorial now frame this for both evaluation and multi-objective RL and soften the trainer wording to "depends on the training framework's NeMo Gym integration." The app.py module docstring (L14-33) and the reward_components field comment (L62-64) still describe it as "intended for GDPO" and assert the specific "NeMo-RL exposes them as reward1, reward2, … ordered by component name" ordering. Would it be worth aligning those app.py comments in this PR so the code and docs tell one story? (Looks like it matches your TODO above.)
| @@ -0,0 +1,182 @@ | |||
| --- | |||
| title: "Multi-Reward Verification" | |||
There was a problem hiding this comment.
multi-reward-environment.mdx:2
Nice addition! Every other tutorial has a <Card> in the Environment Tutorials Overview grid — would it make sense to add one for Multi-Reward too, so readers browsing the Overview can discover it?
There was a problem hiding this comment.
intentionally didn't add those nav elements previously since I was waiting on another PR, this is going to move to a new Build Verifiers section
ffrujeri
left a comment
ffrujeri
left a comment
There was a problem hiding this comment.
Just left a few small suggestions
8d0e4d6 to
d1a0e9e
Compare
d1a0e9e to
c7ffa26
Compare
sephmard
left a comment
sephmard
left a comment
There was a problem hiding this comment.
This is great. A few small nits worth consideration before merge - but approving as is:
1. Step 3 wording. "The sum of your components is the usual default" reads like a contract. Worth softening to "a common convention" since weighted sums or non-additive combinations are valid too.
2. Split verify() snippet. the "Score each objective" and "Return all three forms" sections each show part of the verify() method body, but it's not obvious they're meant to be one continuous method. Someone copying the code would have to manually stitch them together. A # ... between the two blocks, or merging them into one, would make the continuity explicit.
3. predicted_calls on the response class it appears in the code block but isn't mentioned in the surrounding text. Worth a one-liner clarifying it's for debugging/inspection and not required by the pattern.
Merge order note: this PR and #1781 both modify build-verifiers/index.mdx (both remove the Resources Server card, but in different ways). Whichever lands second will need a quick rebase to confirm the index ends up with Verification Patterns + Multi-Reward cards and nothing else.
Approved.
Add a how-to for returning multiple reward components from a verifier, useful for per-objective evaluation and multi-objective RL (e.g. GDPO). Link it from the README tutorials table and the example_tool_call_multireward server README. Signed-off-by: Chris Wing <cwing@nvidia.com>
- Move the Multi-Reward Verification page from Environment Tutorials into the Build Verifiers section and add it as a card on the section index - Point README and example-server README links at the new URL - Fix the aggregate-metrics link to /evaluation/aggregate-metrics - Align example_tool_call_multireward app.py comments with the docs: framed for both evaluation and multi-objective RL, with trainer consumption described as framework-integration dependent Signed-off-by: Chris Wing <cwing@nvidia.com>
c7ffa26 to
9fafb6f
Compare
- Soften the scalar reward wording: summing components is "a common convention" rather than a default contract - Make the split verify() snippet's continuity explicit with continuation markers across the scoring and return blocks - Drop the unexplained predicted_calls field from the response class snippet (example-specific debug output, not part of the pattern) Signed-off-by: Chris Wing <cwing@nvidia.com>
|
@sephmard rebased to latest main and addressed your feedacbk |
Summary
reward_componentsdict, per-objective top-level fields, and the summed scalarreward— for per-objective evaluation and multi-objective RL (e.g. GDPO).example_tool_call_multirewardserver README, and aligns that README's wording with the tutorial.Status of each path
reward_componentsreaches the trainer" to the framework-integration docs, since consuming components end-to-end depends on per-framework NeMo Gym integrationTest plan
fern checkpasses