From 402645bc674840f7d727a9a9e1a95083a07f32c9 Mon Sep 17 00:00:00 2001 From: joaovictor91123 Date: Mon, 13 Jul 2026 00:15:07 -0400 Subject: [PATCH] fix(validator): skip synchronous eval for submissions with no artifact (#143) Infra/bugfix PRs that ship no submissions/final_model/best_theta.npy were being evaluated, failing with `missing_checkpoint`, and marked `failed` -- the state that closes them before a maintainer can merge (#143). The async paths already skip such submissions: the pipeline enqueue in /submit and queue.py both gate on `submission_artifact_id is not None`. The synchronous branch (`if settings.sync_eval_on_submit and not settings.uses_train_pipeline`) did not, so evaluate_submission ran on a checkpoint-less submission. Extract _should_sync_eval(settings, submission) and add the same artifact clause so the sync path matches the async ones. eval_runner's missing_checkpoint failure is left intact -- it is the correct behaviour when an artifact row exists but its checkpoint file is genuinely missing. Tested offline via the pure _should_sync_eval helper (no Postgres required): no-artifact -> skip, artifact present -> eval, sync disabled / train pipeline -> defer to async. --- docs/JOURNAL.md | 22 +++++++++ validator/src/eval_backend/api/routes.py | 19 +++++++- validator/tests/test_submit_sync_eval_gate.py | 45 +++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 validator/tests/test_submit_sync_eval_gate.py diff --git a/docs/JOURNAL.md b/docs/JOURNAL.md index 117bb79..3728cda 100644 --- a/docs/JOURNAL.md +++ b/docs/JOURNAL.md @@ -18,6 +18,28 @@ protocol. **Newest entries at the top.** Tag each entry with one or more of: --- +## 2026-07-13 — /submit sync-eval ran on checkpoint-less PRs, failing them (#143) #mistake #gotcha +**Context:** infra/bugfix PRs (no `submissions/final_model/best_theta.npy`) were being marked +`failed` with `missing_checkpoint`, which closes them before a maintainer can merge (#143). One of +my own PRs (#117) was auto-closed exactly this way. +**Expected:** a submission with no uploaded model bundle is never evaluated. +**Actual:** the async paths already skip such submissions — the pipeline enqueue in `/submit` +(`routes.py`) and `queue.py` both gate on `submission.submission_artifact_id is not None` — but the +**synchronous** branch (`if settings.sync_eval_on_submit and not settings.uses_train_pipeline:`) did +not, so `evaluate_submission` ran on a checkpoint-less submission and hit the +`does not have a checkpoint to evaluate` failure in `eval_runner.py`. +**Root cause:** the artifact gate was added to the async paths but the synchronous path was missed — +the invariant "never evaluate a submission without an artifact" was enforced in two of three places. +**Fix / decision:** extracted `_should_sync_eval(settings, submission)` and added the same +`submission_artifact_id is not None` clause, so the sync path matches the async ones. Deliberately +did **not** touch `eval_runner`'s `missing_checkpoint` failure — that is a legitimate safety net for a +submission that *has* an artifact row whose checkpoint file is genuinely missing. +**Follow-up:** this only stops the validator from *generating* the failure; a maintainer still has to +deploy it, and it cannot reopen already-closed PRs (#117 was re-opened as a fresh PR). Testable +offline via the pure `_should_sync_eval` helper (no Postgres needed). + +--- + ## 2026-07-12 — Validator Postgres tests no longer silently skip in CI #decision #repro **Context:** issue #118 flagged that validator DB-backed tests could ``pytest.skip`` whenever Postgres was unreachable, including on CI where no database service was provisioned. diff --git a/validator/src/eval_backend/api/routes.py b/validator/src/eval_backend/api/routes.py index aeff8f4..1a37f08 100644 --- a/validator/src/eval_backend/api/routes.py +++ b/validator/src/eval_backend/api/routes.py @@ -377,6 +377,23 @@ def health() -> HealthResponse: return HealthResponse() +def _should_sync_eval(settings: Settings, submission: Submission) -> bool: + """Whether to run ``evaluate_submission`` inline for a just-registered submission. + + A submission with no ``submission_artifact_id`` has no uploaded model bundle + (e.g. an infra/bugfix PR that never ships ``best_theta.npy``). Evaluating it + only produces a spurious ``missing_checkpoint`` failure that marks the PR's + submission ``failed`` — the behaviour that closes infra PRs (#143). The async + enqueue paths already skip such submissions (see ``queue.py`` and the pipeline + gate below); this keeps the *synchronous* path consistent with them. + """ + return ( + settings.sync_eval_on_submit + and not settings.uses_train_pipeline + and submission.submission_artifact_id is not None + ) + + @router.post("/submit", response_model=SubmissionCreateResponse) async def submit( request: Request, @@ -449,7 +466,7 @@ async def submit( session.flush() session.commit() - if settings.sync_eval_on_submit and not settings.uses_train_pipeline: + if _should_sync_eval(settings, submission): session.refresh(submission) evaluate_submission(session, submission, runtime_settings) session.commit() diff --git a/validator/tests/test_submit_sync_eval_gate.py b/validator/tests/test_submit_sync_eval_gate.py new file mode 100644 index 0000000..c014800 --- /dev/null +++ b/validator/tests/test_submit_sync_eval_gate.py @@ -0,0 +1,45 @@ +"""Offline tests for the synchronous-eval gate on ``POST /submit`` (issue #143). + +A submission with no ``submission_artifact_id`` has no uploaded model bundle +(an infra/bugfix PR that never ships ``best_theta.npy``). Running +``evaluate_submission`` on it only yields a spurious ``missing_checkpoint`` +failure that marks the PR's submission ``failed`` — the behaviour that closes +infra PRs. The synchronous path must skip such submissions, exactly as the async +enqueue paths already do. + +These tests exercise the pure ``_should_sync_eval`` decision, so they need no +database and run everywhere (the DB-backed validator tests skip without Postgres). +""" +from __future__ import annotations + +from types import SimpleNamespace + +from eval_backend.api.routes import _should_sync_eval +from eval_backend.core.config import PIPELINE_TRAIN_EVAL, Settings + + +def _submission(artifact_id): + return SimpleNamespace(submission_artifact_id=artifact_id) + + +def test_no_artifact_is_never_sync_evaluated(): + """The #143 regression: a checkpoint-less submission must not be evaluated.""" + settings = Settings(sync_eval_on_submit=True) + assert settings.uses_train_pipeline is False + assert _should_sync_eval(settings, _submission(None)) is False + + +def test_artifact_present_is_sync_evaluated(): + settings = Settings(sync_eval_on_submit=True) + assert _should_sync_eval(settings, _submission("artifact-123")) is True + + +def test_sync_eval_disabled_never_runs_inline(): + settings = Settings(sync_eval_on_submit=False) + assert _should_sync_eval(settings, _submission("artifact-123")) is False + + +def test_train_pipeline_defers_to_async_even_with_artifact(): + settings = Settings(sync_eval_on_submit=True, pipeline_mode=PIPELINE_TRAIN_EVAL) + assert settings.uses_train_pipeline is True + assert _should_sync_eval(settings, _submission("artifact-123")) is False