Skip to content

fix(metrics): report raw reward before dynamic sampling filters - #4486

Open
guapisolo wants to merge 4 commits into
mainfrom
codex/raw-reward-before-filter
Open

guapisolo wants to merge 4 commits into
mainfrom
codex/raw-reward-before-filter

Conversation

@guapisolo

@guapisolo guapisolo commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Make rollout/raw_reward include samples dropped by dynamic sampling.

Symptom & Reproduction

  • Symptom: The default reward curve describes the selected training batch, hiding changes in the fraction of all-correct or all-wrong groups.
  • Reproduction: With eight [1, 1] groups, one [0, 0], and one [1, 0], the zero-std filter leaves a training mean of 0.5 even though the generated samples average 0.85. TestMetricGathererRawReward.test_the_mean_covers_kept_and_dropped_groups_alike pins this example.

Root Cause

  1. MetricGatherer already collects pre-filter rewards but publishes them under a separate rollout/raw_reward_unfiltered key (#3073).
  2. log_rollout_data in the trainer publishes the retained, potentially post-processed batch mean as rollout/raw_reward.

Fix

Publish the existing producer aggregate directly as rollout/raw_reward, removing rollout/raw_reward_unfiltered. Rename only the trainer's logged mean to rollout/raw_reward_filtered, so later training logs cannot overwrite generation metrics. Training data, reward post-processing, advantages, and filtering remain unchanged.

The shared gatherer covers synchronous, fully async, and verifiers rollouts. It averages available scalar scores through --reward-key; missing or unkeyed structured rewards contribute no point. Custom rollout implementations must provide the metric themselves. Fully async collection windows can span weight versions.

Verification

  • Focused pytest suite: 385 passed on 4b5aaab4880b; the follow-up changes documentation only, covering filtering, synchronous/fully async rollouts, metric logging, and CI history integration.
  • The three regression cases fail on base a6d60232ca65: the 0.85 example and producer/trainer logging separation for single/named policies.
  • GPU e2e on c3a94da524 via /rerun-test: test_qwen2.5_0.5B_gsm8k_short.py passed (step 0: rollout/raw_reward 0.653, rollout/raw_reward_filtered 0.594 after the zero-std filter), and the GLM-4.7 fully async test passed its metric-history gate on rollout/raw_reward at steps 0 and 1.
  • All applicable pre-commit checks and git diff --check passed.

Review Focus

  • MetricGatherer remains the sole owner of the pre-filter mean; trainer logging must never republish that key.
  • Custom rollouts that previously relied on trainer-only logging.

Use the rollout producer mean for rollout/raw_reward so discarded all-correct and all-wrong groups remain visible. Keep the retained training-batch mean under rollout/raw_reward_filtered without changing training rewards or advantages.
@guapisolo
guapisolo requested a review from fzyzcjy as a code owner October 28, 2026 00:09

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@guapisolo

Copy link
Copy Markdown
Collaborator Author

@claude review always

Comment thread miles/backends/training_utils/metrics/log_utils.py
@guapisolo

Copy link
Copy Markdown
Collaborator Author

/rerun-test tests/e2e/short/test_qwen2.5_0.5B_gsm8k_short.py

@guapisolo

Copy link
Copy Markdown
Collaborator Author

/rerun-test tests/e2e/megatron/test_glm47_flash/test_spec_mtptrain_nor3_fp8_sgl_tp2_meg_tp1cp1_fullyasync.py

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ tests/e2e/megatron/test_glm47_flash/test_spec_mtptrain_nor3_fp8_sgl_tp2_meg_tp1cp1_fullyasync.py passed on stage-c-8-gpu-h200 in 14m21s — workflow run

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ tests/e2e/short/test_qwen2.5_0.5B_gsm8k_short.py passed on stage-c-2-gpu-h200 in 6m18s — workflow run

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

Point dashboards and alerts built on the old retained-batch mean at rollout/raw_reward_filtered.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
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.

1 participant