diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 93cdef3..fd4d1b6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,7 +23,12 @@ jobs: run: ruff check . - name: Install runtime deps run: pip install -r requirements.txt - - name: Pytest (unit) + # fastapi + httpx back the TestClient integration tests (tests/test_api_contract.py). + # Without them that file skips itself rather than failing, but then CI silently stops + # checking every endpoint contract — so install them explicitly. + - name: Install test-only deps + run: pip install fastapi httpx pydantic + - name: Pytest (unit + integration) run: | if git ls-files '*test_*.py' '*_test.py' 'tests/**/*.py' | grep -qv '^tests/e2e/'; then pytest -q --ignore=tests/e2e diff --git a/tests/test_api_contract.py b/tests/test_api_contract.py new file mode 100644 index 0000000..44ff73b --- /dev/null +++ b/tests/test_api_contract.py @@ -0,0 +1,154 @@ +"""Integration tests: every endpoint's CONTRACT, through FastAPI's TestClient. + +The suite already had unit tests (rules, physchem, stereo-safety) and Playwright e2e tests that +drive a real browser against a real server. This is the missing middle: it exercises the actual +HTTP layer — routing, request parsing, response shape, status codes — without a browser, without a +port, and without waiting fifty seconds for models to load. + +That gap mattered. A response that silently drops a field, renames a key, or starts returning 200 +with an error body would sail past the unit tests (which never touch HTTP) and might well pass the +e2e tests too (which assert on rendered pixels, not payloads). The workbench, the MCP server and +the Claude skill all consume these payloads by key, so a renamed field is a breaking change that +nothing else in the pyramid would catch. + +Runs in CI with FLAVORMANCER_NO_MODELS=1: endpoints that need trained heads answer honestly with +`available: false` rather than 500, and that degradation is itself worth asserting — it is what +keeps a partially-built install usable instead of broken. +""" +import os + +import pytest + +# Import the app the same way uvicorn does. NO_MODELS keeps this fast and CI-friendly; the tests +# below assert the *contract*, which must hold whether or not the heads are present. +os.environ.setdefault("FLAVORMANCER_NO_MODELS", "1") + +# importorskip is not enough here: `fastapi.testclient` imports cleanly and then starlette raises +# a RuntimeError if httpx is missing, which pytest reports as a collection ERROR rather than a +# skip. Catch both so an environment without the test extras skips this file instead of failing. +try: + from fastapi.testclient import TestClient +except (ImportError, RuntimeError) as exc: # pragma: no cover — environment-dependent + pytest.skip(f"FastAPI TestClient unavailable ({exc})", allow_module_level=True) + +import app as flavor_app + +VANILLIN = "COc1cc(C=O)ccc1O" + + +@pytest.fixture(scope="module") +def client(): + with TestClient(flavor_app.app) as c: + yield c + + +# --- liveness ------------------------------------------------------------------------------- + +def test_healthz_is_always_answerable(client): + """/healthz must answer even while models are loading — it is what the container's health + check polls, and a 503 during a legitimate 50s warm-up would kill the container.""" + r = client.get("/healthz") + assert r.status_code == 200, r.text + + +def test_status_reports_load_progress(client): + r = client.get("/api/status") + assert r.status_code == 200 + body = r.json() + for key in ("loaded", "total", "phase", "ready"): + assert key in body, f"/api/status lost `{key}` — the warming page polls it" + assert isinstance(body["ready"], bool) + + +# --- head catalog --------------------------------------------------------------------------- + +def test_heads_catalog_shape(client): + """The catalog is consumed by the modal, the studios, the MCP server and the skill. Its four + categories and their per-head fields are a contract, not an implementation detail.""" + r = client.get("/api/heads") + assert r.status_code == 200 + body = r.json() + for group in ("taste", "aroma", "mouthfeel", "safety"): + assert group in body, f"/api/heads lost the `{group}` group" + assert isinstance(body[group], list) + + +def test_every_head_publishes_its_calibration(client): + """Since #261 a head must say where its bar sits and how precise it is there. Dropping these + would silently return the project to reporting AUROC alone — the exact failure ACCURACY.md + exists to prevent.""" + body = client.get("/api/heads").json() + for group in ("taste", "aroma", "mouthfeel", "safety"): + for head in body[group]: + assert "head" in head + assert "confident_capable" in head, ( + f"{group}/{head.get('head')} has no confident_capable flag") + thr = head.get("threshold") + if thr is not None: + assert 0.0 < float(thr) <= 1.0, f"{head['head']} threshold {thr} out of range" + + +# --- prediction ----------------------------------------------------------------------------- + +def test_predict_returns_the_documented_blocks(client): + r = client.post("/api/predict", json={"smiles": VANILLIN}) + assert r.status_code == 200, r.text + body = r.json() + assert body.get("smiles") + # physchem and the applicability gate are computed from structure alone, so they are present + # even with no trained heads at all — that is what makes a models-less install still useful. + assert "physchem" in body + assert "applicability" in body + + +def test_predict_rejects_an_unparseable_smiles(client): + """Garbage in must produce a clear error, not a 500 and not a confident-looking empty read.""" + r = client.post("/api/predict", json={"smiles": "not-a-molecule"}) + assert r.status_code < 500, "an unparseable SMILES must not crash the service" + body = r.json() + assert "error" in body or body.get("applicability", {}).get("in_domain") is False, ( + "an unparseable SMILES should surface an error, not a silent empty result") + + +def test_predict_requires_smiles(client): + r = client.post("/api/predict", json={}) + assert r.status_code < 500 + + +# --- search surfaces ------------------------------------------------------------------------ + +def test_design_search_contract(client): + """The reverse search backs the studios. Its envelope (items / requested / total_matches) + is what the UI paginates against.""" + r = client.get("/api/design", params={"descriptors": "vanilla", "limit": 3}) + assert r.status_code == 200 + body = r.json() + for key in ("items", "requested", "total_matches", "offset", "limit"): + assert key in body, f"/api/design lost `{key}`" + assert isinstance(body["items"], list) + assert len(body["items"]) <= 3, "limit must be honoured" + + +def test_design_with_an_unknown_descriptor_is_empty_not_an_error(client): + r = client.get("/api/design", params={"descriptors": "notarealnote"}) + assert r.status_code == 200 + assert r.json()["items"] == [] + + +def test_studio_terms_groups(client): + r = client.get("/api/studio_terms") + assert r.status_code == 200 + body = r.json() + for group in ("flavors", "notes", "mouthfeel", "taste"): + assert group in body, f"/api/studio_terms lost `{group}` — a chip family would vanish" + + +# --- degradation ---------------------------------------------------------------------------- + +def test_model_backed_endpoints_degrade_honestly(client): + """With no heads loaded, endpoints that need them must say so rather than 500 or, worse, + return an empty read that looks like a confident 'no aroma'.""" + body = client.post("/api/predict", json={"smiles": VANILLIN}).json() + aroma = body.get("aroma") + if isinstance(aroma, dict) and aroma.get("available") is False: + assert aroma.get("note"), "an unavailable modality must explain why" diff --git a/training/build_enrichment.py b/training/build_enrichment.py index c50ed89..00f69c2 100644 --- a/training/build_enrichment.py +++ b/training/build_enrichment.py @@ -248,6 +248,7 @@ def _taste_by_skel(): for _skel, _row in _by_skel("iupac_backfill.parquet", ["common_name", "iupac_name"]).items(): tgt = props.setdefault(_skel, {}) for _k in ("common_name", "iupac_name"): + # isinstance(..., str) is the right guard here — it treats NaN as absent correctly if not isinstance(tgt.get(_k), str) and isinstance(_row.get(_k), str): tgt[_k] = _row[_k] # MEASURED boiling/melting points from PUG-View (build_measured_properties.py). The property @@ -257,7 +258,11 @@ def _taste_by_skel(): ["melting_point_c", "boiling_point_c"]).items(): tgt = props.setdefault(_skel, {}) for _k in ("melting_point_c", "boiling_point_c"): - if tgt.get(_k) is None and _row.get(_k) is not None: + # `is None` is WRONG here and silently discarded the entire crawl: a missing value + # read from parquet is NaN, not None, so the guard never fired for any row that + # already existed in properties.parquet with an empty cell — which is all of them. + # Same NaN trap that once left ~500 molecules unnamed (see _pick_name). + if pd.isna(tgt.get(_k)) and not pd.isna(_row.get(_k)): tgt[_k] = _row[_k] taste_doc = _taste_by_skel() curated = _curated_names() # human names for the molecules we hand-curated diff --git a/training/predict.py b/training/predict.py index d803459..e93271f 100644 --- a/training/predict.py +++ b/training/predict.py @@ -266,9 +266,22 @@ def _load_all_models(): """Discover and load every trained head (taste + tox + aroma), updating LOAD_PROGRESS as each lands, then set MODELS_READY. Runs on a daemon thread from import so the port binds instantly. - Loading is SERIAL on purpose: joblib.load is dominated by GIL-bound Python unpickling, so a - thread pool only adds contention (measured ~34 s serial vs ~86 s across 14 threads). The win - from cores comes at INFERENCE time (predict_proba releases the GIL) — see _aroma_scores_canon.""" + Loading is SERIAL, and BOTH ways of parallelising it have now been measured and rejected: + + threads WORSE than serial (~34 s serial vs ~86 s across 14 threads). joblib.load is + dominated by GIL-bound Python unpickling, so threads only add contention. + processes 3.1x FASTER in isolation — 40 files take 11.5 s serial and 3.7 s across a + ProcessPoolExecutor, transfer of the deserialised forests included. But this + function runs DURING MODULE IMPORT, and forking while the interpreter holds the + import lock deadlocks: the children inherit a locked import machinery they can + never acquire. Tried it; the parent and every worker hung indefinitely. + + The process pool is the right answer, but only once loading is deferred out of import time + (a FastAPI startup hook, or an explicit warm() the server calls) — see #225. Until then serial + is correct, and the warming page makes the ~50 s visible rather than mysterious. + + The win from cores comes at INFERENCE time instead (predict_proba releases the GIL) — see + _aroma_scores_canon.""" jobs = [] # (kind, name, path) if TASTE.exists(): jobs += [("taste", p.stem.replace("_rf", ""), p) for p in TASTE.glob("*_rf.joblib")] @@ -323,7 +336,14 @@ def _load_all_models(): # only need featurization (_feat / _MORGAN), like the parallel index builder, which loads each head # in its own worker process rather than in this parent. if os.environ.get("FLAVORMANCER_NO_MODELS") == "1": - pass + # Skip loading, but still mark READY. Leaving the event unset meant the app's warming gate + # returned 503 to every request FOREVER — a models-less install was not "degraded", it was + # dead, which is the opposite of what the docs promised. Structure-derived answers (physchem, + # the sour/salty rules, applicability, substructure) need no heads at all and should be served. + with _LOAD_LOCK: + LOAD_PROGRESS["phase"] = "ready (no models)" + LOAD_PROGRESS["ready"] = True + MODELS_READY.set() elif os.environ.get("FLAVORMANCER_BLOCKING_LOAD") == "1": _load_all_models() else: