fix(lcb-service): distinguish infra crashes from submission failures in grading children - #433
fix(lcb-service): distinguish infra crashes from submission failures in grading children#433liayan wants to merge 16 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #433 +/- ##
=======================================
Coverage ? 80.52%
=======================================
Files ? 152
Lines ? 20720
Branches ? 0
=======================================
Hits ? 16685
Misses ? 4035
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e9aa1db to
4ff0df4
Compare
|
Verified on the Python 3.14 lcb-service image (forkserver default): grading works with the fork pin; dead grading children classify as -6, and the all-infra-errors check raises, so the original failure is still caught; an all-timeout batch scores 0 without raising — that case would have raised on commit one fix. |
47115a5 to
9c6739e
Compare
9c6739e to
3dfa3ca
Compare
83a5f2f to
3dfa3ca
Compare
nv-alicheng
left a comment
nv-alicheng
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Reviewed by: Claude + Code-Quality (×2: diff + import-neighborhood, per request) | Depth: quick + forced code-quality
codex was unavailable in this environment. See the summary comment for neighborhood findings on _server.py/run_lcb_tests.py and untouched-line items that can't be posted inline.
Review Council — Multi-AI Code ReviewReviewed by: Claude + Code-Quality (run twice — diff scope + import-neighborhood, per request) | Depth: quick with code-quality forced on (normally skipped at quick depth) Tight, well-reasoned fix. One high-severity correctness gap survives it, plus code-quality/neighborhood items you asked for. Existing bot/human threads corroborated but not duplicated (see bottom). 🔴 Must Fix (high)
🟡 Should Fix (medium)
🔵 Consider (low)
Existing threads (corroborated, not duplicated)
|
|
One thing to note - I think this is a bug on my part: I'd used py3.14 as the base container for the lcb_runner, but maybe this also should be studied: From the lcb_runner repo, they use py3.11 (https://github.com/LiveCodeBench/LiveCodeBench) I have personally not checked but there might be some variance between python versions (i.e. version specific language features like walrus operator, same-line multi-context managers, etc.), and the code generated by the worker should be either version-agnostic or catered to a specific version. This can get solved a little more easily if we just pin to a python 3.11 container for lcb-service. |
3.14's forkserver default is what exposed this, but rolling back to 3.11 would just hide it — fork-from-thread, the -6/-8 attribution, and the SystemExit handling are all version-independent bugs that'd still exist, so I don't see much upside to reverting now. |
7f4d040 to
750f3aa
Compare
|
Kindly ping. |
Lets also revert to 3.11 to keep it consistent with LCB specification. We can do that separately. |
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Reviewers: Codex (gpt-5.6-sol) + Claude (concurrency + accuracy lenses) + Grok 4.5 (Cursor). Event: comment only (no approve/reject). Findings verified across models; a couple of single-model claims were dropped after checking (e.g. an "open Manager() makes the fork worker multi-threaded" claim — empirically the worker stays single-threaded, so the spawn-pool / fork-child split is sound).
This PR has iterated well: the earlier SystemExit→-7, outer-pool fork→spawn, Manager-leak→with+join, return-types, and DRY threads are all resolved — not re-raised here.
Posted inline (still open)
| Line | Sev | Finding |
|---|---|---|
| 117 | high | started_flag flips before judge-side setup → a judge-setup crash is mislabeled -8 and escapes the all-infra guard → silent pass@1=0 (mirror of the 83db6ed fix; boundary one frame too high). |
| 213 | high | The regression test the PR body claims does not exist anywhere in tests/. |
| 400 | med | All-infra guard is narrow: -8/-1 excluded, a single non-infra result disables it, and LCB's -4 "Error during testing" (run_lcb_tests.py:521/541) carries no "error" key so it's never classified/logged. |
| 371 | med | warning→error floods ERROR with routine timeouts/submission failures, drowning genuine -5/-6. |
Lower priority (not posted inline)
- Lazy imports in
execute_code_single(numpy,run_lcb_tests) — hoisting to module scope also shrinks the line-117 window (two birds). - PR description drift: the body says fork is used for the "outer process pool," but the code uses
spawnthere (fork only for the grading child) — please update. - Document the invariant that the
forkchild is safe only while the pool worker is single-threaded, so a future top-level import that spawns a thread doesn't silently reintroduce a fork+lock deadlock. started_flag'sValuecan belock=False(parent reads only post-join); the per-sampleManagercould be aPipe/SimpleQueue— hardening only.
Filed separately
- A pre-existing accuracy bug found during this review — an empty test suite scores every submission as PASS (
all([]) == True) — is tracked in #443 (out of scope for this PR's diff).
Orthogonal (existing thread)
The Python-3.11 discussion: pinning would only mask the (real, version-independent) MP bugs, so keep these fixes — but grading-interpreter parity with upstream LCB is a separate result-validity concern worth its own tracking, not a substitute.
89dbccb to
f866eb7
Compare
|
@liayan can you add more details on how to reproduce the failures. I have tried to run LCB with py3.11 and py3.14 base images on x86/linux and unable to see any failures without the PR. Can you share the steps to reproduce the |
Good catch — I originally hit this on vera-rubin with a new lcb-service image on 3.14, but also confirmed it reproduces on GB200/GB300. Just haven't checked on x86 yet. Just reproduced it again Steps:
Grading children dead as zombies under a nested tree of forkserver helper processes — same shape as the original 0/349-for-3.5h, one defunct child per pool worker. |
|
Interesting — same exact steps on x86_64 (same Python 3.14, same code): finishes in ~1s, every time, at every scale I tried (up to 349 samples / 176 workers). So this looks architecture-specific rather than Python-only-specific. |
a9547a5 to
7058663
Compare
Python 3.14 changed the default multiprocessing start method on Linux from fork to forkserver. The grading pipeline (pool workers forking a per-problem mp.Process + mp.Manager) only works with fork: under forkserver the grading children die at startup, every result comes back as an error, and execute_code_single_suppressed_errors turns that into all-failed tests, so the service sits at 0/N forever. Pin the fork context for the executor, the per-problem Process and its Manager. Also raise if every subprocess reported an execution error -- that means the judge is broken, not that all samples failed -- and log those errors at error level instead of warning. Seen on a python 3.14 lcb-service image: 0/349 after 3.5h, one defunct child per pool worker. Same inputs with fork forced: done in 6 min. The repo pins 3.12 so CI won't hit this, but shipped images have.
Timeouts were counted as execution errors, so a small batch where every submission loops forever would trip the guard and raise instead of scoring 0. Split the empty-buffer case in run_code_subprocess: child still alive at the deadline -> timeout (-1, submission's fault), child exited without reporting -> new GradingChildDied (-6, judge's fault). The guard now only counts -5/-6, so the forkserver startup deaths still raise and all-timeout batches score normally. Also log the multiprocessing start method at service init; that would have made the original 0/N a one-line diagnosis.
…failure sys.exit() is a BaseException, not caught by the existing `except Exception`. grade_call_based's method invocation has no SystemExit guard (unlike the stdio path's call_method, which already does), so a call-based submission calling sys.exit() killed the grading child before it filled resp_buffer and got misclassified as -6 GradingChildDied - an infra error that can trip the all-errors guard even for a single-sample batch. Give it its own code (-7) instead, kept out of _LCB_INFRA_ERROR_CODES.
evaluate() runs on an executor thread (the server dispatches it via run_in_executor), so the per-request ProcessPoolExecutor was forking an already-multithreaded process - a known deadlock risk: only the forking thread survives in the child, locks held by other threads stay locked forever. Switch the pool to spawn: fork+exec inherits no locks, so it is safe to start from a thread, and everything submitted to the pool is picklable, so it is a drop-in. Tried forkserver first, but its helper hangs at pool shutdown in the lcb-service container (Python 3.14.5) and leaks semaphores. Probed all three start methods in the deployment image: fork and spawn tear down cleanly, forkserver hangs indefinitely. The inner grading child keeps fork: grading relies on fork semantics, and forking from a freshly exec'd single-threaded pool worker is fine. The startup log now prints both start methods.
A submission can kill its own grading child in ways no except block sees (os._exit(), a native segfault, an OOM kill). That landed in the -6 GradingChildDied bucket and counted as an infrastructure error, so a batch where every submission crashed its interpreter tripped the all-infra-errors guard and aborted instead of reporting a legitimate 0 score. The child now sets a shared started flag right before grading begins, so an empty resp_buffer can be attributed: died before the flag means a judge startup failure - still -6, still counted by the guard; died after means the submission killed the interpreter - new -8 SubmissionKilledChild, scored as a normal failed sample. Exit codes cannot make this distinction because os._exit() lets the submission pick any code.
…process Return the reported result early, hoist the shared all-failed res out of the attribution branches, and keep only the metadata construction per branch. No behavior change.
Each graded sample created a Manager (its own server process) and relied on the GC finalizer to shut it down. Make the lifecycle explicit with a with-block so the process count under load is bounded deterministically, capture the child's started/exitcode state before the scope closes, and reap a killed grading child with join() instead of leaving a zombie in the pool worker.
execute_code_single_suppressed_errors is the fork target, but took fully untyped variadics, so a miswired argument only failed at runtime inside the grading child (surfacing as a spurious child-death error). Give it named parameters, and declare the tuple[list, dict] return type on all three grading helpers so the res/metadata unpacking is checked.
Should've gone out with the earlier attribution-fix commits -- had this written already, just missed staging it at the time. Covers timeout (-1), sys.exit (-7), os._exit (-8), and judge-side deaths (-6, both before run_test and during its own setup), plus the all-infra guard: an os._exit()-only batch scores 0 without tripping it, a judge-startup-death batch does.
started_flag flipped True at wrapper entry, before run_test's own setup (reliability_guard, suite parse) ran -- a death in that window got misattributed as -8 SubmissionKilledChild instead of -6 GradingChildDied, so a real judge bug could sneak past the all-infra guard as a silent 0. Moved the flag into run_test itself, set right before dispatch to grade_call_based/grade_stdio -- the actual first line of the submission's own code. Hoisted the numpy/run_lcb_tests imports to module scope while in there too (same window, and it'd been flagged as a lazy import anyway); costs the grading child nothing since it forks from an already-warm pool worker.
…e logging Two gaps in the all-infra guard/logging: the outer except in grade_call_based/grade_stdio's callers returns -4 with no "error" key, so a bad submission that fails to compile or define the expected function never gets logged (the classification gate keys on "error" in metadata) -- give it one. Left -4 out of _LCB_INFRA_ERROR_CODES on purpose: it's reached whenever the submission's own code fails to compile, which is the common case and is plainly not the judge's fault, not some rare harness bug worth aborting a whole run over. Also split logger.error into error (infra codes) vs warning (everything else) -- it was firing for every timeout/sys.exit/os._exit/bad-code sample, which drowns the real -5/-6 signal in ops on any batch with a few slow or broken submissions.
…low-up PR Reviewer flagged a perf concern with pinning the pool to spawn; dropping the explicit context here and following up with a separate PR + perf numbers. Everything else (SystemExit, started_flag attribution, Manager with-block, type annotations, logging levels) stays as-is. Verified in a python:3.11-slim container (< 3.14, defaults to fork) — all 11 regression tests pass.
fdf2882 to
c14c3c2
Compare
github-actions
Bot
added
the
size/normal
liayan
changed the title
|
Discussed offline with @arekay-nv — we're moving the explicit spawn/fork context pinning to a separate follow-up PR with more performance data, to avoid a perf regression on the spawn side. Everything else through c14c3c2 (SystemExit handling, started_flag attribution, Manager with-block, type annotations, logging levels) stays as-is here. Verified in a python:3.11-slim container (Python < 3.14, defaults to fork) — all 11 regression tests pass. Also opened #457 to pin the base image back to Python 3.11, mirroring LiveCodeBench's own supported version. Could you take a look and merge that one too — it's still blocking lcb-service on aarch64 without the new spawn soluton/fork solution. |
CI's full test run already forces the process-wide multiprocessing default to spawn (endpoint_client/worker.py sets it at import time), breaking these tests' fork-only monkeypatch assumptions. Force fork locally in the tests that need it instead of relying on the ambient default.
Hey @arekay-nv, could you give this another review and approve it if everything looks good? I’d also appreciate your help merging it, since I don’t have write permissions. I opened #457 as well for consistency. |
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
Review council — follow-up pass (round 2)
Ran an independent multi-model council (Codex gpt-5.6-sol @ xhigh + two Claude review lenses) over the current HEAD (7038582). Codex found no actionable regressions. The attribution core verified correct: the started_flag boundary is placed right (after reliability_guard() / suite-parse, before grade_*), the three no-result branches are mutually exclusive and each reachable, SystemExit→-7 handling is correct, and the with mp.Manager() switch fixes a pre-existing per-sample Manager-process leak.
New findings posted inline. My earlier round-1 comments (incl. the all-infra guard narrowness at line 403) still stand and are not re-raised here.
| Sev | Where | Issue |
|---|---|---|
| medium | run_lcb_tests.py:530 |
Malformed ground-truth outputs → -4 (submission) while malformed suite → -5 (infra): split attribution, dataset faults can silently report 0 |
| medium | lcb_serve.py:362 |
-5 path and the guard's mixed-batch == boundary untested |
| low | run_lcb_tests.py:558 |
stdio / grade_stdio branch untested |
| low | lcb_serve.py:164 |
mp.Value default lock=True — latent deadlock + per-sample semaphore; use lock=False |
| low | test_lcb_serve.py:124 |
Docstrings narrate dev-history / reference an unmerged "follow-up PR" (AGENTS.md) |
Additional low-severity, not posted inline:
lcb_serve.py:383—all([])isTrue, so an empty/degenerate suite (LCBTestLoader(strict=False)returnsinputs: []) scores a free pass; guard withbool(res) and all(...). Pre-existing.lcb_serve.py:360—future.result()is unguarded; aBrokenProcessPool, or a raise insiderun_code_subprocess(malformed-suitejson.loads, Manager spawn failure), bypasses the guard with an opaque crash. Pre-existing.- Fork-only fault injection —
-6/-8attribution is exercised only under forcedfork; theforkserver/spawnmethod that caused the production incident isn't directly tested. Worth confirming the fork-pinned tests pass on Python 3.14 / macOS (fork-after-threads). - Docs — the new
-5..-8taxonomy and the refuse-to-report-0RuntimeErroraren't documented inlivecodebench/README.mdordocs/evaluation/DESIGN.md.
🤖 Generated by an AI review council (Codex + Claude); posted for the author's consideration.
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
Looks good. Thanks for putting this together and addressing the review comments.
There are just a final few ones - please address them before merging.
…e flag Malformed suite JSON already landed as -5 TestRunnerError, but malformed ground-truth outputs were parsed one level down inside grade_call_based and landed as -4 (submission's fault) instead. Move that parsing next to the suite parse in run_test so both dataset problems get the same -5 attribution. Also: drop the lock on started_flag (single writer, single reader after join, no contention to guard against), and cover the -5 path, the stdio branch, and a mixed infra/non-infra batch, none of which had a test. Signed-off-by: Liang Yan <lyan@coreweave.com>
|
All follow-up items from the new round review are addressed in a442550 — see replies on each thread. Ran the full tests/unit/evaluation/ suite on both B300 and GB300 hardware; no regression. @arekay-nv the new commit landed but seems still need another reviewer approval per requrement, could you help merge it directly or should I ping other reviewers here. |
|
@liayan sure will do - can you update the PR description as well - it still seems to reference the fork vs spawn changes. |
6 participants
What
Distinguishes judge-side (infra) failures from submission-side failures when
a LiveCodeBench grading child produces no result, instead of scoring
everything as a submission failure:
started_flag,flipped in
run_testright after judge setup (reliability_guard, suiteparse) and before the submission's own code runs, tells the parent whether
the child died during the judge's setup (infra) or while running the
submission (submission's fault).
sys.exit()in submitted code is aBaseException,so it previously fell through the child's
except Exceptionand gotmisclassified as
GradingChildDied. Caught separately now.TestRunnerErrorand -6GradingChildDiedare bothjudge-side; a batch where every future is infra-attributed now raises
instead of silently reporting a 0 score.
failure, parsed alongside the suite parse in
run_testinstead of beingfolded into the submission's -4 inside
grade_call_based.mp.Manager()is used in awith-block and the killed child isjoin()ed, so a killed grading child no longer lingers as a zombie orleaks a manager process.
mp.Valueuseslock=False(singlewriter/reader, no lock needed).
This PR does not touch the multiprocessing start method (fork/spawn) —
that's split out into #457 (pinning the base image back to Python 3.11) to
avoid conflating a perf-sensitive change with this one.
Type of change
Testing
pytest -m unit— full suite passes, no regressionstests/unit/evaluation/test_lcb_serve.py(new): covers every attributionpath — -1/-5/-6/-7/-8, the all-infra guard (including a mixed batch that
must not trip it), both call-based and stdio, malformed ground truth
tests/unit/evaluation/suite (314 tests) passes on bothChecklist
mis-attributed 0-result path)