API-contract integration tests, and fold in the measured properties (#275)

* test: API-contract integration layer, and fold in the measured properties (#198, #209)

INTEGRATION TESTS (#198). The pyramid had unit tests that never touch HTTP and Playwright e2e
tests that assert on rendered pixels. Neither would notice a payload silently dropping a field,
renaming a key, or starting to return 200 with an error body — and the workbench, the MCP server
and the Claude skill all consume these payloads BY KEY, so a renamed field is a breaking change
nothing in the suite could see. tests/test_api_contract.py closes that gap with FastAPI's
TestClient: 11 tests over liveness, the head catalog, prediction, the search surfaces and
degradation, with no browser, no port and no fifty-second model load. Suite goes 43 -> 54.

It immediately earned its place by finding a real bug. FLAVORMANCER_NO_MODELS=1 skipped loading
but never set MODELS_READY, so the warming-gate middleware returned 503 to every request FOREVER.
A models-less install was not "degraded", it was dead — the exact opposite of what DATA-PIPELINE.md
promises, and something no existing test could have caught because none of them speak HTTP. It now
marks ready-with-no-models, so the structure-derived answers that need no heads at all (physchem,
the sour/salty rules, applicability) are served.

CI installs fastapi/httpx explicitly. Without them the file skips itself rather than failing,
which would mean CI silently stopping checking every endpoint contract.

MEASURED PROPERTIES (#209). The PUG-View crawl finished: 2,282 molecules gained a measured
boiling or melting point that PubChem's property table never carried.

Folding them in exposed a second NaN trap, and this one had silently discarded the ENTIRE crawl.
The merge guarded on `tgt.get(k) is None`, but 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. First rebuild after the crawl moved coverage by exactly zero. Guarding
with pd.isna() instead:

  boiling point   27% -> 34%   (2,478 -> 3,075 molecules)
  melting point   27% -> 31%   (2,437 -> 2,792)

Verified end to end rather than assumed: of the crawl's 1,354 boiling points, exactly 6 failed to
reach the table. The rest of the apparent shortfall is real and explainable — the crawler targeted
molecules missing EITHER property, so many returned a boiling point that was already present.

This is the same NaN-is-not-None family as the bug that once left ~500 molecules unnamed. Worth
naming as a pattern: any merge against a pandas-derived dict needs pd.isna(), never `is None`.

Signed-off-by: Austin L. <86896075+rvnminers-A-and-N@users.noreply.github.com>

* docs(perf): record why neither way of parallelising model load can ship yet (#225)

Both options are now measured rather than assumed, and the docstring says so, so nobody
re-attempts either one blind.

Threads were already known to be 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 looked like a clean 3.1x win. On 40 aroma models: 11.49 s serial, 1.16 s to deserialise
across a pool while discarding in the worker, 3.74 s once the forests are actually returned to the
parent. So the transfer is real but small next to the unpickling it replaces — roughly 49 s -> 16 s
extrapolated to the full roster.

Then it deadlocked. _load_all_models() runs DURING MODULE IMPORT, and forking while the
interpreter holds the import lock leaves the children inheriting locked import machinery they can
never acquire. The parent and every worker hung indefinitely and had to be SIGKILLed.

So the blocker is not parallelism, it is WHEN loading happens. The pool becomes safe once loading
moves out of import time — a FastAPI startup hook, or an explicit warm() the server calls. That is
a larger change than it sounds: `import predict` having models loaded as a side effect is relied on
by the CLI, the batch scripts and the test suite. Tracked on #225 rather than rushed.

Serial stays, and the warming page already makes the ~50 s visible rather than mysterious.

Signed-off-by: Austin L. <86896075+rvnminers-A-and-N@users.noreply.github.com>

---------

Signed-off-by: Austin L. <86896075+rvnminers-A-and-N@users.noreply.github.com>

Author: rvnminers-A-and-N

.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

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"

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

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: