[framework] fix: emit reward=0 trajectory for failed validation sessions - #152
Draft
RichardFido wants to merge 1 commit into
Draft
[framework] fix: emit reward=0 trajectory for failed validation sessions#152RichardFido wants to merge 1 commit into
RichardFido wants to merge 1 commit into
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
RichardFido
marked this pull request as draft
August 25, 2026 13:51
A failed session (runner exception / timeout / sandbox failure) previously called abort_session and vanished from the TransferQueue. Downstream reward/mean averages only non-aborted sequences (``aborted_mask = response_length == 0`` in metric_utils), so a failed session silently dropped from the denominator and inflated the reported reward — masking real regressions. Replace abort-and-drop with: abort the live session (discard its partial output — a failed rollout's tokens have undefined correctness), then write a minimal reward=0 placeholder (1 prompt + 1 response token, response_mask=0, finished=False). The mask keeps it out of the policy/value gradient (train/critic); its non-zero response_length keeps it counted as a zero in the val reward/mean denominator. Uniform across val and train so critic and val stay consistent; CancelledError still takes the pure abort path above (it is a BaseException, caught before this branch). Test: a crashed val/train session now lands a masked reward=0 dummy (materialization_reason="session_failed") in TQ alongside the successful session's real trajectory, and the uid stays tagged finished on partial failure so the val scorer still reads it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Failed validation sessions (runner exception / timeout / sandbox failure) previously called abort_session, producing no trajectory in the val TransferQueue. reward/mean@1 averages only non-aborted sequences (aborted_mask = response_length == 0 in verl/trainer/ppo/metric_utils.py), so a failed session that had produced partial tokens vanished from the denominator and inflated the reported val reward. This PR finalizes such sessions instead, emitting their partial output as reward=0 / finished=False trajectories so they count as zeros in the val mean. Train-side failure handling is unchanged.
Changes
Thread partition_id through _run_session_with_concurrency_limit → _run_session so the failure branch can tell val from train.
For val-only failures, replace abort_session with _finalize_failed_validation_session: finalize whatever the gateway already produced, rewrite each trajectory to reward_score=0.0 + reward_info={..., "reward": 0.0, "finished": False}, write to the val TQ.
Apply _select_session_trajectories(..., trajectory_selection) before writing, mirroring the success path — a length-split multi-chain session writes the same trajectory count a success would (the val mean is per-sequence).
Merge (not overwrite) reward_info to preserve runner-posted diagnostics (acc, materialization_reason); only reward/finished are forced.
Best-effort throughout: finalize_session and the TQ write are each try/except-wrapped and never mask the original failure; CancelledError is a BaseException, caught by the earlier except so cancellation still aborts.
PR title
Use
[area] type: summary.agents,framework,gateway,logging,sandbox,tasks,tools,training,app,docs,examples,ci,build,deps,miscfeat,fix,refactor,perf,test,docs,chore,revert[agents, sandbox] feat: add isolated harness execution[BREAKING]:[BREAKING][tasks, docs] refactor: replace task config schema[1/N]:[1/N][gateway] refactor: split protocol adaptersChecklist
pre-commit run --all-files --show-diff-on-failurepasses.