diff --git a/src/llm/api.py b/src/llm/api.py index deef2e0..72a190e 100644 --- a/src/llm/api.py +++ b/src/llm/api.py @@ -11,13 +11,28 @@ from __future__ import annotations import json import re +from contextlib import asynccontextmanager from importlib import resources -from typing import Iterator +from typing import AsyncIterator, Iterator from fastapi import FastAPI, Header, HTTPException from fastapi.responses import HTMLResponse, StreamingResponse from pydantic import BaseModel + +@asynccontextmanager +async def _lifespan(app: FastAPI) -> AsyncIterator[None]: + """Warm the DuckDB replica (and refresh the derived families from + it) at boot instead of on the chat's first code-valuation lookup — + refs #699 ruling B3. Never raises: ``llm.evidence.warm`` is a quiet + no-op when the replica file isn't there yet.""" + from llm import config as llm_config + from llm.evidence import warm + + warm(llm_config.load()) + yield + + app = FastAPI( title="llm chat", description=( @@ -25,6 +40,7 @@ app = FastAPI( "reference corpus." ), version="0.2.0", + lifespan=_lifespan, ) _ISO_DATE = re.compile(r"^\d{4}-\d{2}-\d{2}$") diff --git a/src/llm/evidence.py b/src/llm/evidence.py index bf32b41..de03e2f 100644 --- a/src/llm/evidence.py +++ b/src/llm/evidence.py @@ -149,7 +149,26 @@ def _replica_path(cfg: LlmConfig) -> str: return str(ROOT / p) +def warm(cfg: LlmConfig) -> None: + """Open (and cache) the DuckDB replica connection ahead of the first + real request (refs #699 ruling B3) — called once at API startup + (``llm.api``'s startup hook) and again, defensively, at the top of + every ``valuation_evidence`` call, where it is a no-op once cached + (``_connect`` keys on path + mtime, so a second call in the same + process just returns the cached handle without reopening). A missing + replica file is a quiet no-op — never raises, so it can never fail + startup or a chat turn.""" + path = _replica_path(cfg) + if not Path(path).exists(): + return + try: + _connect(path) + except Exception as e: # noqa: BLE001 — duckdb raises several types + log.warning("replica warm-up skipped (%s): %s", path, e) + + def valuation_evidence(question: str, cfg: LlmConfig) -> ValuationEvidence | None: + warm(cfg) det = detect_codes(question) if not det.codes: return None diff --git a/src/pfs/families.py b/src/pfs/families.py index 2e7a278..7e5f9aa 100644 --- a/src/pfs/families.py +++ b/src/pfs/families.py @@ -168,7 +168,18 @@ def _trie_alternation(phrases: Sequence[str]) -> str: return "" # end() only — caller makes it optional alts = [re.escape(ch) + _compile(node[ch]) for ch in chars] body = alts[0] if len(alts) == 1 else "(?:" + "|".join(alts) + ")" - return f"{body}?" if end else body + if not end: + return body + # Bug (fix round 2): a single-branch continuation's `body` can be + # a multi-character string ("s" from `alts[0]`, not grouped) — + # `f"{body}?"` then made only the *last character* optional, not + # the whole continuation, so any phrase that is a proper prefix + # of another (e.g. "cardiac catheterization" inside "cardiac + # catheterization for congenital heart defects") silently + # stopped matching. Always parenthesize before the `?`, even + # when `body` is already a single grouped alternation (harmless + # double-wrap) — this is the only case that's provably correct. + return f"(?:{body})?" return _compile(root) @@ -254,13 +265,27 @@ class Detection: codes: tuple[str, ...] # sorted unique: explicit + family-expanded families: tuple[str, ...] # family keys, sorted explicit: tuple[str, ...] # codes literally present in the text + #: Ruling B2: family keys detected (by name or member code) whose + #: ``len(codes) > MAX_FAMILY_EXPAND`` — still listed in ``families``, + #: but their member codes are *not* folded into ``codes`` (a wide + #: family's whole code list would crowd a valuation prompt). + wide: tuple[str, ...] = () + + +#: Ruling B2: a family this big is a real grouping worth naming in the +#: answer, but not worth pricing every member code for — a 378-code +#: heading (e.g. a broad CPT chapter-level rollup) would otherwise blow +#: the valuation prompt budget for one mention. +MAX_FAMILY_EXPAND = 20 def detect_codes(text: str) -> Detection: """Codes a question is about: explicit codes plus every code of any - family named (by synonym, or by a qualifying derived name) or touched - (by one member code). Runs off the precompiled ``_PHRASE_RE``/ - ``_CODE_INDEX`` — no per-family scan.""" + *narrow* (``len(codes) <= MAX_FAMILY_EXPAND``) family named (by + synonym, or by a qualifying derived name) or touched (by one member + code); a wide family is still named in ``families``/``wide`` but its + codes are not expanded into ``codes``. Runs off the precompiled + ``_PHRASE_RE``/``_CODE_INDEX`` — no per-family scan.""" explicit = find_codes(text) lowered = text.lower() families: set[str] = set() @@ -269,14 +294,20 @@ def detect_codes(text: str) -> Detection: for code in explicit: families.update(_CODE_INDEX.get(code, ())) codes = set(explicit) + wide: set[str] = set() for key in families: fam = FAMILIES.get(key) - if fam is not None: + if fam is None: + continue + if len(fam.codes) <= MAX_FAMILY_EXPAND: codes.update(fam.codes) + else: + wide.add(key) return Detection( codes=tuple(sorted(codes)), families=tuple(sorted(families)), explicit=explicit, + wide=tuple(sorted(wide)), ) diff --git a/tests/llm/test_api.py b/tests/llm/test_api.py index c7139db..5682feb 100644 --- a/tests/llm/test_api.py +++ b/tests/llm/test_api.py @@ -113,6 +113,22 @@ def _cfg(**kw): return LlmConfig(**base) +class TestStartup: + """#699 ruling B3: the replica is warmed at boot, not on the chat's + first valuation lookup. ``with TestClient(app) as c:`` is required to + actually trigger FastAPI's startup event — a bare ``TestClient(app)`` + (as ``client`` above, module-level) never sends the ASGI lifespan + ``startup`` message.""" + + @patch("llm.config.load") + def test_startup_event_warms_the_replica(self, mock_load): + mock_load.return_value = _cfg() + with patch("llm.evidence.warm") as mock_warm: + with TestClient(app): + pass + mock_warm.assert_called_once_with(mock_load.return_value) + + class TestHosts: @patch("llm.pool.pick_model", return_value="big") @patch("llm.pool.HostPool.check", return_value=["http://h2:11434"]) diff --git a/tests/llm/test_evidence.py b/tests/llm/test_evidence.py index e66e882..41d0dc0 100644 --- a/tests/llm/test_evidence.py +++ b/tests/llm/test_evidence.py @@ -4,6 +4,7 @@ from __future__ import annotations from concurrent.futures import ThreadPoolExecutor from dataclasses import replace +from types import SimpleNamespace from unittest.mock import MagicMock, patch import duckdb @@ -16,6 +17,7 @@ from llm.evidence import ( code_cited_sources, merge_sources, valuation_evidence, + warm, ) from pfs.valuation import ValuationRow @@ -228,6 +230,52 @@ class TestValuationEvidence: assert mock_val.call_args.kwargs == {"years": 2} +class TestWarm: + """#699 ruling B3: warm the replica connection ahead of the first + real request — at API startup and again (a no-op once cached) at the + top of every ``valuation_evidence`` call.""" + + def test_warm_opens_once_and_is_a_noop_on_replay(self, tmp_path): + path = tmp_path / "aco.ro.duckdb" + path.touch() # warm() only needs the file to exist, not be a real db + cfg = replace(CFG, duckdb_replica=str(path)) + with patch("llm.evidence.duckdb.connect") as mock_connect: + mock_connect.return_value = MagicMock() + warm(cfg) + warm(cfg) + mock_connect.assert_called_once_with(str(path), read_only=True) + + def test_warm_is_a_noop_when_the_replica_file_is_missing(self): + with patch("llm.evidence.duckdb.connect") as mock_connect: + warm(CFG) # CFG.duckdb_replica ("/nonexistent/...") doesn't exist + mock_connect.assert_not_called() + + def test_warm_swallows_connect_failures(self, tmp_path, caplog): + path = tmp_path / "aco.ro.duckdb" + path.touch() + cfg = replace(CFG, duckdb_replica=str(path)) + with patch("llm.evidence.duckdb.connect", side_effect=OSError("boom")): + warm(cfg) # must not raise + assert "replica warm-up skipped" in caplog.text + + @patch("llm.evidence.detect_codes") + @patch("llm.evidence.warm") + def test_valuation_evidence_warms_before_detecting_codes( + self, mock_warm, mock_detect + ): + order: list[str] = [] + mock_warm.side_effect = lambda cfg: order.append("warm") + + def _detect(question): + order.append("detect") + return SimpleNamespace(codes=()) + + mock_detect.side_effect = _detect + assert valuation_evidence("anything", CFG) is None + assert order == ["warm", "detect"] + mock_warm.assert_called_once_with(CFG) + + class TestConnectRefreshesFamilies: """#699: derived families (``pfs.code_family``) live only on the replica — ``_connect`` is the chat's one hook to pick them up, on diff --git a/tests/pfs/test_families.py b/tests/pfs/test_families.py index 09d8fab..4778e91 100644 --- a/tests/pfs/test_families.py +++ b/tests/pfs/test_families.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +import re import time import duckdb @@ -21,9 +22,11 @@ from pfs.codetables import ( from pfs.families import ( FAMILIES, HAND_FAMILIES, + MAX_FAMILY_EXPAND, Detection, Family, _cpt_edges, + _trie_alternation, cpt_groups, derive_families, detect_codes, @@ -125,6 +128,77 @@ class TestDetectCodes: ) +class TestTrieAlternation: + """Fix round 2, item 1: ``_trie_alternation``'s optional-continuation + bug. When a node has exactly one continuation, ``body`` was an + unwrapped (possibly multi-character) string, and a trailing ``?`` + applied to it bound to only its *last character* — so any phrase + that is a proper prefix of a longer registered phrase (e.g. + "cardiac catheterization" inside "cardiac catheterization for + congenital heart defects") silently stopped matching at all. The + task-1 report's "verified identical" claim was based on running one + sample question through the live registry, which happened not to + exercise a prefix-of-another-phrase pair — it did not prove + equivalence in general, and was wrong.""" + + #: A two-way prefix collision ("cardiac catheterization" is a strict + #: prefix of the "... for congenital heart defects" phrase), a + #: three-way share (both "... services" and "... program" continue + #: the same "chronic care management" prefix, so that node has an + #: end marker *and* two children), plus the five phrases the report + #: named as broken live. + PHRASES = ( + "cardiac catheterization", + "cardiac catheterization for congenital heart defects", + "chronic care management", + "chronic care management services", + "chronic care management program", + "tricuspid valve", + "tricuspid valve repair", + "adaptive behavior assessments", + "repair and/or reconstruction", + "subcutaneous cardiac rhythm monitor", + ) + + SENTENCES = ( + "Discuss cardiac catheterization for congenital heart defects in infants.", + "A cardiac catheterization was performed without complication.", + "Chronic care management services are billed monthly.", + "Our chronic care management program includes personalized outreach.", + "We provide chronic care management for our patients, full stop.", + "Tricuspid valve repair is a common cardiac procedure.", + "The tricuspid valve was evaluated by echo.", + "Adaptive behavior assessments are conducted for autism evaluations.", + "This code covers repair and/or reconstruction of the tendon.", + "A subcutaneous cardiac rhythm monitor was implanted last week.", + "None of these words appear in this sentence at all.", + ) + PHRASES # each phrase alone too + + @staticmethod + def _flat_re(phrases) -> re.Pattern[str]: + ordered = sorted(phrases, key=len, reverse=True) + alt = "|".join(re.escape(p) for p in ordered) + return re.compile(rf"\b(?:{alt})\b", re.IGNORECASE) + + @staticmethod + def _trie_re(phrases) -> re.Pattern[str]: + alt = _trie_alternation(sorted(phrases)) + return re.compile(rf"\b(?:{alt})\b", re.IGNORECASE) + + def test_trie_matches_flat_alternation(self): + flat_re = self._flat_re(self.PHRASES) + trie_re = self._trie_re(self.PHRASES) + for s in self.SENTENCES: + lowered = s.lower() + flat = [m.group(0) for m in flat_re.finditer(lowered)] + trie = [m.group(0) for m in trie_re.finditer(lowered)] + assert trie == flat, (s, flat, trie) + # And directly: every prefix-collision phrase, embedded in its + # longer sibling, must still match on its own. + assert self._trie_re(self.PHRASES).search("cardiac catheterization today") + assert self._trie_re(self.PHRASES).search("chronic care management today") + + class TestDetectDerivedFamilies: """#699: the chat must see derived (``pfs.code_family``) families, not only the five hand families — by member code always, by name @@ -188,6 +262,41 @@ class TestDetectDerivedFamilies: elapsed = time.perf_counter() - start assert elapsed < 0.2, elapsed + def test_wide_family_does_not_expand_codes(self, restore_families): + # Ruling B2: a family with more than MAX_FAMILY_EXPAND (20) codes + # is still named (in `families`/`wide`) but its member codes are + # not folded into `codes` — only explicit codes survive. + wide_codes = tuple(f"{20000 + i}" for i in range(378)) + assert len(wide_codes) > MAX_FAMILY_EXPAND + restore_families.FAMILIES["WIDE-FAMILY"] = Family( + "WIDE-FAMILY", + "Comprehensive Ambulatory Service Bundle", + wide_codes, + (), + cpt=True, + ) + rebuild_index() + d = detect_codes("How is the Comprehensive Ambulatory Service Bundle valued?") + assert d.explicit == () + assert d.codes == d.explicit # nothing expanded + assert "WIDE-FAMILY" in d.families + assert "WIDE-FAMILY" in d.wide + + def test_narrow_family_still_expands_under_the_cap(self, restore_families): + codes = tuple(f"{30000 + i}" for i in range(MAX_FAMILY_EXPAND)) + restore_families.FAMILIES["NARROW-FAMILY"] = Family( + "NARROW-FAMILY", + "Narrow Bundled Service Package", + codes, + (), + cpt=True, + ) + rebuild_index() + d = detect_codes("How is the Narrow Bundled Service Package valued?") + assert d.codes == codes + assert "NARROW-FAMILY" in d.families + assert d.wide == () + def test_hand_families_regression(self): # Unchanged from TestDetectCodes — the phrase/index refactor must # not alter a single hand-family detection.