feat(eval): persist failures and resume unfinished evaluations - #3302
yuhezhang-ai wants to merge 4 commits into
Conversation
|
🌿 Preview your docs: https://nvidia-preview-pr-3302-5cd78777cadf.docs.buildwithfern.com/nemo/gym |
|
/ok to test 1e89221 |
|
/ok to test f993e61 |
yaoyu-33
added
area:evaluation
Now that NVIDIA-NeMo#3179 landed, `BaseVerifyResponse` can name the kind of failure rather than only whether the sample is usable. The three fields stay orthogonal: `mask_sample` decides usability, `failure_kind` groups, `failure_reason` explains. A degraded but validly measured sample names a kind and stays unmasked. The field validates against `nemo_gym.failure_kinds` and warns rather than rejects. An unregistered name is a migration signal; failing the response over it would replace a visible wrong label with an invisible lost failure. An environment that needs something the shared set should not grow uses `<server>:<kind>`. Run-level reconciliation is no longer here. NVIDIA-NeMo#3302 implements it for NVIDIA-NeMo#2135, using the materialized inventory and attempt journal to account for omitted and unknown work offline — neither of which this contract can see. A second copy would be the duplication this workstream exists to remove. What remains is the per-sample half: the contract field and masked-aware aggregation. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
|
/ok to test 67aa8fc |
Now that NVIDIA-NeMo#3179 landed, `BaseVerifyResponse` can name the kind of failure rather than only whether the sample is usable. The three fields stay orthogonal: `mask_sample` decides usability, `failure_kind` groups, `failure_reason` explains. A degraded but validly measured sample names a kind and stays unmasked. The field validates against `nemo_gym.failure_kinds` and warns rather than rejects. An unregistered name is a migration signal; failing the response over it would replace a visible wrong label with an invisible lost failure. An environment that needs something the shared set should not grow uses `<server>:<kind>`. Run-level reconciliation is no longer here. NVIDIA-NeMo#3302 implements it for NVIDIA-NeMo#2135, using the materialized inventory and attempt journal to account for omitted and unknown work offline — neither of which this contract can see. A second copy would be the duplication this workstream exists to remove. What remains is the per-sample half: the contract field and masked-aware aggregation. Signed-off-by: waple0820 <232305951+waple0820@users.noreply.github.com>
|
/ok to test 9dd9db9 |
| eligible = {logical_rollout_id(row) for row in store.pending(_get_max_rollout_attempts())} | ||
| # Use normalized selected attempts, not raw sidecar lines: legacy | ||
| # import can assign/reassign attempt indices without rewriting payloads. | ||
| selected_rollouts = [row for row in store.failures() if logical_rollout_id(row) in eligible] |
There was a problem hiding this comment.
Consider a saved judge failure whose append reverification is cancelled after the new dispatch is journaled:
attempt 0: judge_failed, saved response available
attempt 1: dispatched, then cancelled before outcome
On restart, store.pending() correctly marks the rollout eligible for attempt 2. However, store.failures() selects payloads only from the latest attempt. Attempt 1 is unknown and has no payload, so the attempt-0 judge failure and its saved response disappear from selected_rollouts.
reproducing this issue, the rerun printed Nothing to be re-verified, made zero new /verify calls, and left coverage at unknown: 1 despite remaining retry budget.
We should retain the latest prior recoverable judge_failed payload as reverification input when the selected attempt is unknown. The older payload should supply only the saved response for a newly allocated attempt; eligibility and terminality should still come from the latest state.
There was a problem hiding this comment.
Thanks @ananthsub — the interrupted judge-retry case is fixed in 786860db3, and I tightened one policy edge for your review:
judge_failed (saved answer A) → unknown → unknown: reuse A in a new judge attempt, subject to the latest retry budget/state.judge_failed (saved answer A) → agent failure or judge failure without an answer → unknown: stop at that newer recorded outcome and skip with a warning. Do not reach past it to reuse A.
This deliberately changes the earlier implementation's broader historical-answer fallback: an interruption alone should not make an older answer eligible again. Judge-only recovery never reruns the agent; users can separately resume collection if they want to generate another answer. Please let me know if you would prefer the broader fallback policy.
I also made Gym's merged-output writes atomic so health checks detect replacement instead of reading rewritten bytes through stale offsets, while preserving permissions and symlink targets.
Validation: 1,192 focused tests passed, including cancellation/restart and no-extra-inference checks; all-files pre-commit and Fern checks passed. The broader core run has 5,212 passes and the same three documented local Hydra help failures. CI has been retriggered for the new head.
| if output_fpath.resolve() in {Path(path).resolve() for path in input_paths}: | ||
| raise ConfigError("Merged output must not overwrite a source rollout artifact or its attempt history.") | ||
| print(f"Merging shards into {output_fpath}") | ||
| with output_fpath.open("wb") as out: |
There was a problem hiding this comment.
Consider an output path that already belongs to a journal-backed run:
combined.jsonl
combined_manifest.json
combined_attempts.jsonl
combined_materialized_inputs.jsonl
combined_failures.jsonl
merge_shards rejects a target that aliases an input shard, but otherwise opens only combined.jsonl with wb. The new merged rows replace that file, while the old manifest, journal, materialized inventory, and failure sidecar remain.
Reproducing this with two valid runs leaves all companions from the old run. A later RolloutStore.read(combined.jsonl) fails because the new rollout IDs are not in the old inventory. The original result file has already been destroyed by then.
We should reject the merge before mutation when any recovery companion exists for the target. Deleting those companions implicitly would discard recovery history, while a merged projection cannot safely inherit one source run's manifest
| ordinal=ordinal, | ||
| source_index=source_index, | ||
| line_number=record.line_number, | ||
| ) |
There was a problem hiding this comment.
Consider a standalone health check that indexes run A, after which a fresh collection reuses the same output path before a process-pool worker opens it:
indexed: rollouts.jsonl, offset 2048, length 4096
path replaced with run B
worker: open("rollouts.jsonl") and read the same range
_LineSlice carries only the pathname and byte range, and _read_record reopens that pathname later. Valid run-B JSON is therefore accepted silently.
A repro indexed tasks [0, 1]; after path replacement, health reported [9, 1] with no parse finding. Automatic post-collection health is unlikely to race, but the standalone command has no immutability or locking requirement.
We should retain the indexed file identity, such as (st_dev, st_ino), and compare it with fstat() after the worker opens the path. Append-only growth can remain valid because it preserves the inode.
|
/ok to test 8fb4ea6 |
github-actions
Bot
removed
the
sla:review-overdue
|
/ok to test 786860d |
|
/ok to test 9623af1 |
|
/ok to test 74c5e7b |
|
Hi @ananthsub, one retry-policy question: the current default limit counts three dispatched attempts, including attempts interrupted by cluster shutdowns and requests still queued locally. Repeated job interruptions can therefore exhaust unfinished tasks without three recorded task failures; increasing Should interruptions continue sharing this limit, or have a separate bounded allowance? I've retained the current behavior and added documentation and regression coverage pending agreement. |
Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
|
/ok to test 5cd7877 |
|
Hi @ananthsub, this is ready for another review at The latest fixes cover running-server configuration checks with Please take another look when you have a chance, including the earlier review threads against the narrowed scope. |
This branch was successfully deployed
4 participants
What does this PR do?
If collection stops after two of three tasks finish, resuming keeps both saved results and retries only the unfinished task from its input. Failures remain separate from measured scores, and reports show how much of the expected evaluation completed.
This is the evaluation-runner portion of #2135, narrowed after the Environment Server design review. It integrates
mainafter the shared failure contract merged in #3877, including #3559, #3427, and #3912. The original full implementation is preserved. Judge-only recovery is draft #3879, and journal-aware health reads are draft #3881; both depend directly on this PR. Execution checkpoint restoration and framework integration remain under #3024.EpisodeFailure(failure_reason) andEpisodeIdwrapper, while preserving legacy sidecar aliases and diagnostic evidence. No reward is invented for a failure.Saved failure wrappers are validated when history is read or written, including agreement between the nested failure and its surrounding run/attempt identity, terminal flag, and routing class. Unsupported versions and unknown fields are rejected even during explicit legacy import. A failure remains a failure when its optional classification is absent; the compatibility sidecar uses a generic source-specific class.
Preserves main's dispatch-budget, longest-first, terminal-timeout opt-in, and invalid-judge migration controls from #3684. Tasks drained before dispatch consume no attempt. Cached-deliverable reuse survives a newer interrupted attempt, but stops at a newer recorded outcome that does not permit reuse. An interrupted sidecar-first invalid-judge migration can be reconciled only when both records match the exact migration; conflicting and foreign-run records remain errors. After validating history, resume repairs incomplete final lines before strict migration reads them.
The supported recovery workflows also retain these guarantees:
--no-serve, identity comes from the running servers' configuration. Model or verifier changes reject resume before dispatch or file mutation; missing required server definitions are errors.execute_only=truecompletions remain unscored, completed, and reusable with failure routing on or off. Present invalid rewards and malformed responses still fail validation.RERUN_INCOMPLETEtoggle does not change experiment identity; model settings and nested task parameters still do.Validation
Collector head:
5cd78777c, based onmainatbf04ab614, using Python 3.13.14 in the dedicated Gym environment.pytest tests/unit_tests/ -m 'not sandbox' -q --tb=short— 6,070 passed, 113 skipped, 718 deselected; 184 subtests passed.pytest tests/unit_tests/test_{rollout_collection,rollout_recovery,rollout_journal,rollout_store,rollout_outcomes,rollout_reverification,rollout_health,aggregate_metrics,cli_eval,cli,episode_types}.py resources_servers/gdpval/tests/test_multistage_orchestrator.py environment_servers/single_agent_turn_legacy/tests --import-mode=importlib -q --tb=short— 1,364 passed.pre-commit run --all-filesandcd fern && npm run check— passed (Fern: zero errors, one warning).Rollouts
Fresh smoke on
5cd78777c: actual cached Qwen2.5-0.5B inference through llama.cpp, Gym's production model adapter, Simple Agent, and FrontierScience judge over HTTP. A controlled protocol producer wraps the real agent outcomes asSingleAgentTurnResponse; the production legacy adapter and collector perform conversion and persistence. The collector has no local server blocks and no injected server-config environment variable: it fetches the authoritative configuration over head-server HTTP, exercising the--no-serveboundary. This tests the metadata and recovery boundaries, not native agent-session lifecycle or live Stirrup sandbox execution.judge_failedanddelivery=delivered, without a fabricated reward or reusableresponsealias. Whole-task resume completed 3/3 with three new policy calls.This checks recovery mechanics, not benchmark accuracy, GPU serving, distributed training, or live execution checkpoint restoration. The small model's answers and judgments were inspected; its scores are not benchmark-quality evidence.
Compatibility and benchmark impact
Structured failures use the canonical
failure_reasonfield required by merged #3877; the formermessagename is no longer accepted. Legacy_ng_failure_*sidecar aliases remain supported.Completed zero rewards, masked results, and native unscored completions remain reusable results. Existing
run_examples()behavior and loose legacy aggregation are retained. Resuming unverified legacy artifacts requires explicitallow_unsafe_resume; corrupt or foreign artifacts remain errors.The retry cap defaults to three dispatched attempts across the saved run, including interrupted attempts. Operators must use one collector per output path; parallel jobs use separate shard paths. This is not an exactly-once remote execution or storage-hardware durability guarantee.
Deliberate intermediate boundary: journal-backed judge-only reverification and health reads are deferred. Their readers reject these histories before modifying files or reporting stale attempts. Existing legacy reverification remains available; health checks can inspect the merged selected-result projection from
gym eval aggregate. Automatic health checks print a short skip message without an error traceback or stopping collection/scoring;+disable_health_check=trueskips them. The follow-up drafts remove these guards with their corresponding integrations. Selected-result projections contain completed answers; they do not export failed answers for judge-only recovery.Source symlinks resolve to the original run and its companion history before selection; aliases of the same input are deduplicated. Recognized runtime connection references may change without invalidating resume, while model and task changes still affect experiment identity. Offline aggregation accepts
retry_terminal_timeoutsto report exhausted attempts under the same explicit retry policy as collection.Previously saved
--no-servehistories whose signature omitted server settings may now reject resume. Start a fresh run, or deliberately accept the existing explicit unsafe override and its unverified-identity reporting.The opt-in
run_outcomes()API honors an explicit run ID and returns invocation-owned row copies without modifying caller inputs. Non-judge producer responses are retained only as_ng_failure_responsediagnostics; they are not scored or eligible for judge-only recovery.The legacy adapter now retains a producer's specific failure kind. Class-based failure summaries and explicit failure-as-zero selection therefore use that supplied kind, rather than the generic environment-failure fallback. This does not make native partial responses eligible for judge-only recovery.
Failures do not lower quality scores by default; coverage makes their absence explicit. Main’s
count_missing_rollouts_as_zeroandcount_failure_classes_as_zeroremain explicit reporting choices. Added zeros never enter result/failure artifacts or turn unfinished tasks into completed work. For journalled runs, missing-zero selection uses the newest dispatched attempt; known failures and intentional unscored/omitted completions are not silently counted as missing. Reports separateimputedfrom measured rewards and counted failures. Changing the missing-zero reporting switch does not invalidate resume.Checklist