Skip to content

feat: add BaseMultiRewardVerifyResponse for shared multi-reward contract - #2050

Merged
init-nikhil merged 7 commits into
mainfrom
feat/base-multi-reward-verify-response
Jul 29, 2026
Merged

feat: add BaseMultiRewardVerifyResponse for shared multi-reward contract#2050
init-nikhil merged 7 commits into
mainfrom
feat/base-multi-reward-verify-response

Conversation

@init-nikhil

@init-nikhil init-nikhil commented Jul 16, 2026

Copy link
Copy Markdown
Member

Define reward_components once in nemo_gym as the trainer-agnostic producer-side contract, and migrate example_tool_call_multireward to inherit it.

TODO: I'm updating docs to reflect these changes.

Closes #1664

Define reward_components once in nemo_gym as the trainer-agnostic
producer-side contract, and migrate example_tool_call_multireward to
inherit it.
Closes #1664

Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
@init-nikhil

Copy link
Copy Markdown
Member Author

Making the reward_components field required

In the case of a MultiRewardVerifyResponse, I think we should keep the reward_components field required (non-optional).

if you subclass MultiRewardVerifyResponse, you're saying this env is multi-reward so reward_components should always be there
optional means trainers still have to handle the missing case, which defeats the point of a shared contract
opt-in is at the class level (MultiRewardVerifyResponse vs BaseVerifyResponse), not "maybe this field is set"

@init-nikhil

Copy link
Copy Markdown
Member Author

Not providing sum(reward_components.values() as the default reward

I don't think we should implicitly aggregate with sum. The aggregation strategy is part of the reward/task definition, and an unweighted sum isn't the common case in modern LLM post-training. Making sum the default bakes in an opinionated choice that will be wrong for many workloads.

verify() in each env definition will specify how to aggregate rewards and if that's not enough, advanced trainer integrations can choose to do custom logic using the reward components.

@github-actions github-actions Bot added the sla:triage-overdue Review assignment is over the one-business-day SLA label Jul 17, 2026
@anwithk
anwithk requested a review from ananthsub July 17, 2026 22:36
@github-actions github-actions Bot added sla:review-overdue Review response is over the one-business-day SLA and removed sla:triage-overdue Review assignment is over the one-business-day SLA labels Jul 17, 2026

@ananthsub ananthsub left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please also update the docs in fern/versions/latest/pages/api-reference/index.mdx and fern/versions/latest/pages/build-verifiers/multi-reward-verification.mdx

so that BaseMultiRewardVerifyResponse is documented for users too

@github-actions github-actions Bot added sla:author-overdue Author response is over the one-business-day SLA and removed sla:review-overdue Review response is over the one-business-day SLA labels Jul 21, 2026
…d-verify-response

Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>

# Conflicts:
#	tests/unit_tests/test_base_resources_server.py
Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
@init-nikhil

Copy link
Copy Markdown
Member Author

Aggregate metrics (and reward profiler?) go off of top level numeric fields in BaseVerifyResponse. I think we want these updated to go into the reward_components dict when its present?

@ananthsub lmk if that makes sense to you and I'll add that support in a follow on PR.

ananthsub
ananthsub previously approved these changes Jul 24, 2026
Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
@init-nikhil

Copy link
Copy Markdown
Member Author

@ananthsub just realized that if I make changes after a PR approval, it'll need approval again, so re-requesting :)

@github-actions github-actions Bot added the sla:review-overdue Review response is over the one-business-day SLA label Jul 27, 2026
@github-actions github-actions Bot removed the sla:review-overdue Review response is over the one-business-day SLA label Jul 28, 2026
@init-nikhil
init-nikhil enabled auto-merge (squash) July 29, 2026 01:43
@init-nikhil
init-nikhil merged commit b995852 into main Jul 29, 2026
30 checks passed
OlegSudakov pushed a commit to OlegSudakov/Gym that referenced this pull request Aug 7, 2026
…act (NVIDIA-NeMo#2050)

Define reward_components once in nemo_gym as the trainer-agnostic
producer-side contract, and migrate example_tool_call_multireward to
inherit it.

TODO: I'm updating docs to reflect these changes.

Closes NVIDIA-NeMo#1664

---------

Signed-off-by: Nikhil Deshmudre <nikhil.deshmudre@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: shared MultiRewardVerifyResponse (reward_components) as a trainer-agnostic multi-reward contract

2 participants