Skip to content

fix(eval): re-run infra-retryable verifier-errored tasks on resume - #1063

Open
Benjamin-eecs wants to merge 2 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-resume-rerun-infra-verifier-errors
Open

fix(eval): re-run infra-retryable verifier-errored tasks on resume#1063
Benjamin-eecs wants to merge 2 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-resume-rerun-infra-verifier-errors

Conversation

@Benjamin-eecs

@Benjamin-eecs Benjamin-eecs commented Aug 29, 2026

Copy link
Copy Markdown

Description

On resume, re-run a task whose newest result is scoreless and whose verifier error is infra-retryable per RetryConfig.should_retry_verifier_error (verifier timeout, session transport loss, download failure). Keep reusing contract failures (e.g. No reward file found) and any result that carries rewards.

Motivation and Context

A scoreless infra verifier error records no signal about the task; reusing it on resume pins a lost score forever, and the only recovery is manually deleting result.json. The change reuses the retry taxonomy the within-run retry already trusts, and preserves the PR #819 / issue #542 behavior for contract failures and scored results.

Closes #1059.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds core functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)

Implemented Tasks

  • Filter infra-retryable scoreless verifier errors out of _get_completed_tasks with an explicit re-run log line
  • Split the resume guard test: infra error re-runs, contract error is reused

Checklist

  • My change requires no change to the documentation.
  • I have updated the tests accordingly (tests/test_verify.py: 58 passed; tests/test_job.py: 53 passed).
  • I have checked the code (ruff check src/benchflow/evaluation.py tests/test_verify.py: passed).

Copilot AI lite review requested due to automatic review settings August 29, 2026 07:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment thread src/benchflow/evaluation.py Outdated
Comment on lines +1119 to +1126
if r.get("rewards") is None and (
self._config.retry.should_retry_verifier_error(r["verifier_error"])
):
logger.info(
f"Re-running verifier-errored task on resume: {task} "
f"({truncate_end(r['verifier_error'], 80)})"
)
continue

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.

🔴 Resumed learning tasks run out of order

In sequential-shared resumes, _get_completed_tasks reruns an earlier errored task after later tasks advanced the persisted learner state. The reordered task consumes future skills and corrupts the learning curve.

Prompt for agents
The new resume filtering in src/benchflow/evaluation.py::_get_completed_tasks is safe for parallel-independent jobs but breaks sequential-shared ordering. A sequential run can continue after task B has a retryable verifier error, then complete task C and persist C's learner generation. On resume, B is the only remaining task and runs against C's later learner state. Preserve sequence semantics by either rewinding the learner store and invalidating/rerunning the errored task plus every later task, or by keeping retryable verifier failures terminal for sequential-shared mode. Add a regression test with completed A, retryable-error B, completed C, and a persisted learner snapshot.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed: re-run is gated to parallel-independent; sequential-shared keeps the reuse semantics. Regression test added.

@JeremyJC67

Copy link
Copy Markdown

The boundary is drawn where it should be, and the tests prove it: I verified test_infra_verifier_errored_reruns fails red against main's evaluation.py and passes with the change, verifier crashed: No reward file found still classifies verifier_failure and is reused, and routing the decision through should_retry_verifier_error means resume and within-run retry share one taxonomy — including the retry_on_verifier_infra=false opt-out. tests/test_verify.py (58), tests/test_job.py (53), and tests/test_usage_tracking.py (13) all pass here, ruff clean.

  1. No cap across resumes. The within-run loop already spent max_retries + 1 attempts before that scoreless infra result was written (evaluation.py:1346-1391); every resume now grants a fresh set. A deterministically sick task — say a genuinely slow verifier that hits verifier timed out every run, which the string taxonomy can't distinguish from the exec-layer wedge the way rollout/_setup.py's timeout-with-output check can — re-runs on every resume, forever. That also silently changes resume for existing users: it flips from "idempotent, regenerates the summary for free" to "re-spends agent rollouts". The bound is already in hand: the rglob loop enumerates every scoreless-infra result.json for the task, so counting them and reusing after N attempts (with a "giving up after N" log line) caps the spend. retry_on_verifier_infra=false works as an escape hatch but also kills the within-run retry, so a count (or separate knob) is cleaner.

  2. The motivating Daytona failure doesn't classify as infra. The [BUG] Verifier hardening execs keep a 10s timeout on the scoring path and record rollouts as verifier crashed #1058 failure mode records verifier crashed: Command timed out after 10 seconds (Daytona's bounded-exec message, wrapped at rollout/_setup.py:519), and classify_verifier_error returns verifier_failure for it — I probed the realistic strings and only transport/setup/download markers and verifier timed out make the retryable set. So resume still pins exactly the artifact class that motivated your pair of PRs, including any post-fix(sandbox): use the verifier-setup budget for hardening execs #1062 hang that exceeds 180s. Consider adding a "command timed out after" marker to _looks_like_verifier_infra_error (scoring.py:231-241): a slow verifier surfaces as verifier timed out, never this exec-layer phrasing, so the false-positive risk looks low — and it would let resume repair result.json files damaged before fix(sandbox): use the verifier-setup budget for hardening execs #1062 lands.

  3. An older scored artifact loses to a newer scoreless-infra artifact. Reproduced locally: rollout-1/result.json with reward: 1.0 plus a newer retry artifact with a scoreless verifier timed out_get_completed_tasks returns nothing and the whole task re-runs, despite the description's "keep reusing … any result that carries rewards". Newest-wins is pre-existing and the normal flow protects you (the retry loop stops on a score), but mtime is fragile across job-dir copies (rsync/scp without -a). Preferring any rewards-bearing artifact during collection would make the description literally true; a comment documenting the mtime assumption is the cheaper alternative.

One layout check for the record: matrix trials each get their own job dir (root/alias/trial-NN), so the per-task_name collapse only merges retry artifacts within one job — the filter's granularity is right for the matrix path.

On ordering vs my #1046 (feat/ablate-cli): a trial merge in both directions is clean — my evaluation.py changes (the task_rollout_config extraction around L1203-1295 plus config fields) don't touch _get_completed_tasks, and your filter handles my branch's artifacts correctly: unscored branch-children that raise instead of reporting 0 write neither rewards nor verifier_error, so they simply re-run, same as agent-errored tasks today. Landing order is free — happy to rebase whichever lands second.

@Benjamin-eecs

Copy link
Copy Markdown
Author

Thanks for verifying. Also addressed the sequential-shared concern from the automated review: the re-run is now gated to parallel-independent mode, with a regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Resume reuses scoreless verifier-errored results so infra-failed tasks never re-run

3 participants