feat(swe-bench): recover conservatively from infrastructure failures - #482
Open
leopck wants to merge 7 commits into
Open
feat(swe-bench): recover conservatively from infrastructure failures#482leopck wants to merge 7 commits into
leopck wants to merge 7 commits into
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
github-actions
Bot
added
the
size/large
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
leopck
changed the title
leopck
force-pushed
the
swe-dist-3-reaper-guards
branch
from
August 27, 2026 15:25
2889689 to
919e30f
Compare
leopck
force-pushed
the
swe-dist-3-reaper-guards
branch
from
August 27, 2026 15:31
919e30f to
b223099
Compare
Builds on "give Pyxis container creation its own deadline", which attached srun's own output to these failures and made them *readable*. They are still not *actionable*: nothing in the text distinguishes "the step never launched" from "the command ran and its report was lost", and only the first can be retried without risking double execution. Measured signature, from an isolated probe with no model and no GPU (20 nodes, 200 workers, 6273 ordinary shell steps): 63 steps failed, and in all 63 the status file still read `pending` -- not `started` -- with srun's output empty. The step script never ran its first line. The command provably did not execute, so re-running it cannot double-apply an edit, a removal or a test run. That is the entire safety argument, and it is only available if the status bytes are captured rather than compared and thrown away. Two changes: * `StepNotLaunched(RunnerError)` records `srun_rc`, the observed `status`, and `provable_non_execution` (`status == "pending"` and no sentinel). It subclasses `RunnerError`, so every existing `except RunnerError` is unaffected, and the status bytes now appear in the message too. * An in-band sentinel, `__MLPERF_STEP_RC__ <nonce> <rc>`, becomes the primary result channel. It travels on srun's stdout, so a step reports its outcome without depending on a readable shared filesystem -- a real failure mode of its own on a distributed one -- and it is what makes "no sentinel" half of the provability test. The nonce makes it unforgeable by the command's own output; it is stripped before the output is returned. This deliberately stops at reporting. Nothing here retries. Tests: `test_step_failure_reports_whether_non_execution_is_provable` covers the three decision points (pending / started / finished), `test_step_reports_its_return_code_in_band`, `test_step_sentinel_cannot_be_forged_by_command_output`, `test_read_step_sentinel_ignores_unrelated_output` and `test_step_not_launched_is_a_runner_error`. Against the parent commit the raised error has no `provable_non_execution`, no `srun_rc`, no `status`, and does not quote the status bytes.
reaper.py releases a stale claim only when it has no result, its heartbeat is past stale_after, AND its owner is provably gone. Liveness is a pluggable protocol: LocalProcessLiveness pairs pid with boot id so a recycled pid on a rebooted host is not read as a live owner, and SlurmStepLiveness treats a step missing from scontrol inside a live job as dead, because the job-level rule alone deadlocks the queue forever. An indeterminate probe releases NOTHING - a false reap creates two owners, duplicate results and a wrong denominator. guards.py kills a runaway graded test only under a full conjunction (RSS over threshold AND cwd inside the testbed AND a container-supervisor ancestor). Kills are by PID and refuse self and any ancestor of self; there is no pattern-kill path in the module at all, and a test greps the source to keep it that way. Each term reports its evidence count, and HealthVerdict.combine returns INDETERMINATE rather than UNHEALTHY when a term has zero evidence, so a conjunctive guard cannot collapse into its weakest clause.
… is proved The reaper returns a claim whose owner is provably gone. This is the same argument one level down: an operation that provably never ran can be run again, and one that may have run cannot. Measured signature, from an isolated probe with no model and no GPU (20 nodes, 200 workers, 6273 ordinary shell steps): 63 steps failed, and in all 63 the step's status file still read `pending` -- not `started` -- with no in-band sentinel. The step script never executed its first line. Re-running those commands cannot double-apply an edit, a removal or a test run. That is the entire safety argument, and it is why the gate is `provable_non_execution` rather than "an error happened". A failure that does not make that claim is re-raised immediately and does not consume the budget, which also makes every exception type this module has never heard of safe by default. `infra_retry` provides three things: * `retry_on_provable_non_execution(...)` -- bounded attempts, and the gate. The evidence is read as an attribute rather than an isinstance check, so the producer of the evidence and this consumer stay decoupled. * `InfraRetryLedger` -- every attempt and its outcome, appended as JSONL so a run that dies still leaves its retry history behind, and held in memory so the counters survive a ledger that cannot be written. Accounting must never be able to take a run down. * `summary()` -- `infra_retries_total`, `instances_saved_by_retry`, `infra_retries_exhausted`, the succeeded-on-attempt distribution, and `run_quality: CLEAN | OK_WITH_RETRIES | DEGRADED`. The counting is the point. The banked campaign retried environment faults without limit and without counting them (`wq_worker.sh:41` WQ_MAX_ATTEMPTS=5, `:256` "ENVIRONMENT FAULTS DO NOT CONSUME THE UNIT'S ATTEMPT BUDGET"), which is exactly why nobody knew how many there had been. A retry loop that quietly absorbs the defect it compensates for turns a broken cluster into an invisible one. Measured effect of adding this loop: RunnerError 59 -> 7 and resolve 47.0% -> 70.0% against a banked 70.67% on the identical 200 instances -- a rescue on that scale is not a clean run, and `run_quality` says so even at 200/200. This is the fleet-side half: the decision rule, the accounting and the quality verdict. The next commit applies the same rule inside the SWE-bench service, where the Pyxis step that produces `provable_non_execution` runs. Tests: `TestTheSafetyGate` covers both sides of the decision boundary (`pending` retried, `started` never retried, unfamiliar exception never retried) plus the bound; `TestAccounting` covers recovery, exhaustion, not-retryable, ledger durability and a ledger that cannot be written; `TestRunQuality` covers all three verdicts including DEGRADED on volume alone, where every operation eventually succeeded.
…aunched Wires the retry decision into the place the failure actually happens. `run_srun_step()` now re-attempts a step only when `StepNotLaunched` reports `provable_non_execution` -- the status file still `pending` and no in-band sentinel, so the step script did not run even its first line and the command definitely did not execute. A `StepNotLaunched` that reached `started`, and every other failure, is raised immediately: re-running work that may already have run can apply an edit twice, delete twice, or double a test run, and none of those announce themselves. Measured signature, from an isolated probe with no model and no GPU (20 nodes, 200 workers, 6273 ordinary shell steps): 63 steps failed and in all 63 the status file still read `pending`. Measured effect of retrying exactly those: `RunnerError` 59 -> 7 and resolve 47.0% -> 70.0% against a banked 70.67% on the identical 200 instances. Bounded by `SWEBENCH_PYXIS_STEP_RETRIES` (default 3, set 1 to disable), and every attempt and outcome is appended to `SWEBENCH_PYXIS_INFRA_RETRY_LOG` when set. The record shape is deliberately identical to `swe_bench_distributed.infra_retry.RetryRecord`, which gains `InfraRetryLedger.from_jsonl()` to read it back and publish `infra_retries_total`, `instances_saved_by_retry`, `infra_retries_exhausted` and `run_quality`. The service is an isolated subproject and must not import the benchmark client, so the two halves share a file format rather than a module -- and a test on each side pins that agreement, because if it breaks the retries stop reaching the run-level quality flag and a rescued run looks clean. A retry loop that quietly absorbs the defect it compensates for turns a broken cluster into an invisible one. That is why the accounting is not optional and why `run_quality` reports DEGRADED on volume alone. Tests: `TestStepRetry` covers both sides of the boundary (`pending` retried, `started` never retried), the bound, the recorded outcomes for recovery and exhaustion, and a log path that cannot be written. `TestReadingBackAWritten Ledger` covers the cross-process format, including a truncated final line from a run that died. An autouse fixture pins the existing single-shot tests to one attempt so they keep asserting single-shot behaviour.
leopck
force-pushed
the
swe-dist-3-reaper-guards
branch
from
August 27, 2026 15:52
b223099 to
c1ebd2d
Compare
leopck
marked this pull request as ready for review
August 27, 2026 19:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds conservative claim reaping, PID-based memory protection, bounded Pyxis container creation, and narrowly scoped infrastructure retries.
Retries occur only when machine-readable evidence proves that execution never started, avoiding accidental reruns of genuine benchmark failures.