Skip to content

fix: aggregate rollout metrics by semantic type (min/max/sum/mean) - #2175

Merged
yuki-97 merged 1 commit into
mainfrom
yifu/fix-rollout-metrics-min-max-aggregation
May 22, 2026
Merged

fix: aggregate rollout metrics by semantic type (min/max/sum/mean)#2175
yuki-97 merged 1 commit into
mainfrom
yifu/fix-rollout-metrics-min-max-aggregation

Conversation

@yfw

@yfw yfw commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Previously all numeric rollout metrics were averaged when aggregating across trajectory groups, giving misleading values for min/max/total metrics. Now min-prefixed/suffixed metrics use min(), max uses max(), total_turns is summed, and the rest are averaged.

Extracts aggregate_rollout_metrics() helper for testability and adds unit tests covering each aggregation path.

Closes #1955

What does this PR do ?

Add a one line overview of what this PR aims to accomplish.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

@yfw
yfw requested review from a team as code owners March 30, 2026 22:59
@yfw yfw added the super-v3 label Mar 30, 2026
@copy-pr-bot

copy-pr-bot Bot commented Mar 30, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@yfw
yfw force-pushed the yifu/fix-rollout-metrics-min-max-aggregation branch from 8bb0339 to 3b197f3 Compare March 30, 2026 23:01
Comment thread nemo_rl/algorithms/grpo.py
Previously all numeric rollout metrics were averaged when aggregating
across trajectory groups, giving misleading values for min/max/total
metrics. Now min-prefixed/suffixed metrics use min(), max uses max(),
total_turns is summed, and the rest are averaged.

Extracts `aggregate_rollout_metrics()` helper for testability and adds
unit tests covering each aggregation path.

Closes #1955

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
@macandro96
macandro96 force-pushed the yifu/fix-rollout-metrics-min-max-aggregation branch from 3b197f3 to ebb202a Compare May 21, 2026 15:16
@macandro96

Copy link
Copy Markdown
Contributor

/ok to test ebb202a

@macandro96 macandro96 added the CI:L0 Run doctests and unit tests label May 21, 2026
@macandro96

Copy link
Copy Markdown
Contributor

/ok to test ebb202a

@macandro96 macandro96 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L0 Run doctests and unit tests labels May 21, 2026
@macandro96
macandro96 requested a review from yuki-97 May 21, 2026 20:37
@yuki-97
yuki-97 merged commit 501cd12 into main May 22, 2026
69 of 71 checks passed
@yuki-97
yuki-97 deleted the yifu/fix-rollout-metrics-min-max-aggregation branch May 22, 2026 13:24
yfw added a commit that referenced this pull request May 27, 2026
…2175)

Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L1 Run doctests, unit tests, and functional tests super-v3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[super-pr] fix min&max metric

3 participants