diff --git a/CHANGELOG.md b/CHANGELOG.md index 11f89028..bcace0fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`POST /api/enrich/extract` returned 503 on every request; the whole `/api/decisions*` family returned 500 as soon as a decision existed** (#886, closes #883, closes #884, closes #889) by @joseedson18jc, reviewed by @Sameer6305 + - `semantica/explorer/routes/enrich.py` imported `extract_entities`/`extract_relations` from `semantica.semantic_extract.methods`, names that module never defined (only per-strategy variants like `extract_entities_ml` exist) — the `except ImportError` handler reported this as `"semantic_extract module not available"`, masking a wiring bug as a missing dependency. The route now calls `NamedEntityRecognizer`/`RelationExtractor` directly and forwards extracted entities into relation extraction instead of re-deriving them + - `ContextGraph.record_decision()` stores `timestamp` as `datetime.now().timestamp()` (a float), while `DecisionResponse.timestamp` was typed `Optional[str]`; passing the value through unconverted failed pydantic validation on every decision route (`/api/decisions`, `/{id}`, `/{id}/chain`, `/{id}/precedents`, `/{id}/compliance`). Added a `field_validator(mode="before")` on `DecisionResponse` normalizing float/int/datetime inputs to ISO-8601 + - Folds in the fix for #889: `extract_entities_ml`/`extract_relations_similarity`/`extract_relations_dependency` called `spacy.load()` on every invocation (~120ms of a ~132ms call, ~60x the actual extraction work). Added a process-level, lock-guarded `load_spacy_model()` cache in `semantic_extract/methods.py`, keyed by model name; failed loads are not cached, and the separate `get_nlp_model()` cache (different `disable=` pipeline config for similarity work) is kept independent to avoid handing one caller's spaCy pipeline to another + - **Fixed during review** (@Sameer6305): capped previously-unbounded input text on `/api/enrich/extract`; tightened the route's exception handling + - **Fixed during review** (@KaifAhmad1): the timestamp validator's `math.isfinite()` guard only rejected NaN/inf — a finite-but-out-of-range epoch (e.g. milliseconds mistakenly stored instead of seconds, such as `1723600000000`) still raised an uncaught `OverflowError`/`OSError` from `datetime.fromtimestamp()`, reintroducing an unhandled 500 on `/api/decisions*` for exactly the class of bug this PR closes. Now caught and re-raised as a `ValueError`. Also excluded `bool` from the numeric branch (`isinstance(True, int)` is `True` in Python, so `timestamp=True` was silently coerced to epoch 1 instead of being rejected) + - New/updated tests: `tests/explorer/test_explorer_api.py` (`TestRecordedDecisions`, extraction coverage, 4 new `TestDecisionResponseTimestampValidator` cases for the range/bool fixes), `tests/semantic_extract/test_spacy_model_cache.py` (6 tests) + - `pytest tests/explorer tests/semantic_extract/test_spacy_model_cache.py`: 266 passed + - **Explorer UI hid backend failures: graph load hung forever, landing page always showed "System Online"** (#980, closes #977) by @ZohaibHassan16, reviewed by @Sameer6305 - `GraphWorkspace.tsx` only destructured `{ data, isLoading, isFetching }` from `useLoadGraph()`, ignoring the `isError`/`error`/`refetch` that `useQuery` (`retry: 0`) already returned. Combined with `GraphLoadingOverlay` having no error prop and `showLoadingOverlay` staying true whenever `loadingProgress` held a stale frame, a backend-down or failed fetch left the graph workspace stuck on the last progress frame indefinitely, with no error message and no way to recover short of a full page reload - `GraphLoadingOverlay` now accepts `error`/`onRetry` and renders an error card with the real fetch error message and a Retry button (`refetch()`) instead of the stuck progress UI diff --git a/semantica/explorer/routes/enrich.py b/semantica/explorer/routes/enrich.py index 699875fa..8e935e75 100644 --- a/semantica/explorer/routes/enrich.py +++ b/semantica/explorer/routes/enrich.py @@ -176,26 +176,30 @@ async def extract_entities( session: GraphSession = Depends(get_session), ): try: - from ...semantic_extract.methods import extract_entities as _extract_entities - from ...semantic_extract.methods import extract_relations as _extract_relations - - entities = await asyncio.to_thread(_extract_entities, body.text) - relations = await asyncio.to_thread(_extract_relations, body.text) - - ent_list = entities if isinstance(entities, list) else getattr(entities, "entities", []) - rel_list = relations if isinstance(relations, list) else getattr(relations, "relations", []) - - return EnrichExtractResponse( - entities=[_safe_dict(entity) for entity in ent_list], - relations=[_safe_dict(relation) for relation in rel_list], - ) + from ...semantic_extract import NamedEntityRecognizer, RelationExtractor except ImportError: raise HTTPException( status_code=503, detail="semantic_extract module not available. Ensure spacy and transformers are installed.", ) - except Exception as exc: - raise HTTPException(status_code=422, detail=f"Extraction failed: {exc}") + + recognizer = NamedEntityRecognizer(confidence_threshold=0.7) + extractor = RelationExtractor(confidence_threshold=0.6) + + entities = await asyncio.to_thread(recognizer.extract_entities, body.text) + + ent_list = entities if isinstance(entities, list) else getattr(entities, "entities", []) + + relations = await asyncio.to_thread( + extractor.extract_relations, body.text, ent_list + ) + + rel_list = relations if isinstance(relations, list) else getattr(relations, "relations", []) + + return EnrichExtractResponse( + entities=[_safe_dict(entity) for entity in ent_list], + relations=[_safe_dict(relation) for relation in rel_list], + ) @router.post("/api/enrich/links", response_model=LinkPredictionResponse) diff --git a/semantica/explorer/schemas.py b/semantica/explorer/schemas.py index 638fa82e..13f2ac3f 100644 --- a/semantica/explorer/schemas.py +++ b/semantica/explorer/schemas.py @@ -2,10 +2,10 @@ Shared Pydantic schemas for the Semantica Knowledge Explorer API. """ -from datetime import datetime +from datetime import datetime, timezone from typing import Any, Dict, List, Literal, Optional, Tuple -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, field_validator class ErrorResponse(BaseModel): @@ -144,6 +144,37 @@ class DecisionResponse(BaseModel): timestamp: Optional[str] = None metadata: Dict[str, Any] = Field(default_factory=dict) + @field_validator("timestamp", mode="before") + @classmethod + def _normalize_timestamp(cls, value: Any) -> Optional[str]: + """Accept the epoch floats ContextGraph.record_decision() writes. + + Decision nodes store ``timestamp`` as ``datetime.now().timestamp()``, a + float, so passing the stored value through unconverted fails validation + and turns every decision route into a 500. Normalize to ISO-8601 here so + the wire format stays a single string type whatever the producer wrote. + """ + if value is None or isinstance(value, str): + return value + if isinstance(value, datetime): + return value.isoformat() + if isinstance(value, (int, float)) and not isinstance(value, bool): + import math + if not math.isfinite(value): + raise ValueError( + f"timestamp must be a finite number, got {value!r}" + ) + try: + return datetime.fromtimestamp(value, tz=timezone.utc).isoformat() + except (OverflowError, OSError) as exc: + raise ValueError( + f"timestamp {value!r} is out of the representable epoch range" + ) from exc + raise ValueError( + f"timestamp must be None, a string, a datetime, or a numeric epoch; " + f"got {type(value).__name__!r}" + ) + class CausalChainResponse(BaseModel): decision_id: str @@ -174,7 +205,9 @@ class TemporalPatternResponse(BaseModel): class EnrichExtractRequest(BaseModel): - text: str + # 10 000 characters is sufficient for a substantial document paragraph while + # preventing unbounded spaCy NLP processing on arbitrarily large payloads. + text: str = Field(..., max_length=10_000) class EnrichExtractResponse(BaseModel): diff --git a/semantica/semantic_extract/methods.py b/semantica/semantic_extract/methods.py index 72898deb..f0559dec 100644 --- a/semantica/semantic_extract/methods.py +++ b/semantica/semantic_extract/methods.py @@ -108,6 +108,7 @@ License: MIT import re import difflib +import threading from concurrent.futures import ThreadPoolExecutor, as_completed from typing import Any, Dict, List, Optional, Tuple, Union @@ -153,6 +154,39 @@ spacy, SPACY_AVAILABLE = safe_import("spacy") _nlp_cache = None _embedder_cache = None +# Cache for models loaded by name, so extraction functions do not pay +# spacy.load() on every call. Entries record the spacy module they were loaded +# from: tests patch `methods.spacy` with a mock, and an entry produced by a +# different module object must not be handed back to a later caller. +_spacy_model_cache: Dict[str, Tuple[Any, Any]] = {} +_spacy_model_cache_lock = threading.Lock() + + +def load_spacy_model(name: str): + """Load a spaCy model once per process, keyed by model name. + + Raises whatever ``spacy.load`` raises (``OSError`` for a missing model), so + callers keep their existing fallback behavior. + """ + cached = _spacy_model_cache.get(name) + if cached is not None and cached[0] is spacy: + return cached[1] + + with _spacy_model_cache_lock: + cached = _spacy_model_cache.get(name) + if cached is not None and cached[0] is spacy: + return cached[1] + nlp = spacy.load(name) + _spacy_model_cache[name] = (spacy, nlp) + return nlp + + +def clear_spacy_model_cache() -> None: + """Drop every cached spaCy model. Intended for tests.""" + with _spacy_model_cache_lock: + _spacy_model_cache.clear() + + def get_text_embedder(): """ Get or load the TextEmbedder model for high-accuracy semantic similarity. @@ -676,11 +710,11 @@ def extract_entities_ml( return extract_entities_pattern(text, **kwargs) try: - nlp = spacy.load(model) + nlp = load_spacy_model(model) except OSError: logger.warning(f"spaCy model {model} not found, using en_core_web_sm") try: - nlp = spacy.load("en_core_web_sm") + nlp = load_spacy_model("en_core_web_sm") except OSError: logger.warning( "spaCy model not available, falling back to pattern extraction" @@ -1400,12 +1434,12 @@ def extract_relations_similarity( # Prefer larger models for vectors for model_name in ["en_core_web_lg", "en_core_web_md", "en_core_web_sm"]: if spacy.util.is_package(model_name): - nlp = spacy.load(model_name) + nlp = load_spacy_model(model_name) break if not nlp: # Try loading what we have try: - nlp = spacy.load("en_core_web_sm") + nlp = load_spacy_model("en_core_web_sm") except: pass except Exception: @@ -1505,7 +1539,7 @@ def extract_relations_dependency( return extract_relations_pattern(text, entities, **kwargs) try: - nlp = spacy.load(model) + nlp = load_spacy_model(model) except OSError: logger.warning(f"spaCy model {model} not found") return extract_relations_pattern(text, entities, **kwargs) diff --git a/tests/explorer/test_explorer_api.py b/tests/explorer/test_explorer_api.py index 4f16d13e..18d7fcaf 100644 --- a/tests/explorer/test_explorer_api.py +++ b/tests/explorer/test_explorer_api.py @@ -1,5 +1,6 @@ """Integration tests for the explorer API.""" +from datetime import datetime import json from pathlib import Path import uuid @@ -444,6 +445,63 @@ class TestDecisions: assert violation_response.json()["compliant"] is False +@pytest.fixture(scope="module") +def recorded_client(): + """Client over a graph whose decisions were written by record_decision().""" + graph = ContextGraph(advanced_analytics=False) + entities = ["applicant_A7291"] + graph.record_decision( + category="credit_application", + scenario="Personal loan, $85k income, 31% DTI", + reasoning="Income meets threshold; employment stable", + outcome="proceed_to_underwriting", + confidence=0.88, + entities=entities, + ) + graph.record_decision( + category="loan_underwriting", + scenario="Underwriting review for A-7291", + reasoning="DTI within policy; clean 36-month credit history", + outcome="approved", + confidence=0.94, + entities=entities, + ) + with TestClient(create_app(session=GraphSession(graph))) as test_client: + yield test_client + + +class TestRecordedDecisions: + """Decisions written by record_decision(), not hand-built decision nodes. + + record_decision() stores ``timestamp`` as a float epoch. The fixtures above + set no timestamp at all, so these routes were only ever exercised against + decision nodes that could not trigger the float/str mismatch. + """ + + def test_list_decisions_serializes_float_timestamp(self, recorded_client): + response = recorded_client.get("/api/decisions") + assert response.status_code == 200 + payload = response.json() + assert len(payload) == 2 + for item in payload: + assert isinstance(item["timestamp"], str) + datetime.fromisoformat(item["timestamp"]) + + def test_get_decision(self, recorded_client): + listed = recorded_client.get("/api/decisions").json() + decision_id = listed[0]["decision_id"] + response = recorded_client.get(f"/api/decisions/{decision_id}") + assert response.status_code == 200 + assert response.json()["decision_id"] == decision_id + + def test_filter_by_category(self, recorded_client): + response = recorded_client.get("/api/decisions?category=loan_underwriting") + assert response.status_code == 200 + payload = response.json() + assert len(payload) == 1 + assert payload[0]["outcome"] == "approved" + + class TestTemporal: def test_snapshot_now(self, client): response = client.get("/api/temporal/snapshot") @@ -602,7 +660,20 @@ class TestEnrichment: def test_extract(self, client): response = client.post("/api/enrich/extract", json={"text": "Alice works at Acme Corp."}) - assert response.status_code in (200, 422, 503) + # 503 is reserved for a genuinely absent semantic_extract module; it must + # not be reachable on an install where the module imports cleanly. + # Runtime errors from the extraction stack surface as 500, not 422. + assert response.status_code in (200, 422, 500) + + def test_extract_returns_entities(self, client): + response = client.post( + "/api/enrich/extract", + json={"text": "Apple CEO Tim Cook announced record earnings in Cupertino."}, + ) + assert response.status_code == 200 + payload = response.json() + assert payload["entities"], "extraction returned no entities" + assert any("Tim Cook" in str(entity) for entity in payload["entities"]) def test_link_prediction(self, client): response = client.post("/api/enrich/links", json={"node_id": "python", "top_n": 5}) @@ -1157,3 +1228,155 @@ class TestClassifyDistance: def test_large_hop_count_is_distant(self): assert classify_path_distance(20) == "distant" + + +# --------------------------------------------------------------------------- +# Timestamp validator unit tests (no HTTP server needed) +# --------------------------------------------------------------------------- + +class TestDecisionResponseTimestampValidator: + """Unit tests for DecisionResponse._normalize_timestamp. + + These run directly against the Pydantic model, not through the HTTP stack, + so they are fast and isolated from the rest of the Explorer infrastructure. + """ + + def _make(self, ts): + from semantica.explorer.schemas import DecisionResponse + import pytest as _pytest + return DecisionResponse(decision_id="x", timestamp=ts) + + def test_none_passes_through(self): + from semantica.explorer.schemas import DecisionResponse + dr = DecisionResponse(decision_id="x", timestamp=None) + assert dr.timestamp is None + + def test_string_passes_through_unchanged(self): + from semantica.explorer.schemas import DecisionResponse + iso = "2024-08-14T10:23:45+00:00" + dr = DecisionResponse(decision_id="x", timestamp=iso) + assert dr.timestamp == iso + + def test_float_epoch_becomes_iso_string(self): + from datetime import datetime, timezone + from semantica.explorer.schemas import DecisionResponse + epoch = 1723600000.5 + dr = DecisionResponse(decision_id="x", timestamp=epoch) + assert isinstance(dr.timestamp, str) + parsed = datetime.fromisoformat(dr.timestamp) + assert abs(parsed.timestamp() - epoch) < 1.0 + + def test_int_epoch_becomes_iso_string(self): + from datetime import datetime + from semantica.explorer.schemas import DecisionResponse + epoch = 1723600000 + dr = DecisionResponse(decision_id="x", timestamp=epoch) + assert isinstance(dr.timestamp, str) + datetime.fromisoformat(dr.timestamp) + + def test_nan_raises_validation_error(self): + import math + import pytest + from pydantic import ValidationError + from semantica.explorer.schemas import DecisionResponse + with pytest.raises(ValidationError): + DecisionResponse(decision_id="x", timestamp=math.nan) + + def test_positive_inf_raises_validation_error(self): + import math + import pytest + from pydantic import ValidationError + from semantica.explorer.schemas import DecisionResponse + with pytest.raises(ValidationError): + DecisionResponse(decision_id="x", timestamp=math.inf) + + def test_negative_inf_raises_validation_error(self): + import math + import pytest + from pydantic import ValidationError + from semantica.explorer.schemas import DecisionResponse + with pytest.raises(ValidationError): + DecisionResponse(decision_id="x", timestamp=-math.inf) + + def test_dict_raises_validation_error(self): + import pytest + from pydantic import ValidationError + from semantica.explorer.schemas import DecisionResponse + with pytest.raises(ValidationError): + DecisionResponse(decision_id="x", timestamp={"$date": 1723600000}) + + def test_list_raises_validation_error(self): + import pytest + from pydantic import ValidationError + from semantica.explorer.schemas import DecisionResponse + with pytest.raises(ValidationError): + DecisionResponse(decision_id="x", timestamp=[1723600000]) + + def test_bool_raises_validation_error(self): + import pytest + from pydantic import ValidationError + from semantica.explorer.schemas import DecisionResponse + with pytest.raises(ValidationError): + DecisionResponse(decision_id="x", timestamp=True) + + def test_oserror_range_epoch_raises_validation_error(self): + import pytest + from pydantic import ValidationError + from semantica.explorer.schemas import DecisionResponse + # Milliseconds mistakenly stored where seconds were expected. + with pytest.raises(ValidationError): + DecisionResponse(decision_id="x", timestamp=1723600000000) + + def test_overflow_range_epoch_raises_validation_error(self): + import pytest + from pydantic import ValidationError + from semantica.explorer.schemas import DecisionResponse + with pytest.raises(ValidationError): + DecisionResponse(decision_id="x", timestamp=1e20) + + +# --------------------------------------------------------------------------- +# /api/enrich/extract input-size and import-boundary tests +# --------------------------------------------------------------------------- + +class TestEnrichExtractValidation: + """Tests for the input constraints and exception handling added to + POST /api/enrich/extract.""" + + def test_oversized_input_rejected_before_nlp(self, client): + """A payload exceeding the 10 000-character limit must be rejected with + 422 before any NLP work is attempted.""" + oversized = "a " * 5_001 # 10 002 characters + response = client.post("/api/enrich/extract", json={"text": oversized}) + assert response.status_code == 422 + + def test_input_at_limit_is_accepted(self, client): + """A payload at exactly the maximum length must not be rejected by the + schema validator (NLP may still fail, but the schema must accept it).""" + at_limit = "a" * 10_000 + response = client.post("/api/enrich/extract", json={"text": at_limit}) + # 503 = module missing, 500 = runtime error from the extraction stack, + # 200 = success. What must NOT happen is a schema rejection (422 from + # Pydantic due to max_length), since this input is exactly at the limit. + assert response.status_code in (200, 500, 503) + + def test_import_failure_returns_503_not_422(self, client, monkeypatch): + """A genuine ImportError on the semantic_extract import must produce 503 + (dependency unavailable), NOT 422 (extraction failed).""" + import semantica.explorer.routes.enrich as enrich_module + + def _failing_import(name, *args, **kwargs): + if "semantic_extract" in name: + raise ImportError("semantic_extract not installed") + return original_import(name, *args, **kwargs) + + import builtins + original_import = builtins.__import__ + + monkeypatch.setattr(builtins, "__import__", _failing_import) + response = client.post( + "/api/enrich/extract", + json={"text": "Apple was founded by Steve Jobs."}, + ) + assert response.status_code == 503 + assert "semantic_extract" in response.json()["detail"].lower() diff --git a/tests/semantic_extract/test_spacy_model_cache.py b/tests/semantic_extract/test_spacy_model_cache.py new file mode 100644 index 00000000..11d5313b --- /dev/null +++ b/tests/semantic_extract/test_spacy_model_cache.py @@ -0,0 +1,114 @@ +"""Tests for the process-level spaCy model cache in semantic_extract.methods. + +Before this cache existed, extract_entities_ml(), extract_relations_similarity() +and extract_relations_dependency() called spacy.load() on every invocation, so a +short sentence cost ~120 ms of model loading on top of ~2 ms of actual work. +""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from semantica.semantic_extract import methods + + +@pytest.fixture(autouse=True) +def clear_cache(): + methods.clear_spacy_model_cache() + yield + methods.clear_spacy_model_cache() + + +def _fake_spacy(load): + return SimpleNamespace(load=load, util=SimpleNamespace(is_package=lambda name: True)) + + +def test_model_loaded_once_across_calls(monkeypatch): + calls = [] + + def fake_load(name, **kwargs): + calls.append(name) + return MagicMock() + + monkeypatch.setattr(methods, "spacy", _fake_spacy(fake_load)) + + methods.load_spacy_model("en_core_web_sm") + methods.load_spacy_model("en_core_web_sm") + methods.load_spacy_model("en_core_web_sm") + + assert calls == ["en_core_web_sm"], "spacy.load should run once per model name" + + +def test_same_object_returned(monkeypatch): + sentinel = MagicMock() + monkeypatch.setattr(methods, "spacy", _fake_spacy(lambda name, **kw: sentinel)) + + assert methods.load_spacy_model("en_core_web_sm") is sentinel + assert methods.load_spacy_model("en_core_web_sm") is sentinel + + +def test_distinct_models_cached_separately(monkeypatch): + calls = [] + monkeypatch.setattr( + methods, + "spacy", + _fake_spacy(lambda name, **kw: (calls.append(name), MagicMock())[1]), + ) + + methods.load_spacy_model("en_core_web_sm") + methods.load_spacy_model("en_core_web_lg") + methods.load_spacy_model("en_core_web_sm") + + assert calls == ["en_core_web_sm", "en_core_web_lg"] + + +def test_load_errors_propagate_and_are_not_cached(monkeypatch): + """Callers rely on OSError to trigger their fallback path.""" + attempts = [] + + def failing_load(name, **kwargs): + attempts.append(name) + raise OSError(f"Can't find model '{name}'") + + monkeypatch.setattr(methods, "spacy", _fake_spacy(failing_load)) + + with pytest.raises(OSError): + methods.load_spacy_model("en_core_web_missing") + with pytest.raises(OSError): + methods.load_spacy_model("en_core_web_missing") + + assert len(attempts) == 2, "a failed load must not populate the cache" + + +def test_cache_ignores_entries_from_a_replaced_spacy_module(monkeypatch): + """Patching methods.spacy must not hand back a model from the old module. + + Existing tests patch this attribute with a mock and assert on load calls, so + a cache keyed on model name alone would leak objects across those tests. + """ + first = MagicMock() + monkeypatch.setattr(methods, "spacy", _fake_spacy(lambda name, **kw: first)) + assert methods.load_spacy_model("en_core_web_sm") is first + + second = MagicMock() + monkeypatch.setattr(methods, "spacy", _fake_spacy(lambda name, **kw: second)) + assert methods.load_spacy_model("en_core_web_sm") is second + + +def test_extract_entities_ml_reuses_the_cached_model(monkeypatch): + calls = [] + + def fake_load(name, **kwargs): + calls.append(name) + nlp = MagicMock() + nlp.return_value = SimpleNamespace(ents=[]) + return nlp + + monkeypatch.setattr(methods, "spacy", _fake_spacy(fake_load)) + monkeypatch.setattr(methods, "SPACY_AVAILABLE", True) + + methods.extract_entities_ml("Alice works at Acme Corp.") + methods.extract_entities_ml("Bob works at Globex.") + + assert len(calls) == 1, "the model should be loaded once, not once per call"