diff --git a/CHANGELOG.md b/CHANGELOG.md index d263f9d5..b23ed156 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Corrected during review**: `add_temporal_edge`/`create_temporal_snapshot` docstrings overclaimed numeric-timestamp support; `_parse_time()` only special-cases `str` and `datetime`, falling back to a bare `str()` cast for anything else (not true numeric parsing). Narrowed to "datetime or ISO-formatted string" - **Fixed along the way**: `build()`'s `**options` documented a default only for `extract`; `extract_relations`, `extract_triplets`, `ner_method`, `relation_method`, and `triplet_method` all have concrete defaults in `_extract_from_text()` (`True`, `True`, `"llm"`, `"llm"`, `"llm"`) that were left unstated, inconsistent with CONTRIBUTING.md's own docstring example of noting defaults inline - `python -m pytest tests/kg/test_kg.py tests/kg/test_graph_builder_external.py -q`: 45 passed +- **`GraphBuilder` raw-text extraction now defaults to local extractors instead of LLM extraction** (closes #930) by @dex0shubham + - `GraphBuilder._extract_from_text()` defaulted `ner_method`, `relation_method`, and `triplet_method` to `"llm"`, and ran relation extraction unconditionally (`extract_relations` defaulted to `True`) — all four contradicting the defaults documented in the `build()` docstring at the time (`"ml"` / `"pattern"` / `False`), and diverging from the standalone extractors (`NERExtractor` defaults to `method="ml"`, `RelationExtractor` and `TripletExtractor` to `method="pattern"`). The practical effect was that any raw-text `build()` call silently required a configured provider, an API key, and network access + - Defaults are now `ner_method="ml"`, `relation_method="pattern"`, `triplet_method="pattern"`, and `extract_relations=False`, matching the docstring. LLM extraction remains fully available and is now opt-in + - **To restore the previous behaviour**, pass the methods explicitly: + ```python + builder.build( + sources, + ner_method="llm", + relation_method="llm", + triplet_method="llm", + extract_relations=True, + ) + ``` + - #878 landed in the meantime and resolved the same mismatch in the opposite direction, documenting the LLM values (`"llm"` / `"llm"` / `"llm"`, `extract_relations: True`) as the contract. Per the decision on #930 the code is the side that changes, so those docstring defaults are corrected here to `"ml"` / `"pattern"` / `"pattern"` / `False`, keeping #878's formatting + - Removed the stale `# Default to LLM methods as per requirement` comment, which read as an intentional decision but did not match the documented contract + - **Fixed along the way**: `_extract_from_text()` constructed a fresh extractor for every text, and `NERExtractor.__init__` loads its spaCy model eagerly when the method includes `"ml"` — so with the new default, a multi-document build would have reloaded the model once per source. Extractors are now built once per `(kind, method)` and reused for the lifetime of the builder, via `GraphBuilder._get_extractor()`. This path was previously unreachable by default because the old `"llm"` default never touched spaCy + - **Fixed along the way**: `_extract_from_text()` never forwarded its extracted relations to triplet extraction — it passed only `entities=`, so `TripletExtractor` re-derived relations itself (via a method taken from `triplet_method`) whenever `relations is None`, duplicating work and producing triplets that could disagree with the relations already extracted using `relation_method`. Relations are now passed through as `relations=`; when relation extraction is disabled or fails, `None` is forwarded and `TripletExtractor` keeps its existing self-derivation behaviour + - **Fixed along the way**: `GraphBuilder._extraction_stats` was only initialised inside `build()`, so calling `_extract_from_text()` directly raised an `AttributeError` that the extraction path's broad `except` swallowed and reported as `"Entity extraction failed"`. It is now seeded in `__init__` as well; `build()` still resets it per run + - New regression coverage in `tests/kg/test_graph_builder_extraction_defaults.py` pinning all four defaults, verifying that no default resolves to `"llm"`, confirming explicit LLM opt-in still routes correctly, asserting extractors are constructed once across repeated texts, covering fallback method lists (e.g. `ner_method=["pattern", "ml"]`) for all three extractors, asserting relations are forwarded to triplet extraction (and that `None` is forwarded when relation extraction is disabled or fails), and running the real default path end to end with no provider mocked. Verified to fail against the pre-fix code + - Full `kg` suite: 473 passed ### Fixed diff --git a/semantica/kg/graph_builder.py b/semantica/kg/graph_builder.py index 6ed783f9..314b923d 100644 --- a/semantica/kg/graph_builder.py +++ b/semantica/kg/graph_builder.py @@ -23,7 +23,7 @@ License: MIT """ from datetime import datetime -from typing import Any, Dict, List, Optional, Union +from typing import Any, Dict, List, Optional, Tuple, Union import time @@ -90,6 +90,18 @@ class GraphBuilder: self.version_snapshots = version_snapshots self.graph_store = graph_store self.config = kwargs # Store additional config for extractors + # Extractors are reused across texts: NERExtractor loads its spaCy model + # eagerly in __init__, so constructing one per text would reload the + # model on every source in a multi-document build. + self._extractor_cache: Dict[Tuple[str, Any], Any] = {} + # build() resets these per run; seed them here so _extract_from_text + # is usable on its own instead of raising an AttributeError that the + # broad except in the extraction path silently swallows. + self._extraction_stats: Dict[str, int] = { + "extracted_entities": 0, + "extracted_relations": 0, + "extracted_triplets": 0, + } # Initialize logging from ..utils.logging import get_logger @@ -228,6 +240,28 @@ class GraphBuilder: # Unknown type pass + def _get_extractor( + self, kind: str, extractor_cls, method: Union[str, List[str]] + ): + """Return a cached extractor for this method, building it on first use. + + Extractors hold no per-text state but are expensive to construct — + ``NERExtractor(method="ml")`` loads a spaCy model in ``__init__``. + Keying on kind and method is enough because ``self.config`` is fixed + for the lifetime of the builder. + + Args: + kind: Extractor role, one of ``"ner"``, ``"relation"``, ``"triplet"``. + extractor_cls: Extractor class to construct on a cache miss. + method: A method name, or a list of them for fallback ordering. + Lists are converted to tuples for the cache key only; the + extractor still receives the original value. + """ + key = (kind, tuple(method) if isinstance(method, list) else method) + if key not in self._extractor_cache: + self._extractor_cache[key] = extractor_cls(method=method, **self.config) + return self._extractor_cache[key] + def _extract_from_text(self, text: str, all_entities: List[Any], all_relationships: List[Any], **options): """Helper to extract knowledge from text using configured methods.""" if not options.get("extract", True): @@ -237,15 +271,17 @@ class GraphBuilder: from ..semantic_extract.relation_extractor import RelationExtractor from ..semantic_extract.triplet_extractor import TripletExtractor - # Default to LLM methods as per requirement - ner_method = options.get("ner_method", "llm") - relation_method = options.get("relation_method", "llm") - triplet_method = options.get("triplet_method", "llm") + # Local extractors by default — raw-text build() must not require a + # provider, API key, or network access. Pass ner_method="llm" (and the + # relation/triplet equivalents) to opt into LLM extraction. + ner_method = options.get("ner_method", "ml") + relation_method = options.get("relation_method", "pattern") + triplet_method = options.get("triplet_method", "pattern") self.logger.info(f"Extracting knowledge from text ({len(text)} chars) using {ner_method}...") # 1. Extract Entities - ner = NERExtractor(method=ner_method, **self.config) + ner = self._get_extractor("ner", NERExtractor, ner_method) try: entities = ner.extract_entities(text, **options) extracted_count = len(entities) @@ -258,8 +294,16 @@ class GraphBuilder: entities = [] # 2. Extract Relations (if requested) - if options.get("extract_relations", True): - rel_extractor = RelationExtractor(method=relation_method, **self.config) + # Stays None when relation extraction is skipped or fails, which lets + # TripletExtractor derive its own relations as before. When we do have + # them, they are forwarded below so triplets reuse the relations + # extracted with relation_method rather than re-deriving via + # triplet_method. + relations = None + if options.get("extract_relations", False): + rel_extractor = self._get_extractor( + "relation", RelationExtractor, relation_method + ) try: # Pass entities if available to help relation extraction relations = rel_extractor.extract_relations(text, entities=entities, **options) @@ -273,9 +317,13 @@ class GraphBuilder: # 3. Extract Triplets (if requested) if options.get("extract_triplets", True): - trip_extractor = TripletExtractor(method=triplet_method, **self.config) + trip_extractor = self._get_extractor( + "triplet", TripletExtractor, triplet_method + ) try: - triplets = trip_extractor.extract_triplets(text, entities=entities, **options) + triplets = trip_extractor.extract_triplets( + text, entities=entities, relations=relations, **options + ) extracted_count = len(triplets) self._extraction_stats["extracted_triplets"] += extracted_count self.logger.info(f"Extracted {extracted_count} triplets") @@ -307,20 +355,24 @@ class GraphBuilder: raw string or ``{"text": ...}`` dict is passed as a source (default: ``True``). - ``extract_relations`` (bool): Whether to extract relations - during text extraction (default: ``True``). + during text extraction (default: ``False``). - ``extract_triplets`` (bool): Whether to extract triplets during text extraction (default: ``True``). - ``ner_method`` (str): NER backend used for text extraction - (e.g. ``"ml"``, ``"pattern"``, ``"llm"``; default: ``"llm"``). + (e.g. ``"ml"``, ``"pattern"``, ``"llm"``; default: ``"ml"``). - ``relation_method`` (str): Relation-extraction backend - (e.g. ``"pattern"``, ``"llm"``; default: ``"llm"``). + (e.g. ``"pattern"``, ``"llm"``; default: ``"pattern"``). - ``triplet_method`` (str): Triplet-extraction backend - (e.g. ``"pattern"``, ``"llm"``; default: ``"llm"``). + (e.g. ``"pattern"``, ``"llm"``; default: ``"pattern"``). - ``entity_resolver``: An :class:`EntityResolver` instance that overrides the one configured on the builder. - ``relationships`` (list): An explicit list of relationships to include in addition to those found in *sources*. + Raw-text extraction uses local extractors by default and needs no + provider or API key. To use LLM extraction, pass the methods + explicitly, e.g. ``ner_method="llm"``. + Returns: A dictionary containing the graph's ``entities``, ``relationships``, and build ``metadata``. diff --git a/tests/kg/test_graph_builder_extraction_defaults.py b/tests/kg/test_graph_builder_extraction_defaults.py new file mode 100644 index 00000000..fdc2a0e2 --- /dev/null +++ b/tests/kg/test_graph_builder_extraction_defaults.py @@ -0,0 +1,262 @@ +"""Pins GraphBuilder's raw-text extraction defaults to the documented values. + +Regression guard for #930: `_extract_from_text` defaulted to LLM extraction for +all three methods and ran relation extraction unconditionally, both of which +contradicted the `build()` docstring and silently required a provider and API +key for any raw-text build. +""" + +import unittest +from unittest.mock import patch + +from semantica.kg.graph_builder import GraphBuilder + + +class TestGraphBuilderExtractionDefaults(unittest.TestCase): + + def setUp(self): + self.ner_patcher = patch( + "semantica.semantic_extract.ner_extractor.NERExtractor" + ) + self.rel_patcher = patch( + "semantica.semantic_extract.relation_extractor.RelationExtractor" + ) + self.trip_patcher = patch( + "semantica.semantic_extract.triplet_extractor.TripletExtractor" + ) + self.NER = self.ner_patcher.start() + self.Rel = self.rel_patcher.start() + self.Trip = self.trip_patcher.start() + self.addCleanup(self.ner_patcher.stop) + self.addCleanup(self.rel_patcher.stop) + self.addCleanup(self.trip_patcher.stop) + + self.NER.return_value.extract_entities.return_value = [] + self.Rel.return_value.extract_relations.return_value = [] + self.Trip.return_value.extract_triplets.return_value = [] + + self.builder = GraphBuilder(merge_entities=False, resolve_conflicts=False) + + def _extract(self, **options): + self.builder._extract_from_text( + "Apple Inc. was founded in 1976.", [], [], **options + ) + + def test_ner_method_defaults_to_ml(self): + self._extract() + self.assertEqual(self.NER.call_args.kwargs["method"], "ml") + + def test_triplet_method_defaults_to_pattern(self): + self._extract() + self.assertEqual(self.Trip.call_args.kwargs["method"], "pattern") + + def test_relation_extraction_is_off_by_default(self): + self._extract() + self.Rel.assert_not_called() + + def test_relation_method_defaults_to_pattern_when_enabled(self): + self._extract(extract_relations=True) + self.assertEqual(self.Rel.call_args.kwargs["method"], "pattern") + + def test_no_extractor_defaults_to_llm(self): + """No raw-text default may require a provider or API key.""" + self._extract(extract_relations=True) + extractors = ( + ("ner", self.NER), + ("relation", self.Rel), + ("triplet", self.Trip), + ) + for name, mock_cls in extractors: + with self.subTest(extractor=name): + self.assertNotEqual(mock_cls.call_args.kwargs["method"], "llm") + + def test_llm_extraction_is_still_available_explicitly(self): + self._extract( + ner_method="llm", + relation_method="llm", + triplet_method="llm", + extract_relations=True, + ) + self.assertEqual(self.NER.call_args.kwargs["method"], "llm") + self.assertEqual(self.Rel.call_args.kwargs["method"], "llm") + self.assertEqual(self.Trip.call_args.kwargs["method"], "llm") + + +class TestGraphBuilderExtractorReuse(unittest.TestCase): + """Extractors must be built once per method, not once per text. + + `NERExtractor.__init__` loads its spaCy model eagerly, so with the `"ml"` + default a per-text construction would reload the model for every source in + a multi-document build. + """ + + def setUp(self): + self.ner_patcher = patch( + "semantica.semantic_extract.ner_extractor.NERExtractor" + ) + self.NER = self.ner_patcher.start() + self.addCleanup(self.ner_patcher.stop) + self.NER.return_value.extract_entities.return_value = [] + + self.builder = GraphBuilder(merge_entities=False, resolve_conflicts=False) + + def test_ner_extractor_built_once_across_texts(self): + for i in range(5): + self.builder._extract_from_text(f"Document {i}.", [], []) + self.assertEqual(self.NER.call_count, 1) + + def test_distinct_methods_get_distinct_extractors(self): + self.builder._extract_from_text("a", [], []) + self.builder._extract_from_text("b", [], [], ner_method="pattern") + self.builder._extract_from_text("c", [], []) + self.assertEqual(self.NER.call_count, 2) + + +class TestGraphBuilderForwardsRelationsToTriplets(unittest.TestCase): + """Relations extracted with relation_method must reach triplet extraction. + + `TripletExtractor` re-derives relations itself when `relations is None`, + using a method derived from `triplet_method` — so not forwarding them both + duplicates work and can produce triplets inconsistent with the relations + already extracted. + """ + + def setUp(self): + self.ner_patcher = patch( + "semantica.semantic_extract.ner_extractor.NERExtractor" + ) + self.rel_patcher = patch( + "semantica.semantic_extract.relation_extractor.RelationExtractor" + ) + self.trip_patcher = patch( + "semantica.semantic_extract.triplet_extractor.TripletExtractor" + ) + self.NER = self.ner_patcher.start() + self.Rel = self.rel_patcher.start() + self.Trip = self.trip_patcher.start() + self.addCleanup(self.ner_patcher.stop) + self.addCleanup(self.rel_patcher.stop) + self.addCleanup(self.trip_patcher.stop) + + self.NER.return_value.extract_entities.return_value = [] + self.Trip.return_value.extract_triplets.return_value = [] + + self.builder = GraphBuilder(merge_entities=False, resolve_conflicts=False) + + def _triplet_kwargs(self): + return self.Trip.return_value.extract_triplets.call_args.kwargs + + def test_extracted_relations_are_forwarded(self): + sentinel = [object()] + self.Rel.return_value.extract_relations.return_value = sentinel + + self.builder._extract_from_text("x", [], [], extract_relations=True) + + self.assertIs(self._triplet_kwargs()["relations"], sentinel) + + def test_relations_is_none_when_extraction_disabled(self): + """Default path keeps TripletExtractor's own relation derivation.""" + self.builder._extract_from_text("x", [], []) + + self.assertIsNone(self._triplet_kwargs()["relations"]) + self.Rel.assert_not_called() + + def test_relations_is_none_when_extraction_fails(self): + self.Rel.return_value.extract_relations.side_effect = RuntimeError("boom") + + self.builder._extract_from_text("x", [], [], extract_relations=True) + + self.assertIsNone(self._triplet_kwargs()["relations"]) + + +class TestGraphBuilderFallbackMethodLists(unittest.TestCase): + """All three extractors accept a list of methods for fallback ordering. + + The extractor cache must key on something hashable, or passing a list + raises `TypeError: unhashable type: 'list'` before extraction even starts. + """ + + def setUp(self): + self.ner_patcher = patch( + "semantica.semantic_extract.ner_extractor.NERExtractor" + ) + self.rel_patcher = patch( + "semantica.semantic_extract.relation_extractor.RelationExtractor" + ) + self.trip_patcher = patch( + "semantica.semantic_extract.triplet_extractor.TripletExtractor" + ) + self.NER = self.ner_patcher.start() + self.Rel = self.rel_patcher.start() + self.Trip = self.trip_patcher.start() + self.addCleanup(self.ner_patcher.stop) + self.addCleanup(self.rel_patcher.stop) + self.addCleanup(self.trip_patcher.stop) + + self.NER.return_value.extract_entities.return_value = [] + self.Rel.return_value.extract_relations.return_value = [] + self.Trip.return_value.extract_triplets.return_value = [] + + self.builder = GraphBuilder(merge_entities=False, resolve_conflicts=False) + + def test_list_method_does_not_raise(self): + self.builder._extract_from_text( + "x", [], [], ner_method=["pattern", "ml"], extract_triplets=False + ) + self.assertEqual(self.NER.call_args.kwargs["method"], ["pattern", "ml"]) + + def test_list_methods_accepted_for_every_extractor(self): + self.builder._extract_from_text( + "x", + [], + [], + ner_method=["pattern", "ml"], + relation_method=["pattern", "cooccurrence"], + triplet_method=["pattern", "rules"], + extract_relations=True, + ) + self.assertEqual(self.NER.call_args.kwargs["method"], ["pattern", "ml"]) + self.assertEqual( + self.Rel.call_args.kwargs["method"], ["pattern", "cooccurrence"] + ) + self.assertEqual(self.Trip.call_args.kwargs["method"], ["pattern", "rules"]) + + def test_equal_lists_reuse_one_extractor(self): + for _ in range(3): + self.builder._extract_from_text( + "x", [], [], ner_method=["pattern", "ml"], extract_triplets=False + ) + self.assertEqual(self.NER.call_count, 1) + + def test_different_lists_get_different_extractors(self): + self.builder._extract_from_text( + "x", [], [], ner_method=["pattern", "ml"], extract_triplets=False + ) + self.builder._extract_from_text( + "x", [], [], ner_method=["ml", "pattern"], extract_triplets=False + ) + self.assertEqual(self.NER.call_count, 2) + + +class TestGraphBuilderDefaultsRunOffline(unittest.TestCase): + """The default raw-text path must work with no provider and no network.""" + + def test_default_build_needs_no_provider(self): + builder = GraphBuilder(merge_entities=False, resolve_conflicts=False) + entities, relationships = [], [] + + # No mocks: this runs the real ml/pattern extractors end to end. If any + # default resolved to "llm", this would attempt a provider call. + with patch("semantica.semantic_extract.providers.create_provider") as provider: + builder._extract_from_text( + "Apple Inc. was founded by Steve Jobs in 1976.", + entities, + relationships, + ) + + provider.assert_not_called() + self.assertIsInstance(entities, list) + + +if __name__ == "__main__": + unittest.main()