fix(tasks): read a timeout off the message prefix, not by substring - #142
Merged
Conversation
Six tasks decided `timeouts` with `"timeout" in msg.lower()`. The code-eval
service documents a small closed set of message shapes, but every one of them
interpolates the failing program's own output after the colon -- so the
substring test also charged these to the clock:
failed: [TimeoutError] deadline exceeded the program raised
failed: [ValueError] timeout must be positive
failed: output ['timeout'] != expect ['ok'] LiveCodeBench, wrong answer
failed [build exit 1]: error: no member named 'timeout' in 'Config'
`timeouts` is the one bucket that says the program RAN and was merely slow, so a
misfiled failure sends a reader after efficiency instead of the exception or the
wrong answer in front of them.
Diagnostic only, and that is why it survived six copies: `correct` comes from
the service's boolean, never from this string, so no headline moves and no
metric test could have failed.
The vocabulary now has one home (`_code_eval_msg.py`) because it is one contract
with six readers and it belongs to the service, not to any benchmark -- the same
reason `_sqlite_exec.py` sits there. It is the union across executors, since the
spellings differ by which one answered: `failed: subprocess timeout:`
(`exec_py_code`, `exec_py_test`), `failed: case timeout:` / `compile timeout:`
(LiveCodeBench's per-case budgets), `failed: timeout` (js/ts), `failed: build
timeout` (compiled rows). Each is emitted only by a timeout path, so the union
has no false positives.
`scicode` keeps BOTH readings, deliberately: its `timeouts` sits beside
`memory_errors` and `import_errors`, which read exception CLASS NAMES out of the
message tail on purpose, so a raised `TimeoutError` is in scope there. Only the
word-appearing-anywhere-else case is removed. Narrowing that counter to the wall
alone would redefine someone else's metric, which a bug fix does not get to do.
Two existing fixtures asserted `timeouts == 1` from `"timeout"` and `"Timeout
exceeded"` -- strings `exec_py_code` cannot emit. They would have passed against
any implementation; both now carry the service's real wording, plus a case
proving a raised `TimeoutError` is not counted.
Re-drift is what the survey is for (`test_timeout_bucketing_convention.py`),
built like `test_grading_call_site_convention.py`: it reads the AST rather than a
hand-kept list, forbids the BARE literal only, and counts CALL nodes rather than
occurrences of the name -- an unused import satisfied the first version of that
assertion, which reverse-mutation caught. All three mutants (predicate reverted,
one call site reverted, every reader removed) now fail the suite.
Follow-up to #138, which fixed this in its own `_failure_buckets` and where the
prefixes were first enumerated. When that lands, its bucketing should adopt this
module; the branches touch no common file, so they merge in either order.
6796 tests pass; ruff, ty, both sync checks and all 24 preflight checks clean.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Review follow-up. The prefix set was missing one spelling that the substring test it replaced had caught by accident, so the fix came out stricter than intended in exactly one place. `failed: [CaseTimeout]` is the per-case wall arriving late. `_unsafe_execute` catches `CaseTimeout` per case, but a signal the kernel has already delivered cannot be un-delivered: an alarm firing as a case ends is raised at the next bytecode check, which can land past that `except`. The worker's outer handler then formats it through the generic class-name branch, so a real wall reads `failed: [CaseTimeout] ` instead of `failed: case timeout: 6.0s`. The vendored service names `CaseTimeout` in that handler for precisely this reason. Reachable only where a per-case budget is armed, which is the two LiveCodeBench tasks (both default `timeout_per_case=6.0`); HumanEval, MBPP and SciCode append the test to the program and take the `exec_py_code` path. Still diagnostic only. The name cannot come from submitted code -- it is a service-internal `BaseException` -- so the added prefix carries no false-positive risk. Also from review: - `exec_lang` and `failed: build timeout` are marked forward-looking. Neither is in the vendored service at this commit; both arrive with #138, and the js/ts entry two lines up already carried that caveat. - scicode's comment claimed its `TimeoutError` test was "matched the way `memoryerror` below is". It is not -- the new one anchors on the brackets that `failed: [{type}] ...` always supplies, while the neighbours match bare. The comment now says so. The neighbours are deliberately left alone: bracketing them would narrow `memory_errors` and `import_errors` without a measurement, which is the same scope line this change already draws. - The convention survey's `zip(..., strict=False)` now says why it is not strict: `n` ops against `n + 1` operands, so `strict=True` would raise on every chained compare in the tree. Prose trimmed throughout. The three-line comment repeated verbatim at five identical call sites is gone -- the helper's name and its module carry it -- and the module and test docstrings say the same things in fewer words. Reverse-mutation: dropping the new prefix fails two tests, one of them the pairwise case asserting that `[CaseTimeout]` and `[TimeoutError]` split opposite ways. 6798 tests pass; ruff, ty, both sync checks and preflight clean. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Follow-up to the review thread on `memory_errors` / `import_errors`, left
matching a bare `"memoryerror" in msg` beside the timeout fix. Same defect as
the timeout one: the tail of `failed: [{type}] {e}` is the program's own
output, so `[ValueError] simulated memoryerror path` counted as an OOM.
Bracketing them was the obvious fix and is wrong. The service reports the
CONCRETE class, so a subclass is what actually arrives, and a test for
`[memoryerror]` drops it. Surveyed every exception class reachable from the
SciCode execution image's stack (docker/Dockerfile.scicode -- numpy 1.26.4,
scipy, sympy, matplotlib; no torch, no pyarrow):
memoryerror builtins.MemoryError, numpy's (below) no loss
modulenotfounderror builtins.ModuleNotFoundError no loss
importerror builtins.ImportError, zipimport.ZipImportError
`ZipImportError` is an `ImportError` that a bracketed test would silently stop
counting. numpy is safe for a non-obvious reason worth recording: its
`_ArrayMemoryError` carries `@_display_as_base`, which assigns
`cls.__name__ = cls.__base__.__name__`, so the allocation failure most likely
in a numpy-heavy benchmark reports as a plain `MemoryError` -- checked at the
pinned 1.26.4, not only at the version installed locally.
So the slot is parsed rather than pattern-matched. `exception_class_name`
returns the class the service named, and each counter matches its family with
`endswith`. That drops false positives only: it never stops counting a class
the service actually named, which is what separates this from the metric
change I said would need its own measurement first. `timeouts` now reads the
same way, which makes the parity its comment used to claim untruthfully true.
14 new tests. Reverse-mutation, each direction caught by the test written for
it: reverting to the bare substring fails the tail case, narrowing the slot to
the builtin name fails the subclass case.
6812 tests pass; ruff, ty, both sync checks and preflight clean.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
#138 landed while this branch was open, which makes two of the module's statements false and one of them dangerous. Stale: `exec_lang` and `failed: build timeout` are in the tree now rather than forward-looking, and "no in-tree task sends a non-default `lang`" stopped being true the moment MultiPL-E arrived with 24 of them. Dangerous: MultiPL-E routes `failed: build timeout` to `n_build_errors`, NOT to `timeouts`, and argues for it -- the run never started, and build-versus-run is the split its three keys exist to carry. `is_timeout_message` answers "did the service stop the clock", which for a build wall is yes. Those are different questions, and this branch's own note that MultiPL-E "should adopt this module" reads as an instruction to collapse them. So the module now says which of the two questions it answers, and points at the counter that deliberately disagrees. No behaviour change, and no new test: MultiPL-E already asserts `n_build_errors == 2, timeouts == 0` on that exact message in tests/unit/tasks/multipl_e/test__base.py, so a naive adoption fails there rather than silently moving a bucket. Rebased onto main (#141, #138) in the same push. 7123 tests pass -- the jump from 6812 is MultiPL-E's own suite arriving, not new tests here; ruff, ty, both sync checks and preflight clean. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
ethan-scitix
force-pushed
the
fix/timeout-bucketing
branch
from
September 10, 2026 18:26
24032f9 to
6c2d94d
Compare
Review follow-up, five findings. `CaseTimeout` is forgeable. The module claimed submitted code could not produce the name because the service's class is a `BaseException`. That is why the service's own handler names it explicitly, but it says nothing about what a program may call its own class: `class CaseTimeout(Exception)` raised from a solution is caught by `_unsafe_execute_fn_call`'s `except Exception` and wrapped into `failed: [CaseTimeout] ...`, which `is_timeout_message` accepts. Checked against the service's own helper rather than argued. The prefix stays -- dropping it loses a real wall, the substring test it replaced counted the forgery too, and what the forgery costs is one diagnostic count -- but the note now says what is true. The vocabulary is split into `CODE_EVAL_BUILD_TIMEOUT_PREFIXES` and `CODE_EVAL_RUN_TIMEOUT_PREFIXES`, union unchanged, so MultiPL-E reads the two groups instead of keeping its own copy of two of them. The fused tuple was what blocked it: its three keys split build from run, which `is_timeout_message` deliberately does not, so it could take the predicate only by moving its build walls. No behaviour moves -- none of its 24 languages is Python, so every prefix it gains is unreachable there. Routing them anyway is what keeps one home for the contract, and means a Python row added later lands in `timeouts` rather than silently in `n_execution_errors`. Also: - The prefix tuples are lowercased, and only `[casetimeout]` depends on it, so a caller comparing a raw message passes against the other five and silently drops that one. Documented, and asserted. - scicode's `import_errors` matched `ModuleNotFoundError` by equality where its two siblings match a family. The stated rule is the family, since the service reports the concrete class, so a subclass is what arrives. - The convention test justified its narrowness with scicode's `"[timeouterror]" in msg`, which 6d72735 had already replaced with a class-slot read. The carve-out is kept for the next such counter; the example is now the live one. Reverse-mutation, each caught by a test: the two groups made to overlap, a prefix left uppercased, MultiPL-E routing the run group to `n_build_errors`, and scicode's import family narrowed back to the builtin. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
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
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.
Type
Summary
timeoutscount with"timeout" in msg.lower(). The code-eval service documents a small closed set of message shapes, but each one interpolates the failing program's own output after the colon — so the substring test also charged these to the clock:failed: [TimeoutError] deadline exceeded(the program raised),failed: [ValueError] timeout must be positive,failed: output ['timeout'] != expect ['ok'](LiveCodeBench, a wrong answer),failed [build exit 1]: error: no member named 'timeout' in 'Config'(the compiled-language executor from feat(tasks): add MultiPL-E — HumanEval + MBPP in 24 languages #138).timeoutsis the one bucket that says the program ran and was merely slow, so a misfiled failure sends a reader after efficiency instead of the exception or wrong answer in front of them.correctcomes from the service's boolean, never from this string, so no headline metric moves and no metric test could have failed. Nothing any of the six scored was wrong.sieval/tasks/_code_eval_msg.py— one contract, six readers, owned by the service rather than any benchmark (the same reason_sqlite_exec.pysits there). It is the union across executors, since the spelling depends on which one answered:failed: subprocess timeout:(exec_py_code,exec_py_test),failed: case timeout:/compile timeout:(LiveCodeBench per-case budgets),failed: [CaseTimeout](below),failed: timeout(js/ts),failed: build timeout(exec_lang). The last two reach MultiPL-E rather than this module's readers, which are all Python. Each is emitted only by a timeout path, so the union has no false positives.timeouts." feat(tasks): add MultiPL-E — HumanEval + MBPP in 24 languages #138 merged while this branch was open, which made that distinction load-bearing rather than pedantic: MultiPL-E routesfailed: build timeoutton_build_errors, not totimeouts, and argues for it — the run never started, and build-versus-run is the split its three keys exist to carry.is_timeout_messageanswers the first question, so for a build wall it says yes. The module now states which question it answers and points at the counter that deliberately disagrees. No behaviour change, and no new test:tests/unit/tasks/multipl_e/test__base.pyalready assertsn_build_errors == 2, timeouts == 0on that exact message, so a naive adoption fails there instead of silently moving a bucket.failed: [CaseTimeout]is the per-case wall arriving late:_unsafe_executecatchesCaseTimeoutper case, but a signal the kernel has already delivered cannot be un-delivered, so an alarm firing as a case ends can land past thatexceptand is then formatted by the worker's outer handler through the generic class-name branch. The vendored service namesCaseTimeoutin that handler for exactly this reason. The substring test caught it by accident and the first prefix set dropped it — the one place this change was stricter than what it replaced. Reachable only where a per-case budget is armed, which is the two LiveCodeBench tasks (both defaulttimeout_per_case=6.0); the other four append the test to the program and take theexec_py_codepath. Service-internalBaseException, so submitted code cannot forge the name and the added prefix carries no false-positive risk.scicodekeeps both readings, deliberately. Itstimeoutssits besidememory_errorsandimport_errors, which read exception class names on purpose, so a raisedTimeoutErroris in scope there; only the word-appearing-anywhere-else case is removed. Narrowing the counter to the wall alone would redefine someone else's metric, which a bug fix does not get to do.memory_errorsandimport_errorscarried the same defect ([ValueError] simulated memoryerror pathcounted as an OOM), so they are fixed with it. Bracketing them would have been wrong: the service reports the concrete class, so a subclass is what arrives, and"[memoryerror]" in msgsilently stops counting it. Surveyed every exception class reachable from the execution image's stack (docker/Dockerfile.scicode— numpy 1.26.4, scipy, sympy, matplotlib; no torch, no pyarrow) andzipimport.ZipImportErroris a live loss under bracketing. Soexception_class_name()parses the slot and each counter matches its family withendswith— which drops false positives only, and never stops counting a class the service actually named. That is what keeps it a bug fix rather than a metric change owing its own measurement._ArrayMemoryErrorcarries@_display_as_base, which assignscls.__name__ = cls.__base__.__name__, so the allocation failure most likely in a numpy-heavy benchmark reports as a plainMemoryError. Checked at the pinned 1.26.4, not only at the locally installed version.Related Issues
Follow-up to #138 (now merged), which fixed this in its own
_failure_bucketsand is where the prefixes were first enumerated. This branch is rebased onto it. If its bucketing later adopts this module, it should take the vocabulary (CODE_EVAL_TIMEOUT_PREFIXES) and keep its own build-versus-run routing — swapping inis_timeout_messagewholesale would move build walls intotimeouts, which is the one thing that split exists to prevent.Test Plan
Automated
ruff check && ruff format --check)ty check)scripts/check_preflight.py— no failures (check_linksskipped at the default level);sync_meta_index.py --checkandsync_package_stubs.py --checkboth cleanManual
No model run: this changes a diagnostic counter, not a score. Verified by construction instead.
vendor/code-evaluator/app/exec_*.py— six timeout spellings and six near-misses. Each near-miss contains the word, so the whole file would pass trivially against a substring test; rejecting them is what the assertions are for.failed: [CaseTimeout]andfailed: [TimeoutError] ...differ only by which class and must split opposite ways — the service's own wall versus the program's own exception. They are asserted as a pair, because getting that backwards in either direction is the whole defect: one way misfiles a wrong answer as slow, the other loses a real wall."timeout"and"Timeout exceeded", whereexec_py_codesaysfailed: subprocess timeout: 3.0s. They would have passed against any implementation. Both now carry the real wording, plus a new case proving a raisedTimeoutErroris not counted.test_timeout_bucketing_convention.py), built liketest_grading_call_site_convention.py: reads the AST rather than a hand-kept list, forbids the bare literal only (so scicode's deliberate"[timeouterror]" in msgstays legal), and resolves the task tree from the imported package so a worktree cannot survey the primary checkout.endswithdesign. The two failure modes are pinned by a test each, in opposite directions.[CaseTimeout]prefix dropped (2),memory_errorsreverted to the bare substring (1, the tail case),memory_errorsnarrowed to the builtin name (1, the subclass case). The third was not caught by the first version of the reader assertion — it grepped for the name, and a leftoverimportline satisfied it — so it now countsast.Callnodes.Checklist
Required (all PRs)
AI-Generated Code - Claude Opus 5 (1M context) (Anthropic)in module docstringcore/—core/untouchedIf: New or Modified Benchmark
Not a benchmark change. No task's
score,pass@1ordenominator_policymoves — onlytimeouts, plus scicode'smemory_errors/import_errors, and only where each was previously wrong.If: Breaking Change
Not breaking.
timeoutswill read lower on any future run whose failures happened to quote the word, which is the fix rather than a regression; historicalreport.jsonfiles are untouched. The[CaseTimeout]prefix moves it the other way, on the two LiveCodeBench tasks only, and only for a wall that was always a wall.If: New Dependency
None.