Skip to content

Align aggregate metrics with persisted rollouts#1799

Merged
marta-sd merged 2 commits into
NVIDIA-NeMo:mainfrom
fallintoplace:fix/aggregate-only-persisted-rollouts
Jul 24, 2026
Merged

Align aggregate metrics with persisted rollouts#1799
marta-sd merged 2 commits into
NVIDIA-NeMo:mainfrom
fallintoplace:fix/aggregate-only-persisted-rollouts

Conversation

@fallintoplace

@fallintoplace fallintoplace commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • aggregate rollout metrics from the rows written to the main rollouts JSONL only
  • keep sidecar failures and _ng_no_persist attempts out of runtime /aggregate_metrics
  • preserve already-persisted rows when resuming from cache
  • add regression coverage for successful, sidecar-failure, no-persist, and resumed rollouts

Root cause

run_from_config() appended every result to the in-memory results list before the persistence gate, then passed that full list into _call_aggregate_metrics(). That let runtime aggregation include rows that never landed in output.jsonl, so gym eval run could disagree with gym eval aggregate.

Validation

  • uv run --extra dev python -m pytest tests/unit_tests/test_rollout_collection.py

@copy-pr-bot

copy-pr-bot Bot commented Jun 27, 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.

@nemo-automation-bot nemo-automation-bot Bot added the community-request Issue reported or requested by someone from the community label Jun 27, 2026
@svcnvidia-nemo-ci svcnvidia-nemo-ci added the waiting-on-maintainers Waiting on maintainers to respond label Jun 29, 2026
@marta-sd
marta-sd self-requested a review July 16, 2026 07:19

@marta-sd marta-sd 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.

Thanks you for your contribution @fallintoplace !

I added one small comment. Also please sign your commits as described in contributing guideliness so we can accept your PR :)

Comment thread nemo_gym/rollout_collection.py
@marta-sd

Copy link
Copy Markdown
Contributor

/ok to test f55751e

@svcnvidia-nemo-ci svcnvidia-nemo-ci added waiting-on-customer Waiting on the original author to respond and removed waiting-on-maintainers Waiting on maintainers to respond labels Jul 16, 2026
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
@fallintoplace
fallintoplace force-pushed the fix/aggregate-only-persisted-rollouts branch from f55751e to 22ea6ac Compare July 16, 2026 12:00
@fallintoplace

Copy link
Copy Markdown
Contributor Author

Somehow the CI fails. I can have a look later today.

@sephmard sephmard linked an issue Jul 24, 2026 that may be closed by this pull request
5 tasks
@marta-sd
marta-sd merged commit 8c1af2c into NVIDIA-NeMo:main Jul 24, 2026
24 of 26 checks passed
@svcnvidia-nemo-ci svcnvidia-nemo-ci removed the waiting-on-customer Waiting on the original author to respond label Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request Issue reported or requested by someone from the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants