Repository navigation
feat(genrm_compare): drop failed judge comparisons; fail the group only when a rollout has none left - #5330
feat(genrm_compare): drop failed judge comparisons; fail the group only when a rollout has none left#5330yfw wants to merge 6 commits into
Conversation
…ly when a rollout has none left Since #3351 one failed judge comparison (no completed answer, judge HTTP error or request deadline) fails the whole cohort, so all of its rollouts are masked even when every other comparison succeeded. With 16 rollouts and a long judge answer, one comparison that exhausts max_output_tokens on both attempts discards 15 good comparisons. Drop each comparison that raises a JudgeError and score every rollout from the comparisons it has left; nothing is masked per rollout. As soon as a rollout has no comparison left that could succeed, fail the cohort as a judge failure exactly as before (every member masked through the judge failsafe) and cancel the comparisons still running, so an outage still ends the group early. Under circular comparisons only two adjacent failures leave a rollout uncovered. Non-judge errors still fail the group with 503, malformed completed answers keep the default-score fallback, and cohort_evaluation_timeout_s still bounds the group. The fixed-baseline path and /compare follow the same rule; /compare leaves dropped comparisons out of comparison_results. There is no configuration key. Report genrm_failed_comparison_rate_per_group and log each drop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test f6d207d |
…_pairs - Log a dropped comparison when it fails, not after the loop. A later failure, a non-judge error or the group deadline can end the group first, and the group error carries only the last failure's text. - README: a connection error or a non-JSON judge reply is dropped too. A request deadline drops a comparison only if it fires before the group deadline, and by default the group deadline fires first. HTTP 503 applies to /verify. - README: name the position effect of a dropped circular comparison. - Test all_pairs, where each rollout has three comparisons, and test that an earlier failure is logged when the group fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test a66283d |
…comparison A failed judge comparison is still dropped. A rollout whose comparisons all failed is now masked alone through the judge failsafe (reward 0.0, mask_sample, failure_kind judge_failed), and its siblings keep their scores. Before, it failed the whole group. The masked rollout is left out of the group's length and style adjustments. When every rollout is left uncovered, the whole group fails as a judge failure, as before. - The server now waits for every comparison to finish or fail, within cohort_evaluation_timeout_s. A non-judge error still fails the group at once and cancels the comparisons still running. - The fixed_baseline path follows the same rule. /compare has no per-response mask, so it still returns 503 when a response keeps no comparison. - Checkpoints save and restore a member masked alone (masked_alone). - New metric: genrm_masked_rollout_rate_per_group. The masked rollout's reward is a placeholder, so a trainer must keep it out of the group statistics: in NeMo RL, grpo.masked_reward_policy exclude_infra (NVIDIA-NeMo/RL#4968) or exclude. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test d94066c |
A failed judge comparison now masks its two rollouts, and every other rollout is scored from all of its comparisons. Before, the two rollouts were scored from their remaining comparison. In a group-relative trainer that mixed rewards averaged over two comparisons with rewards from one, and a judge that favors one position moved the two scores in opposite directions. - A masked rollout's successful comparisons still score its neighbors. The group's length and style adjustments see only the scored rollouts. - The same rule covers all_pairs, several judge passes and the fixed_baseline path: a rollout with any failed comparison or pass is masked. The whole group fails only when every rollout is masked. - /compare has no per-response mask, so any failed comparison returns 503 there, as on main. - The masked-member reason now reads "GenRM rollout_index=N lost a comparison to a judge failure (k of n comparisons failed): ...". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test 6aeff06 |
…d answer A resume regenerates a masked member's answer. The completed group now answers it with the recorded judge failure, as a failed group does, instead of HTTP 409, which aborted collection by default. Also: when every rollout is masked, the group failure says so; the README names env.should_mask_flagged_samples in the consumer requirement and restores the /compare sentence; the _collect_comparisons return type holds JudgeError; tests check that a failed comparison is logged before a later error ends the group, and that a neighbor of a masked rollout keeps both comparisons. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test 9ee8593 |
Resolve the import conflict in tests/test_http_lifecycle.py: use main's new module paths and import JudgeError from nemo_gym.verification. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yi-Fu Wu <yifu.wu@gmail.com>
|
/ok to test 1f4ccf8 |
There was a problem hiding this comment.
Thanks for working on this, @yfw! I left one inline comment about returning promptly after a judge failure in /compare.
I also have an earlier performance PR, #3625, that touches some of the same scoring and checkpoint code. It already incorporates main’s newer failure reporting, so I’d recommend landing that first, then rebasing this PR and rechecking the combined changes. I think that would make the overlap easier to reconcile.
Would that order work for you, or would you prefer a different approach? Happy to coordinate. We’d still keep NVIDIA-NeMo/RL#4968 ahead of this PR, as you’ve documented.
| if not isinstance(error, JudgeError): | ||
| raise error | ||
| position = positions[task] |
There was a problem hiding this comment.
[P2] Preserve fail-fast behavior for /compare
Could we keep prompt failure/cancellation for the batch /compare path while retaining partial collection for /verify? This loop now records JudgeError and waits for the remaining comparisons, but _run_compare rejects the entire batch if any comparison failed. One immediate failure plus a slow judge call therefore delays the 503 until that call finishes; previously the remaining calls were cancelled promptly. /compare also has no enclosing cohort timeout. Please preserve the batch behavior and add a regression test with one failing and one pending judge call.
What does this PR do?
Since #3351, one failed judge comparison fails the whole cohort. A failed comparison is one with no completed answer, or a judge HTTP request that fails (an HTTP error, a connection error or a reply that is not JSON, after any retries). All of the cohort's rollouts are then masked, even when every other comparison succeeded. With 16 rollouts and a long judge answer, one comparison that exhausts
max_output_tokenson both attempts discards 15 good comparisons.This PR masks only the two rollouts of a failed comparison, and scores every other rollout from all of its comparisons. There is no new configuration key.
JudgeErrorare masked throughjudge_failsafe(reward 0.0,mask_sample,failure_kind: judge_failed). Undercircular, one failed comparison (5, 6) masks rollouts 5 and 6, not all 16. Underall_pairs, or with several judge passes, a rollout with any failed comparison is masked.cohort_evaluation_timeout_s./verifygroup with HTTP 503 at once, and cancels the comparisons still running.cohort_evaluation_timeout_sstill bounds the group. Ajudge_request_timeout_sexpiry fails a comparison only if it comes before that deadline; both default to 1,800 seconds, so by default the group deadline fires first and the group fails./comparehas no per-response mask, so a failed comparison still returns 503 there.fixed_baselinepath follows the same rule: a rollout with a failed judge pass against the baseline is masked.masked_alonein its saved state); its siblings replay their rewards.--resumethat reruns a masked row records it as masked again, instead of getting HTTP 409 and stopping the collection.genrm_failed_comparison_rate_per_groupandgenrm_masked_rollout_rate_per_groupmetrics. Each failed comparison is logged when it fails, and each masked rollout is logged with its reason.circular, the comparison that is left also shows one rollout only asresponse_1and the other only asresponse_2, so a judge that favors one position moves their scores in opposite directions.Consumer requirement, and merge order. A masked rollout carries a placeholder reward of 0.0. A trainer must keep it out of the group's baseline and normalization, or it shifts its siblings' advantages. In NeMo RL that is
grpo.masked_reward_policy: exclude_infra(NVIDIA-NeMo/RL#4968;judge_failedis not a policy failure kind) orexclude. Under NeMo RL's default,include, the placeholder counts. NeMo RL also needsenv.should_mask_flagged_sampleson, its default; with it off, NeMo RL trains the masked rows on their 0.0 rewards. So this PR should merge after NVIDIA-NeMo/RL#4968, and GenRM recipes should setexclude_infrawhen they move to a Gym pin that has it.Validation
Against the final diff (rebased on
5911d628c), Python 3.13.14:New tests in
tests/test_failed_comparisons.py,tests/test_checkpoint.pyandtests/test_http_lifecycle.py:all_pairs: one failed pair masks its two rollouts, and the other two are scored from all three comparisons; when every rollout is masked, the group fails./compare: a failed comparison returns 503./verify./runand the failsafe, 16 rollouts: one failed comparison, and two adjacent failures, mask exactly their rollouts withjudge_failed; the other rollouts return their rewards.main's server code, 16 of these 20 test cases fail. The other 4 cover behaviour that does not change.Rollouts
This PR's arm ran at
6aeff06e4. The next commit,9ee859305, changes only the reply to a regenerated answer for a masked member, the reason text when every rollout is masked, docs and tests; scoring and masking are the same. The merge ofmainafter it changes only import paths. Themainarm ran at5911d628c(the base of this PR) in an earlier run, with the same judge, policy, data and settings.Setup
nvidia/Qwen3-Nemotron-235B-A22B-GenRM, served by vLLM 0.27.1 with tensor parallel size 4 on one GB200 node.Qwen/Qwen3.5-4Bwith thinking off, served by vLLM on one GPU of a second node.resources_servers/genrm_compare/data/example.jsonl. Each run holds one task with a new_ng_group_id, so it makes one 16-rollout circular group.genrm_comparesettings, with two changes that cause real judge failures:genrm_responses_create_params.max_output_tokens: 1341. We first ran 16 judge comparisons with a 16,384-token budget. The answers were 531 to 2,981 tokens long, and 1,341 is their 55th percentile. So some answers run out of budget before the verdict.genrm_parse_retries: 1, so each comparison gets 2 attempts.Commands. Each arm starts its servers once, and they stay up for all 4 tasks.
genrm_5330_run.yaml
genrm_compare_resources_serverandgenrm_simple_agentare the same as inconfigs/genrm_compare.yaml, withmax_output_tokens: ${genrm_max_output_tokens}. The two model servers point at the external vLLM servers:Results (one 16-rollout group per task and arm)
6aeff06e4)main(5911d628c)"Rates" are
genrm_failed_comparison_rate_per_groupandgenrm_masked_rollout_rate_per_group, as reported on each scored row.This PR scored 48 of the 64 rollouts, and
mainscored 16. On this PR, the masked rollouts are exactly the rollouts of the failed comparisons, and no group failed as a whole. On both arms, every masked row hasmask_sample: true,failure_kind: judge_failedand reward 0.0, and lands in the collector's_failures.jsonlsidecar.The two arms ran separately, and each sampled its own policy and judge answers. Tasks 1, 3 and 4 had failed comparisons on both arms, but not the same ones. On
main, a group fails at its first failed comparison, so its log shows one failed comparison per masked group.Server log lines
This PR, task 4 (one failed comparison, two rollouts masked):
main, task 1 (one failed comparison, group failed):Compatibility and benchmark impact
exclude_infrafrom feat(grpo): add exclude_infra masked-reward policy for infra-masked rollouts RL#4968, orexclude). Merge after feat(grpo): add exclude_infra masked-reward policy for infra-masked rollouts RL#4968./compare. Unchanged: a failed comparison returns 503./verify, andmasked_alonein a checkpointed member's state.Checklist
resources_servers/genrm_compare/README.md).pre-commit run --all-files).git commit -s).🤖 Generated with Claude Code