API-contract integration tests, and fold in the measured properties - #275
Merged
rvnminers-A-and-N merged 2 commits intoJul 31, 2026
Merged
Conversation
…ties (#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>
rvnminers-A-and-N
added
area:data
…ip 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #198. Closes #209.
#198 — the missing middle of the pyramid
Unit tests never touch HTTP; Playwright e2e tests 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, MCP server and 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— 11 TestClient tests over liveness, head catalog, prediction, search surfaces and degradation. No browser, no port, no 50-second model load. Suite 43 → 54.It found a real bug on first run
FLAVORMANCER_NO_MODELS=1skipped loading but never setMODELS_READY, so the warming-gate middleware returned 503 to every request, forever. A models-less install wasn't degraded — it was dead. That's the opposite of whatDATA-PIPELINE.mdpromises, and no existing test could have caught it, because none of them speak HTTP.Now marks ready-with-no-models, so structure-derived answers that need no heads at all (physchem, sour/salty rules, applicability) are served.
CI installs
fastapi/httpxexplicitly — without them the file skips itself rather than failing, which would mean CI silently stopping checking every endpoint contract.#209 — the crawl landed, after a second NaN trap
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 silently discarded the entire crawl. The merge guarded on
tgt.get(k) is None, but a missing value read from parquet isNaN, notNone— so the guard never fired for any row that already existed inproperties.parquetwith an empty cell, which is all of them. The first rebuild after the crawl moved coverage by exactly zero.With
pd.isna()instead:Verified 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 explainable — the crawler targeted molecules missing either property, so many returned a BP that was already present.
A pattern worth naming
This is the second NaN-is-not-None bug (the first left ~500 molecules unnamed). Any merge against a dict built from a DataFrame needs
pd.isna(), neveris None. Written into the code comment so it isn't learned a third time.