fix(genrm): isolate prompt cohort retries - #2814
Conversation
github-actions
Bot
added
the
sla:triage-overdue
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
7f93814 to
3615823
Compare
github-actions
Bot
removed
the
sla:triage-overdue
|
/ok to test 3615823 |
github-actions
Bot
added
the
sla:review-overdue
Preserve group identity, exact-response reattachment and completed reward replay from #2814. Add finite collection, evaluation and judge-request deadlines, cancellation cleanup, and explicit group identity migration. Keep the standard agent error boundary and verify saved collector failures over HTTP. Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
3d897f5 to
1c605e9
Compare
|
/ok to test 1c605e9 |
|
/ok to test 01aae17 |
@macandro96, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 01aae17 |
Yes - propagating transport failures and adding an evaluation deadline are deferred to #3351. Will update PR description |
|
Could retaining completed cohorts change repeated-run behavior for legacy callers without Previously, successful completion removed the cohort, allowing another run of the same task/prompt on the same server. With retained completed cohorts, fresh answers appear to reuse the old identity and receive HTTP 409 until the entry expires or is evicted. Is that intentional? One possible solution is to preserve the previous behavior for legacy callers by retiring successfully completed cohorts, while keeping cached-result replay and duplicate protection for callers supplying explicit group IDs. Legacy callers would still lack reliable isolation for overlapping runs and late retries, but ordinary sequential reruns would continue working. Would that fit the intended backward-compatibility contract? |
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test c2e884f |
Yes, that fits the intended backward-compatibility contract. Addressed in c2e884f: completed legacy cohorts without Ideally, we can remove the completed cohort for new path too. But for now - we can apply the change for legacy paths as for the new path there will not be any collision (and it is removed after ttl_s). |
yuhezhang-ai
left a comment
yuhezhang-ai
left a comment
There was a problem hiding this comment.
Thanks for addressing the legacy rerun case. Verified c2e884f: sequential legacy runs work, explicit-ID replay remains intact, and all 81 GenRM tests pass. Leaving final merge judgment to @ananthsub; deadline and failure-cleanup follow-ups remain in #3351.
Thanks for preserving the legacy behavior. For explicit-ID cohorts, I’d keep the bounded cache for now, since it lets callers recover rewards after losing a response without judging again. Removing it would change that replay behavior. |
Add finite collection, evaluation and judge-request deadlines, drain owned work on failure or shutdown, and reject judge transport failures without publishing ordinary rewards. Preserve merged legacy sequential reruns and explicit-ID replay, keeping group constants local to the resource server. Cover collector failure artifacts and repeated legacy runs over real HTTP. Accept null judge status and make malformed-answer fallback independent of retry order. Document caller-owned group recovery and bounded retention. Co-authored-by: Anish Mahishi <amahishi@nvidia.com> Co-authored-by: Jiacheng Xu <jiachengx@nvidia.com> Co-authored-by: Teodor-Dumitru Ene <teodord.ene@gmail.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
) This follows merged NVIDIA-NeMo#2814, which established group, attempt, and member identities. It addresses NVIDIA-NeMo#3181's immediate timeout and failure-cleanup requirements. - **While collecting answers:** enforce a finite deadline. If a member never arrives, fail the group and release waiting requests without publishing partial rewards. - **Once all answers are available:** retry transient judge HTTP failures, including interrupted response bodies, within a bounded budget using the same answers. - **If judging ultimately fails:** preserve each generated answer and return an explicitly masked failure. The zero reward is a placeholder, excluded from Gym evaluation metrics by default. - **On failure, cancellation, or shutdown:** release waiters and clean up pending work. ### Recovery and compatibility A judge failure returns HTTP 200 with `mask_sample: true`, `failure_kind: judge_failed`, a bounded `failure_reason`, and the existing failure-sidecar tags. The shared judge failsafe also preserves `instance_config.mask_sample: true` for older RL consumers. This shared response fix applies to every environment using `judge_failsafe`. The existing evaluation opt-in `count_failure_classes_as_zero: [judge_failed]` still includes selected failures as zeros. Only the metric-input copy is unmasked; saved failure records and training masks remain unchanged. Online and offline aggregation apply the same policy, including when every rollout fails. With the opt-in, an all-`judge_failed` run reports `mean/reward: 0.0`; without eligible counted failures or successful results, collection still raises. The main rollout JSONL never receives synthetic successes. **A masked HTTP 200 result does not automatically make RL retry the group.** It is a completed, unusable result for the current RL caller. If the caller chooses replacement: - **Explicit-ID groups:** advance the shared group-attempt number for every member and submit a complete replacement group. - **Legacy groups without an ID:** failed groups retain a failure record. Reusing their task/prompt key returns failure with migration guidance; recovery requires a fresh explicit group ID for the complete group. This prevents delayed old members from joining a replacement. Successful legacy groups still allow sequential new evaluations. The judge-request default is now **1,800 seconds**, matching the overall evaluation budget. This allows substantially more room for long generations and queueing than the previous 300-second default; deployments must still size both limits for their workload. **Required RL coordination:** before NeMo RL adopts this Gym revision, coordinate with [RL #4160](NVIDIA-NeMo/RL#4160) / [#4061](NVIDIA-NeMo/RL#4061). Reading the mask alone is insufficient: TransferQueue must preserve it in the training loss mask, and unavailable judge rewards must be excluded from group statistics. #4160 includes these fixes with `masked_reward_policy: exclude` as the default. Top-level field ingestion remains separate migration work, so the nested compatibility field remains. Before updating RL's Gym revision, validate a real failed Gym response through RL's loss mask and group statistics. Gym can merge first if RL keeps its existing Gym revision until those fixes are available. GenRM-specific usage and recovery documentation is now in the [resources-server README](https://github.com/NVIDIA-NeMo/Gym/blob/yuhez/3181-genrm-cohort-lifecycle/resources_servers/genrm_compare/README.md#genrm-comparison-groups), rather than a new Fern site page. Closes NVIDIA-NeMo#3181. <details> <summary>Validation</summary> Current code: `6a7f30b707aee62c46478d757005f3fcc9fabe0d`, incorporating main at `d4c0f86c7`. The latest fix is `059adb409`. - **5,110 core, GenRM, and OSWorld dependency-policy tests passed** on Python 3.13.14: `python -m pytest tests/unit_tests resources_servers/genrm_compare/tests responses_api_agents/osworld_agent/tests/test_dependency_policy.py -m 'not sandbox' -q --tb=short`. There were 103 skips, 673 sandbox tests deselected, and 184 passing subtests. Sandbox tests were not run locally. - The all-failure regression checks one and four masked judge failures, zero counting enabled/disabled, an unrelated counted class, and deferred aggregation. Four cases fail on the preceding code and pass with this fix; default and wrong-class error behavior remains intact. Existing mixed-success tests remain covered. - Actual TCP collector → SimpleAgent → GenRM tests check both collection timeout and judge outage, with zero counting on/off. Online and offline metrics match, the failure sidecar remains byte-for-byte unchanged, and the main/merged rollout JSONL stays empty. - Final all-files pre-commit, whitespace checks, and DCO sign-off passed. [Exact-head CI](https://github.com/NVIDIA-NeMo/Gym/actions/runs/35867697700) passed, including core and all eight server shards. The preceding run passed core and seven server shards but failed OSWorld dependency resolution; merged NVIDIA-NeMo#3649 fixes that NumPy metadata conflict and is included in this head. [Previous-head CI](https://github.com/NVIDIA-NeMo/Gym/actions/runs/35681811360) passed on `be7e6353d`, including core and all eight server shards. - No new model run for the latest collector aggregation fix: it changes metric eligibility, not policy generation or judging. The earlier real-model evidence below is from `be7e6353d`. - **Real-model functional validation passed:** Slurm job **19084624**, exit 0 in **14m58s**, on `be7e6353d`. Gym's native CLI/HTTP path used Qwen3-0.6B policy and Qwen3-Nemotron-235B-A22B-GenRM-2603 on eight H100s. Eight policy generations and four completed real judge calls were captured. - The run preserved four masked failure answers during an injected judge outage, then scored a caller-coordinated complete replacement attempt. Four HTTP 503s and four interrupted HTTP response bodies recovered within the retry budget without extra policy generation. A second collector resume made no further model calls. Failed legacy groups rejected delayed members, explicit zero-count aggregation preserved saved masks, and rewards exactly matched the captured judgments. Saved artifacts were independently inspected. - The model run enabled `CUDA_LAUNCH_BLOCKING=1` for diagnostics after an earlier CUDA failure. That failure did not recur; this does not establish its root cause. The run's functional assertions passed before the separate launcher teardown issue described below. - No full RL training, checkpoint restart, or comparative throughput claim. </details> <details> <summary>Limits and follow-ups</summary> The caller coordinates complete replacement attempts. There is no partial scoring, automatic group scheduling, durable answer reuse, or change to generation/judge overlap. Legacy identities cannot isolate overlapping runs or provide completed replay. Explicit-ID replay and failure records are process-local and bounded by retention; eviction or restart is not a recovery protocol. Ordinary individual reverification is unsupported. The existing `judge_failed_only` guard bypass does not create groups or advance attempts: retained failures remain failures until the caller supplies a complete replacement with the required identity. Active-group memory limits, retention optimization, caller-minted group identities, durable outcomes, richer telemetry, and improvements to retry backoff remain follow-up work. The inherited judge-failed-only mode-guard bypass needs a separate compatibility change; documenting it does not make individual GenRM reverification supported. A Ray teardown abort was previously reproduced on both main and an earlier PR head; its native cause remains undiagnosed. In the passing job 19084624, the Gym launcher again exited with SIGABRT after the functional assertions completed. The harness stopped the remaining owned children and recorded no survivors; the separate policy server exited normally and Slurm released the allocation. This validates the GenRM failure/recovery behavior, not a clean Ray launcher shutdown. </details> ### Related PRs: what is covered and what could follow Built on NVIDIA-NeMo#2814 (Anish Mahishi), with deadline and cleanup requirements from NVIDIA-NeMo#2385 (Teodor-Dumitru Ene) and NVIDIA-NeMo#2903 (Jiacheng Xu). Contributor credit remains in commit trailers. **NVIDIA-NeMo#2385:** the timeout requirement is covered. Its partial-scoring policy is not adopted: if A/B/C arrive but D is missing, this PR fails the incomplete group. Whole-group failure is allowed by NVIDIA-NeMo#3181. Close NVIDIA-NeMo#2385 as superseded if that policy is accepted; otherwise retain/rework partial scoring as separate follow-up work with explicit missing-member and training/metric semantics. **NVIDIA-NeMo#2903:** together, NVIDIA-NeMo#2814 and this PR cover request deduplication, duplicate-disconnect safety, and failure cleanup. Three policy differences remain: - NVIDIA-NeMo#2903 permits a fresh group under the same identity after failure. Here, replacement requires a newer shared explicit attempt, or a fresh explicit ID when migrating a failed legacy group. - NVIDIA-NeMo#2903 keeps completed rewards for the server's lifetime, including legacy groups. Here, completed replay is bounded and restricted to identical responses in explicit-ID groups; legacy groups have no completed replay. - NVIDIA-NeMo#2903 starts comparing available answer pairs while other answers are still generating. Here, we preserve NVIDIA-NeMo#2814's wait-for-all schedule. Each judge call needs only two answers, so early judging remains a possible optimization; it does not require partial reward publication, and its performance benefit has not been benchmarked. After this PR merges, maintainers can close NVIDIA-NeMo#2903 as superseded if these choices are accepted, tracking desired early judging or broader replay/retry behavior separately. Alternatively, rework the existing PR against current main to focus on the remaining scope. Closing either older PR means its core problem is addressed and remaining choices are accepted or tracked, not that every proposed behavior was merged. --------- Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> Co-authored-by: Anish Mahishi <amahishi@nvidia.com> Co-authored-by: Jiacheng Xu <jiachengx@nvidia.com> Co-authored-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
… in the judge failure (NVIDIA-NeMo#3303) ## What does this PR do? Names GenRM output-budget exhaustion in the judge failure instead of reporting it as a generic "no completed answer", in `resources_servers/genrm_compare`. No behaviour change: same retries, same `JudgeError`, only the message and a per-attempt warning. ## Why When the GenRM model spends its whole `max_output_tokens` on reasoning, the model server returns HTTP 200 with a complete envelope and no verdict: `status: "incomplete"`, `incomplete_details.reason: "max_output_tokens"`. Since NVIDIA-NeMo#2814 and NVIDIA-NeMo#3351 the retry path already handles this correctly (retry, then `JudgeError`), but the error reads the same as an empty or garbled answer, so an operator cannot tell a budget problem from a transport or parsing one without opening the raw response. Measured against a hosted GenRM endpoint with the candidates delivered as `response_1`/`response_2` roles, temperature 0.6, same input each time: | `max_output_tokens` | runs | verdict (`status: completed`) | exhausted (`status: incomplete`, `reason: max_output_tokens`, no verdict) | |---|---|---|---| | 16,384 | 5 | 2 | 3 | | 24,576 | 4 | 2 | 2 | The outcome is bimodal: the same input either converges at roughly 8 to 10k output tokens or runs away and consumes whatever budget it is given (4 of 4 still exhausted at 49,152), so the fix belongs with the judge's prompt or reasoning budget, and the message now points there. ## How - `_output_budget_exhausted(raw_response)`: true when the Responses object is `incomplete` with reason `max_output_tokens`. - Each such attempt logs `GenRM output budget exhausted for pair (i, j) (attempt a/n, max_output_tokens=N)`. - When all attempts fail without a completed answer, the existing `JudgeError` message gains `(k of them exhausted max_output_tokens=N without emitting a verdict; raise the budget or constrain the judge's reasoning)`. Completed-but-empty answers keep the unchanged generic message. - Tests: detection helper, all attempts exhausted -> message names the budget, exhausted then verdict -> normal parse, empty completed answer -> generic message. `resources_servers/genrm_compare/tests`: 35 passed. Per review, the earlier opt-in budget escalation (`genrm_budget_exhausted_retry_multiplier`, `genrm_max_output_tokens_cap`) is dropped. ## Checklist - [x] I have read the [contributing guidelines](https://docs.nvidia.com/nemo/gym/latest/contribute/development-setup). - [x] The change is focused; unrelated "drive-by" edits are tracked as separate issues/PRs. - [x] Tests added or updated and pass locally (`resources_servers/genrm_compare/tests`, 35 passed). - [x] Pre-commit checks pass locally (`pre-commit run --files ...`: all Passed). - [x] All commits have DCO sign-off (`git commit -s`). Signed-off-by: Balaji Ganesan <bganesan@nvidia.com>
This branch was successfully deployed
5 participants
Summary
Fix GenRM comparison cohorts so physical retries cannot add duplicate logical siblings or mix responses from different prompt-group attempts.
Previously, GenRM buffered requests in an append-only list keyed primarily by task/prompt identity. A retry could therefore append another physical response for an existing logical sibling, causing the cohort to exceed
num_rollouts_per_promptor map rewards to the wrong response.This change introduces explicit logical cohort identity:
_ng_group_id: stable identity for one logical prompt group._ng_group_attempt: physical attempt of the entire prompt group._ng_rollout_index: logical sibling slot within the group.GenRM cohorts are now keyed by:
Members within a cohort are keyed by:
Flow
A conflicting response for an already occupied
rollout_index, a stale group attempt, or inconsistent prompt content is rejected rather than silently creating another cohort member.Changes
rollout_index.(group_id, group_attempt).group_idand reject stale requests.rollout_index, independent of HTTP arrival order._ng_group_id,_ng_group_attempt,_ng_task_index, and_ng_rollout_indexthrough the sharedrollout-collection response boundary.
Current failure behavior and follow-up
This PR preserves the existing GenRM judge fallback behavior. Judge transport
errors and exhausted output-parsing retries caught by
_run_single_comparison()return the configured default scores. The cohort istherefore completed with fallback rewards rather than failed with HTTP 503.
A cohort enters the failed state and releases its waiters with HTTP 503 only
when an error escapes
_run_compare(), evaluation is cancelled, or cohortcollection times out.
cohort_collection_timeout_soptionally bounds the time spent waiting for allcohort members. This PR does not add an evaluation deadline or a per-judge HTTP
deadline.
Follow-up #3351 adds bounded collection, evaluation, and judge-request
deadlines; propagates judge transport failures as cohort failures; and expands
failure cleanup and shutdown handling. It may also require an explicit,
run-unique
_ng_group_idfor multi-member verification. Those behavioralchanges are intentionally outside the scope of this PR.
Backward compatibility
Legacy requests without
_ng_group_idcontinue using task/prompt-based cohort identity.When
_ng_group_idis supplied but_ng_group_attemptis omitted, the request is treated as attempt0and emits a migration warning.The cohort registry remains process-local. GenRM comparison should continue using one HTTP worker until this state is moved to shared storage.
Configuration
Adds:
cohort_collection_timeout_scohort_result_ttl_smax_terminal_cohortsCollection timeout is optional so long-running tool rollouts are not expired by default.
Checklist
pre-commit run --all-files) (so CI lint/format/copyright pass).git commit -s) (so the DCO check passes).