feat(lexical-graph): invoke graph_store init hook#185
feat(lexical-graph): invoke graph_store init hook#185iansrobinson merged 1 commit intoawslabs:mainfrom
Conversation
|
LexicalGraphQueryEngine creates a MultiTenantGraphStore via the same factory pattern but doesn't call init(). The indexes created by init() are needed for queries too — should the query engine also invoke the hook? |
| tenant_id = to_tenant_id(tenant_id) | ||
|
|
||
| self.graph_store = MultiTenantGraphStore.wrap(GraphStoreFactory.for_graph_store(graph_store), tenant_id) | ||
| self.graph_store.init() |
There was a problem hiding this comment.
self.graph_store.init() is called inside init() with no exception handling. Both Neo4jGraphStore.init() and the new FalkorDBDatabaseClient.init() (PR #186) perform network I/O. If the database is unreachable at construction time, the exception prevents self.tenant_id, self.extraction_dir, and the extraction pipeline from being set — leaving the object in a partially constructed state. This is a behavioral change for users who construct the index before the database is available.
There was a problem hiding this comment.
self.graph_store.init() is called inside init() with no exception handling. Both Neo4jGraphStore.init() and the new FalkorDBDatabaseClient.init() (PR #186) perform network I/O. If the database is unreachable at construction time, the exception prevents self.tenant_id, self.extraction_dir, and the extraction pipeline from being set — leaving the object in a partially constructed state. This is a behavioral change for users who construct the index before the database is available.
This is intentional fail-fast behavior: if a backend’s init() cannot complete (for example, DB unreachable), construction fails immediately instead of deferring failure to first use. Backends that do not need setup are unaffected because base GraphStore.init() is a no-op. I also added a unit test documenting this expected behavior by asserting the constructor propagates graph_store.init() exceptions.
| assert extraction_config.extraction_llm == llm_cache No newline at end of file | ||
| assert extraction_config.extraction_llm == llm_cache | ||
|
|
||
| class TestLexicalGraphIndex: |
There was a problem hiding this comment.
Would be worth adding a test for the failure case (graph_store.init.side_effect = Exception(...)) to document the expected behavior.
There was a problem hiding this comment.
Good point. I added a failure-path unit test that sets graph_store.init.side_effect and asserts LexicalGraphIndex(...) propagates the exception. This documents the expected fail-fast constructor behavior.
zollie
force-pushed
the
feat/graph-store-init-hook-upstream-pr
branch
from
116ded0 to
765c991
Compare
Agreed. I updated LexicalGraphQueryEngine to invoke graph_store.init() during construction, so traversal-based, semantic-guided, and direct constructor paths all follow the same lifecycle hook. This ensures query-only startup also gets index/bootstrap behavior (for backends that implement it). |
|
Thank you @zollie for the contribution. Sorry for the delay in merging |
4 participants
Invoke existing graph_store.init() lifecycle hook during LexicalGraphIndex initialization, so backend bootstrap logic (if implemented) runs automatically. No API change; no-op for stores that don’t override init().
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.