retain: let a store that owns its vector index skip the Postgres ANN pass - #3478
Draft
nicoloboschi wants to merge 2 commits into
Draft
retain: let a store that owns its vector index skip the Postgres ANN pass#3478nicoloboschi wants to merge 2 commits into
nicoloboschi wants to merge 2 commits into
Conversation
…pass The default SQL stores have no standing kNN index, so retain runs a pgvector ANN pass (_run_final_semantic_ann) over the committed units to populate memory_links, which the recall graph arm then reads. A store that keeps its own vector index can serve those same neighbours itself (at read time, or from links it derived at fold time), so for it the pass is redundant: it scans a memory_units table that is empty for its banks and writes memory_links rows nothing on its read path consults. Add a backend-agnostic capability flag, derives_semantic_links_internally (with a per-bank derives_semantic_links_internally_for), mirroring the existing writes_memory_rows_in_sql_for / owns_document_store_for pattern, and gate the final ANN pass on it. Default False, so there is no behaviour change for existing stores. Tests included.
The gate only covered the streaming full-retain path, so a store that owns its vector index still paid for the Postgres semantic graph everywhere else: delta retain and bank import ran the pgvector ANN probe in Phase 1, and Phase 2 wrote the within-batch similarities (computed in Python, so skipping the probe alone does not stop them) into memory_links — rows the store's read path never consults. Move each gate to its single choke point instead: the ANN probe in _pre_resolve_phase1, the memory_links write in _insert_facts_and_links, and the post-commit pass inside _run_final_semantic_ann. Every retain path (full, delta, import) now inherits the skip and no caller can reintroduce the query. Also state the obligation the flag carries — a store that sets it True must serve the semantic neighbours from its own graph_retriever, since there will be no semantic memory_links rows for the SQL graph arm to find — and drop the claim that the pass "scans" memory_units: for a store whose facts are not in that table the pass currently degenerates to a lookup that finds none of its units and logs "(unexpected)" on every retain. Tests now pin the three gates through the orchestrator (each fails if its gate is removed) and cover the per-bank answer, replacing a unit test that asserted only the class attribute.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a backend-agnostic capability flag,
derives_semantic_links_internally(with a per-bankderives_semantic_links_internally_for), that lets a store which owns its own vector index skip the retain-time Postgres kNN work that populatesmemory_links.Why
The default SQL stores have no standing kNN index, so retain finds each unit's semantic neighbours with a pgvector ANN probe and materializes them as
semanticmemory_linksrows, which the SQL graph arm reads back at recall time.A store that keeps its own vector index can answer the same question from that index (at read time, or from links it derived at fold time), so the Postgres work is redundant for it — and for a store whose memories don't live in
memory_unitsat all it is worse than redundant: it queries a table holding none of its rows and writesmemory_linksrows nothing on its read path consults.How
The flag is honoured at three choke points, so every retain path — streaming (full), delta, and bank import — inherits the skip and no caller can reintroduce the query by forgetting to pass a flag down:
engine/retain/orchestrator.py_pre_resolve_phase1— skips the ANN probe (this is the probe delta retain and bank import run)._insert_facts_and_links— skips thememory_linkswrite. Necessary on its own: the within-batch similarities are computed in Python, so skipping the probe alone would still leave rows behind._run_final_semantic_ann— skips streaming retain's post-commit pass, which reaches pgvector directly rather than through Phase 1.engine/memories/base.py— adds the class attrderives_semantic_links_internally: bool = Falseandderives_semantic_links_internally_for(bank_id), mirroring the existingwrites_memory_rows_in_sql_for/owns_document_store_forpattern.Setting the flag True is a promise, not just an optimization, and the docstring says so: the store's
graph_retriever/graph_direct_linksmust serve the semantic neighbours itself, because afterwards there are nosemanticrows inmemory_linksfor the SQL graph arm to find.Tests
tests/test_retain_orchestrator_mapping.pydrives all three gates through the orchestrator — each test fails if its gate is removed — and covers the per-bank answer (one store, one bank it owns the index for and one it leaves in SQL).tests/test_memories_extension.pykeeps the default-False assertion.Default is
False, so there is no behaviour change for existing stores.