fix(lcb-service): use the fork start method explicitly - #433
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #433 +/- ##
=======================================
Coverage ? 81.70%
=======================================
Files ? 146
Lines ? 19343
Branches ? 0
=======================================
Hits ? 15804
Misses ? 3539
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
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.
9c6739e to
3dfa3ca
Compare
83a5f2f to
3dfa3ca
Compare
|
|
||
| with ProcessPoolExecutor(max_workers=self.n_lcb_workers) as executor: | ||
| with ProcessPoolExecutor( | ||
| max_workers=self.n_lcb_workers, mp_context=_MP_CTX |
There was a problem hiding this comment.
High — concurrency: LCBServe.evaluate() is invoked through event_loop.run_in_executor() in _server.py, so this outer ProcessPoolExecutor is created from a worker thread in the already multithreaded uvicorn process. Forcing it to use fork can inherit locks held by threads that disappear in the child, leaving grading workers deadlocked and the evaluation hung. The reported compatibility problem establishes that the inner grading child needs fork semantics, but not that the outer pool does. Please use separate contexts—for example, forkserver for this outer pool and fork only for the Manager/Process inside run_code_subprocess, where the pool worker is single-threaded.
There was a problem hiding this comment.
High — concurrency:
LCBServe.evaluate()is invoked throughevent_loop.run_in_executor()in_server.py, so this outerProcessPoolExecutoris created from a worker thread in the already multithreaded uvicorn process. Forcing it to useforkcan inherit locks held by threads that disappear in the child, leaving grading workers deadlocked and the evaluation hung. The reported compatibility problem establishes that the inner grading child needs fork semantics, but not that the outer pool does. Please use separate contexts—for example,forkserverfor this outer pool andforkonly for theManager/Processinsiderun_code_subprocess, where the pool worker is single-threaded.
Good catch — tried forkserver as suggested, it passed locally but hung at pool shutdown in the lcb-service container (Python 3.14.5, unreaped zombie workers), so I switched the outer pool to spawn instead: worker startup is a bit slower but it's thread-safe for the same reason and all local + container tests pass; the grading child keeps fork. Let me know if you see any other issues.
| # submitted code. | ||
| res = [-1] * len(suite["inputs"]) | ||
| metadata = { | ||
| "error": "Grading subprocess died before reporting a result", |
There was a problem hiding this comment.
Medium — correctness: execute_code_single_suppressed_errors() catches Exception, but SystemExit derives from BaseException. A submitted solution that calls sys.exit() therefore terminates the grading child without filling resp_buffer, and this branch classifies it as GradingChildDied (-6). For a one-sample batch—or when every submission exits—the all-infrastructure-error guard raises RuntimeError instead of recording ordinary failed submissions. This was reproduced through the actual LiveCodeBench path. Please catch SystemExit as a submission runtime failure and account for os._exit, which can bypass exception handling entirely.
There was a problem hiding this comment.
Good catch — I did see SystemExit getting mixed in with GradingChildDied. Turns out it's specifically the call-based (fn_name) grading path, since the stdio path already guards against it internally. Added a SystemExit except guard as suggested (bc502f1), scoring it as its own -7 SubmissionExit instead of -6, so it doesn't get miscounted as an infra failure anymore.
Let me know if you still see any issues.
…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.
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.
| metadata = { | ||
| "error": "Grading subprocess died before reporting a result", | ||
| "error_code": -6, | ||
| "error_message": f"GradingChildDied (exitcode={p.exitcode})", |
There was a problem hiding this comment.
[Claude] high (data-integrity): The -6 GradingChildDied branch conflates a broken judge with a submission that kills its own grading process, and -6 is in _LCB_INFRA_ERROR_CODES (line 64), so an all-crash batch trips the infra_errors == len(futures) RuntimeError (line 361) and refuses to report a legitimately-0 score — the exact opposite of this PR's stated goal. Untrusted submitted code runs inside the child (_MP_CTX.Process(target=execute_code_single_suppressed_errors) → run_test(..., test=code)), and the child only records a result at the very end (resp_buffer.append(...), line 122). Any path terminating the interpreter before that append — and before the SystemExit/except handlers run — leaves resp_buffer empty with p.is_alive()==False, landing here as -6. Submission-controlled ways to hit it (all bypass Python exception handling, so the new SystemExit→-7 fix at :106 does NOT cover them): os._exit(0); a native segfault (malicious/broken numpy, ctypes, C-extension); an OOM SIGKILL from a huge allocation. So -6 is reachable by ordinary adversarial/bad model output, not just infra. Consequence: a batch where every submission crashes its interpreter aborts with RuntimeError instead of reporting the correct pass@1=0 — discarding a true 0 score.
Root cause / fix: the infra-vs-submission distinction is asserted in a comment + a set literal, not derived from how the child died. Reserve -6 for judge-startup failures (the fork/forkserver problem this PR targets) and classify submission-self-terminated deaths as a submission fault alongside -7 — e.g. treat a clean exitcode==0 empty-buffer as submission fault, and exclude negative/signal exit codes a submission can self-induce from _LCB_INFRA_ERROR_CODES. (Distinct from the already-fixed SystemExit miscount at :176.)
There was a problem hiding this comment.
Good catch — os._exit/segfault/OOM do bypass the -7 guard entirely. Went with a shared started-flag instead of exit codes (a submission can fake any exit code via os._exit): the child flips it right before grading starts, so pre-flag deaths stay -6 infra and post-flag deaths become -8 SubmissionKilledChild, scored as a normal fail. Verified in the container that an all-os._exit batch now reports a true 0 instead of tripping the guard (83db6ed).
| ) + flat_timeout_extension | ||
|
|
||
| manager = mp.Manager() | ||
| manager = _MP_CTX.Manager() |
There was a problem hiding this comment.
[Quality] medium (code-quality): manager = _MP_CTX.Manager() creates a full multiprocessing Manager (a separate server process) for every single code sample and never shuts it down — no manager.shutdown(), no with — so it's only reclaimed by GC finalizer and manager processes leak under load. This compounds with the fork change: run_code_subprocess already runs inside a forked ProcessPoolExecutor worker, so each grade is pool-worker-fork → Manager-server-fork → grading-Process-fork, nested fork-from-fork each carrying inherited parent state. Wrap in with _MP_CTX.Manager() as manager: (or reuse one manager per _LCBWorker batch) to bound process count deterministically.
There was a problem hiding this comment.
Fair point — this actually predates this PR, but worth tightening while we're in here: wrapped it in a with-block and added a join() after kill so a killed grading child gets reaped instead of lingering as a zombie (1918fd7).
| _LCB_INFRA_ERROR_CODES = {-5, -6} | ||
|
|
||
|
|
||
| def execute_code_single(test_suite_json: str, code: str, timeout_sec: int = 60): |
There was a problem hiding this comment.
[Quality] low (code-quality): Missing return-type annotations on the three grading-path functions that cross the process boundary: execute_code_single (67), execute_code_single_suppressed_errors (91), run_code_subprocess (126) — all return tuple[list, dict] (results + metadata) but none declare it, so the res, metadata = ... unpack the whole error-code protocol depends on is unchecked. Relatedly, the fork target at line 91 is def execute_code_single_suppressed_errors(*args, resp_buffer=None, **kwargs) — fully untyped variadics exactly where _MP_CTX.Process(target=..., args=..., kwargs=...) wires args, so a wrong positional/keyword fails only at runtime inside the child (surfacing as a spurious -6). Give both concrete signatures / a named result-tuple alias.
There was a problem hiding this comment.
Done — gave the fork target named parameters and declared tuple[list, dict] on all three grading helpers (7f4d040); the variadics were from previous prs but agreed they hid wiring mistakes until runtime.
| if timed_out: | ||
| p.kill() | ||
|
|
||
| if len(resp_buffer) == 0: |
There was a problem hiding this comment.
[Both] low (code-quality): The two no-result branches duplicate res = [-1] * len(suite["inputs"]) plus a same-shape {error, error_code, error_message} dict (timeout arm 164-169, child-died arm 174-179), and use an else-after-return. A guard clause flattens it: if resp_buffer: return resp_buffer[0] up front, then handle the empty case with no nested else; hoist the shared res assignment and set only the differing metadata per branch.
There was a problem hiding this comment.
Done — early return for the reported-result case and hoisted the shared res (35feb8e); the duplication was honestly my own doing from stacking the attribution branches one review round at a time.
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)
|
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.
4 participants
What
Pin the
forkmultiprocessing context for the outer process pool, per-problem process, and manager used by the LiveCodeBench service. Python 3.14 changed the Linux default toforkserver; in the shipped Python 3.14 service image that caused grading children to die at startup and left evaluation at 0/N.The patch intentionally preserves the existing x86/Python 3.12 grading contract. An empty child response remains an ordinary failed submission, with no new error codes or service-level failure conditions.
Type of change
Testing
pre-commit run --all-filespytest -m unit— 1,514 passed, 5 skippedforkcompleted grading successfullyChecklist