Fail loudly on partial instance failures - #327
Conversation
compute_task_metrics built an error summary from the failed instances, logged it as a warning, and then never assigned it to the TaskResult it returned, so a task that lost most of its instances still came back clean. finalize_task had the same gap. Attach the summary to the TaskResult, and record saved-vs-processed instance accounting alongside it so the gap between what was dispatched and what was scored is visible in the task summary line, the results summary table and metrics.json. On top of that accounting, mark a task failed when its hard-failure rate exceeds max_hard_failure_rate (HarnessConfig, default 0.05) and fail the run once results have been written, so the artifacts survive for diagnosis. A hard failure is an instance that returned an error and no outputs at all; soft failures that carry a usable output still score, as before.
Seeded from the reproduction of 27B attempt-1 (experiment 01M0VW7G0WRRN1452DXCWMW8W6): a stub provider that is healthy but rejects a subset of requests with a per-request 400, driven through the real process_chat_request path. Covers 47/63 hard failures, 100% failures (the issue #310 shape), a clean run, soft failures that must keep scoring, the threshold boundary (exactly at, just under, just over the 0.05 default) and threshold configurability, plus what the operator sees: the task completion line, the results summary table and metrics.json.
Follow-ups from review of the partial-failure reporting change: - Register max_hard_failure_rate in HARNESS_CONFIG_FIELDS. The documented `--harness ... -o max_hard_failure_rate=` knob was rejected by the CLI's override validation before it ever reached HarnessConfig. Also document the field in README's HarnessConfig table. - Carry the accounting into the stored task row. A gated task used to persist as empty metrics with no instance counts and no reason; StoredTaskResult and the task_results table now carry instances_processed, instances_failed and error_summary, with a migration for the new columns. - Record num_instances for failed tasks too, so metrics.json cannot report num_instances 0 beside instances_saved 16. Metrics themselves stay unpublished for a failed task: a score computed on a subset is not the task's score. - Say which failed task a suite average excluded. The exclusion is deliberate, but it silently changes what the average covers, so name the suite, the task and the reason once per excluded task. - Give the hard-failure rule one home: is_hard_failure() in results.py, used by process_results and available to tests instead of being restated. - Remove the hard-failure gate from finalize_task. The async runner calls it only for trackers that failed before any instance ran, so the gate there was unreachable; the reachable task-level error path keeps its error summary and reports no instance counts, since nothing was dispatched. - Report "-" for instance counts in the completion line when nothing was processed, matching the summary table. - Delete TaskTracker.get_error_summary and the stale RunnerResultsMixin copy of _report_task_completion, both dead. The write-then-gate order moves into AsyncEvalRunner._finalize_and_gate so the invariant (results on disk before the run is failed) is a named seam with a test.
- Drive the documented -o knob through the real CLI path: argv through reconstruct_ordered_args and process_ordered_args, then the same to_dict/_apply_dotlist_overrides/from_dict wiring RunConfigBuilder uses, asserting the value arrives as a float and not the raw string. - Assert the gate fires only after results are saved: a recorded _finalize_and_save proves ordering, so deleting or reordering the call fails. - Assert a gated task's score is absent from results["summary"] while its accounting survives, and that a clean task's score is still published. - Classify hard failures with the production predicate instead of a copy. - Cover the stored task row, the suite-exclusion log line, num_instances agreeing with instances_saved, the finalize_task task-level error path, and the "-" fallback agreeing between the completion line and the table.
finalize_task's task-level error branches set num_instances, the saved count, to the task's full instance count, contradicting the comment right above them and persisting an arithmetically impossible row: instances_saved 63 next to instances_processed 0, in metrics.json and in task_results. A task that failed before dispatch saved nothing, so report zero.
yilunzhao
changed the title
| error_summary=summary, | ||
| instances_processed=total_instances, | ||
| instances_failed=instances_failed, | ||
| hard_failure_rate_exceeded=gate_error is not None, |
There was a problem hiding this comment.
An all-failed task can still exit successfully when max_hard_failure_rate=1.0. In that case, gate_error is None, so the result has an error but hard_failure_rate_exceeded=False. Since check_hard_failure_gate() only checks that flag, the CLI exits 0 even though the task produced no metrics.
We should either require max_hard_failure_rate < 1.0 or always fail the run when no responses were scored. It would also be worth validating the configured range so values like 5 don’t accidentally disable the gate.
There was a problem hiding this comment.
Good catch — fixed by splitting the gate into two conditions: the rate budget still governs partial loss (so 1.0 keeps tolerating it), and a task with instances processed but zero saved now fails on its own condition at any threshold, since there is no rate at which an empty result is worth publishing. Docs and README updated; tests cover all-failed at 1.0 (loud), partial at 1.0 (quiet), all-failed at default (loud).
| num_instances=task_data.get("num_instances"), | ||
| instances_processed=task_data.get("instances_processed"), | ||
| instances_failed=task_data.get("instances_failed"), | ||
| error_summary=task_data.get("error_summary") or task_data.get("error"), |
There was a problem hiding this comment.
This drops the gate error whenever error_summary is present. For example, Postgres stores 47 instances failed (first: boom) but loses Hard failure rate 74.6% exceeds the maximum of 5.0%.
There isn’t another error or status field on the stored task row, so database consumers can’t tell that the threshold was exceeded. Could we persist the task error separately, or combine it with the instance summary here?
There was a problem hiding this comment.
Fixed — the stored row now combines both into the single field, classification first: "Hard failure rate 74.6% exceeds the maximum of 5.0% | 47 instances failed (first: ...)". Identical halves aren't duplicated. Storage test pins the combined form.
max_hard_failure_rate=1.0 read as "never fail on instance failures", so an all-failed task returned no gate error, left hard_failure_rate_exceeded False, and exited 0 despite producing no metrics at all. Separate the two conditions. The rate budget still governs partial loss, so 1.0 keeps tolerating it. A task that saved zero instances now fails on its own condition with its own message, because there is no rate at which an empty result is worth publishing.
The stored row has one text field and the fallback preferred error_summary, so Postgres kept "47 instances failed (first: ...)" and dropped "Hard failure rate 74.6% exceeds the maximum of 5.0%" - the classification that says why the task was failed, with no other status field to recover it from. Combine instead of preferring: task error first, then the summary, so consumers see both. Identical values are not duplicated. Tests for both review fixes land in this commit: all-failed at threshold 1.0 is loud, partial-failed at 1.0 stays quiet, all-failed at the default stays loud, and the stored row pins the combined error form.
undfined
left a comment
undfined
left a comment
There was a problem hiding this comment.
lgtm! note that you'll need to run the migrations to get the updated schema. Docs should cover this but ping me if you have trouble.
|
Thanks — we'll run the migration before pointing the updated code at the shared database. |
2 participants
Fixes the reporting half of #310: a task can lose most of its instances and still report a clean, scored, exit-0 run.
We hit this on a real run: a healthy vLLM server rejected 47 of 63 instances one by one (agentic loops outgrew the context window, each request 400'd), and the run still logged
Processed 63/63, printed a green Success row scored on the surviving 16, and exited 0. The only trace of the 47 dead instances was one WARNING line deep in the log —compute_task_metricsbuilds an error summary from failed instances and then never attaches it, nothing compares saved against processed, and nothing fails a run on its failure rate.Changes
TaskResult.error_summaryis now set (a separate field — settingerrorwould drop the task's metrics entirely).instances_processedandinstances_failedjoin the existing saved count, and show up in the completion line, a newInstancescolumn in the results table,metrics.json, and the stored task row.max_hard_failure_rate(default 0.05) the task is marked failed and the run exits 1 — after results are written, sometrics.jsonand predictions stay on disk. Separately, a task that saves zero instances fails at any threshold: it produced no metrics, so there is nothing to publish. Hard failure = error with no output; soft failures (e.g.MaxTurnsExceededwith a fallback answer) still score, as before.Notes for reviewers
max_hard_failure_rate=1.0(YAML or-o) to stop failing runs on partial instance loss; a task that saves zero instances still fails, since it produced no metrics. The accounting is recorded either way.task_resultstable gains three columns via a migration that does not auto-run: applymake db-upgradebefore rolling out this code, or inserts will fail.Out of scope
max_tokensclamp (vllm_server: max_tokens beyond model context makes every request fail, run still reports Success with 0.0 #310's other half) — this PR only makes those failures impossible to miss.Tests
New suite drives a healthy provider stub that 400s a fixed subset of instances through the real request path: the incident shape (47/63) and the 100% case both turn loud, clean runs stay green, threshold boundaries and the CLI override are covered, and a mutation-style test pins the write-then-gate ordering. Full unit suite, lint, and type checks pass; the migration round-trips against a real Postgres with columns matching the ORM.