Let a retired memory keep its vector and its place in the present
The second review round of p2-4 measured three behaviour defects behind the
as_of feature it had just accepted, and all three came from one mistake: the
code asked "is this memory current?" in three places and answered differently.
- A memory that has closed kept no vector at all, so the dense leg could only
exclude an old fact, never recall it. Vectors now follow existence, not
currency; `_allowed` decides who may see one.
- "Open right now" meant `valid_to IS NULL` in the view and in the words leg
but `valid_to IS NULL OR valid_to > now` in the vector leg, so a supersede
booked for next year hid a fact that is still true today. All three now read
the same rule; schema v12 records the change and the upgrade drops the old
view, since CREATE VIEW IF NOT EXISTS would have kept the old body.
- The dense as_of test was名不副实: dense is off by default, so it passed while
the vector side had no coverage. It now turns dense on with a stand-in model
and pins both `VectorIndex.search(as_of=…)` and the plumbing in retrieval.
Five mutations, each caught by exactly one test: the dense as_of branch, the
retrieval hand-off, the three vector reads, the words-leg currency, the view.
Record side: the round-2 line quoted a D-list I had misread out of the
reviewer's PASS tables (cf2de31 fixed those, and they were its precision notes,
not its findings), the stale line-number parenthetical, an acceptance grep that
scanned CHECKLIST's own quotations, and a Gaps.md tally that disagreed with its
own table (8 closed, 3 closed with reservations, 1 partial).
Gates: 411 passed; ruff, filesize, audit 33/2/6 clean; pyright 0 on the six
touched files; mypy cold cache Success on the same six; scripts/key_test.sh
exit 0 (6 PASS, 1 NOT MEASURED).
This commit is contained in:
7 files changed
+156
-40
No files matched your search
@@ -16,7 +16,7 @@ import sqlite3
|
||||
from .storage import token_equivalent, visual_token_equivalent
|
||||
from .text_index import index_text
|
||||
|
||||
SCHEMA_VERSION = 11
|
||||
SCHEMA_VERSION = 12
|
||||
|
||||
ATOM_COLUMNS_V2 = """
|
||||
atom_id TEXT PRIMARY KEY,
|
||||
@@ -188,7 +188,7 @@ tokenize = 'unicode61'
|
||||
VIEWS = """
|
||||
CREATE VIEW IF NOT EXISTS v_atoms_current AS
|
||||
SELECT * FROM atoms
|
||||
WHERE valid_to IS NULL
|
||||
WHERE (valid_to IS NULL OR valid_to > strftime('%Y-%m-%dT%H:%M:%f+00:00', 'now'))
|
||||
AND archived_at IS NULL
|
||||
AND (valid_from IS NULL OR valid_from <= strftime('%Y-%m-%dT%H:%M:%f+00:00', 'now'))
|
||||
AND (retention_until IS NULL OR retention_until >= strftime('%Y-%m-%dT%H:%M:%f+00:00', 'now'));
|
||||
@@ -328,8 +328,8 @@ def apply(connection: sqlite3.Connection) -> None:
|
||||
for name, definition in _NEW_HOLDER_COLUMNS:
|
||||
if name not in existing_holders:
|
||||
connection.execute(f"ALTER TABLE holders ADD COLUMN {name} {definition}")
|
||||
if version < 10:
|
||||
# CREATE VIEW IF NOT EXISTS keeps the old body, and v10 changed which rows are current, so an upgraded store must be told to look again.
|
||||
if version < 12:
|
||||
# CREATE VIEW IF NOT EXISTS keeps the old body, and v10 and v12 each changed which rows are current, so an upgraded store must be told to look again.
|
||||
connection.execute("DROP VIEW IF EXISTS v_atoms_current")
|
||||
|
||||
connection.executescript(VIEWS)
|
||||
|
||||
@@ -68,7 +68,8 @@ def search_atoms(
|
||||
WHERE atom_fts MATCH ?
|
||||
AND (
|
||||
(? IS NULL
|
||||
AND a.valid_to IS NULL
|
||||
AND (a.valid_to IS NULL
|
||||
OR a.valid_to > strftime('%Y-%m-%dT%H:%M:%f+00:00', 'now'))
|
||||
AND a.archived_at IS NULL
|
||||
AND (a.valid_from IS NULL
|
||||
OR a.valid_from <= strftime('%Y-%m-%dT%H:%M:%f+00:00', 'now'))
|
||||
|
||||
@@ -4,7 +4,8 @@ The first version kept the vectors in `cache/dense_index.npy` plus a JSON list o
|
||||
That pair had no owner: nothing noticed when an atom was added, changed or forgotten, so a
|
||||
search answered from a snapshot of the past and `forget` had to delete the whole file. Here the
|
||||
vectors sit in the same SQLite file as the atoms, in a `vec0` table the `sqlite-vec` extension
|
||||
answers, and the only rule the index has is "one row per current atom, re-encode what changed".
|
||||
answers, and the only rule the index has is "one row per atom that still exists and has words,
|
||||
re-encode what changed".
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -58,7 +59,7 @@ def load_extension(connection: sqlite3.Connection) -> None:
|
||||
|
||||
|
||||
class VectorIndex:
|
||||
"""One float32 vector per current atom: the writer keeps it, the reader asks it questions."""
|
||||
"""One float32 vector per memory that has words: the writer keeps it, the reader asks it questions."""
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
@@ -121,7 +122,9 @@ class VectorIndex:
|
||||
"""Bring the index in step with memories as they are written.
|
||||
|
||||
Nothing on the search path rebuilds, so this is the half that keeps "written now,
|
||||
findable now" true; a memory that has closed or been forgotten loses its vector here too.
|
||||
findable now" true. A forgotten memory loses its vector here; a memory that has merely
|
||||
closed keeps one, because a question about the past still has to reach it — the currency
|
||||
rule in `_allowed` decides who may see it, not the presence of a vector.
|
||||
"""
|
||||
wanted = [atom_id for atom_id in atom_ids if atom_id]
|
||||
if not wanted or not self.settings.dense_embeddings_enabled:
|
||||
@@ -382,12 +385,16 @@ class VectorIndex:
|
||||
return removed
|
||||
|
||||
def _current(self, connection: sqlite3.Connection, atom_ids: list[str]) -> list[tuple[str, Any]]:
|
||||
"""The written memories that still deserve a vector: open, and with words."""
|
||||
"""The written memories that still deserve a vector: ones whose row exists, with words.
|
||||
|
||||
Currency is not asked here on purpose. A superseded memory keeps its vector so `as_of`
|
||||
can answer from it; the search path is where the window gets applied.
|
||||
"""
|
||||
from .row_codec import row_to_atom
|
||||
|
||||
rows = connection.execute(
|
||||
"""
|
||||
SELECT * FROM v_atoms_current
|
||||
SELECT * FROM atoms
|
||||
WHERE atom_id IN (SELECT value FROM json_each(?)) AND text IS NOT NULL AND text != ''
|
||||
""",
|
||||
(to_json(atom_ids),),
|
||||
@@ -425,7 +432,7 @@ class VectorIndex:
|
||||
)
|
||||
|
||||
def _stale(self, connection: sqlite3.Connection) -> list[tuple[str, Any]]:
|
||||
"""Current memories the index does not hold, or holds from an older version of the memory.
|
||||
"""Memories the index does not hold, or holds from an older version of the memory.
|
||||
|
||||
A memory with no words is the visual index's job, so it never reaches this table. The
|
||||
stored stamps are copied into a temp table first: a vec0 table has no index on atom_id,
|
||||
@@ -441,7 +448,7 @@ class VectorIndex:
|
||||
)
|
||||
rows = connection.execute(
|
||||
"""
|
||||
SELECT a.* FROM v_atoms_current AS a
|
||||
SELECT a.* FROM atoms AS a
|
||||
LEFT JOIN dense_seen AS d ON d.atom_id = a.atom_id
|
||||
WHERE a.text IS NOT NULL AND a.text != ''
|
||||
AND (d.atom_id IS NULL OR d.stamp IS NULL OR d.stamp != atom_stamp(a.ingested_at, a.text))
|
||||
@@ -457,14 +464,14 @@ def atom_stamp(ingested_at: str, text: str | None) -> str:
|
||||
|
||||
|
||||
def expected_vectors(database: Database) -> int:
|
||||
"""Memories this index is meant to hold: the open ones that have words."""
|
||||
"""Memories this index is meant to hold: the ones whose row exists and has words."""
|
||||
with database.connect() as connection:
|
||||
return _expected(connection)
|
||||
|
||||
|
||||
def _expected(connection: sqlite3.Connection) -> int:
|
||||
row = connection.execute(
|
||||
"SELECT count(*) AS n FROM v_atoms_current WHERE text IS NOT NULL AND text != ''"
|
||||
"SELECT count(*) AS n FROM atoms WHERE text IS NOT NULL AND text != ''"
|
||||
).fetchone()
|
||||
return int(row["n"])
|
||||
|
||||
|
||||
@@ -8,8 +8,10 @@ about, never who is allowed to see the answer.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import hashlib
|
||||
from datetime import UTC, datetime, timedelta
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
@@ -59,7 +61,9 @@ def test_a_retired_memory_answers_for_the_time_it_was_true(client: TestClient) -
|
||||
superseded = client.post("/api/memory/supersede", json={"atom_id": atom_id, "text": CAR})
|
||||
assert superseded.status_code == 200, superseded.text
|
||||
assert hits(client, {"query": "钥匙"}) == [CAR], "asked now, the current memory answers"
|
||||
asked_then = hits(client, {"query": "钥匙", "filters": {"as_of": (week_ago + timedelta(days=1)).isoformat()}})
|
||||
asked_then = hits(
|
||||
client, {"query": "钥匙", "filters": {"as_of": (week_ago + timedelta(days=1)).isoformat()}}
|
||||
)
|
||||
assert asked_then == [DRAWER], "asked about last week, the retired memory answers"
|
||||
|
||||
|
||||
@@ -71,7 +75,9 @@ def test_a_memory_that_had_not_happened_yet_does_not_answer(client: TestClient)
|
||||
)
|
||||
assert response.status_code == 200, response.text
|
||||
assert hits(client, {"query": "钥匙"}) == [CAR]
|
||||
assert hits(client, {"query": "钥匙", "filters": {"as_of": datetime.now(UTC).isoformat()}}) == []
|
||||
assert (
|
||||
hits(client, {"query": "钥匙", "filters": {"as_of": datetime.now(UTC).isoformat()}}) == []
|
||||
)
|
||||
|
||||
|
||||
def test_an_archived_memory_still_answers_for_the_time_before_it_was_archived(
|
||||
@@ -84,11 +90,18 @@ def test_an_archived_memory_still_answers_for_the_time_before_it_was_archived(
|
||||
)
|
||||
assert response.status_code == 200, response.text
|
||||
archived_at = datetime.now(UTC)
|
||||
moved = lifecycle_store.archive_atoms(Database(settings), [response.json()["atom_id"]], archived_at)
|
||||
moved = lifecycle_store.archive_atoms(
|
||||
Database(settings), [response.json()["atom_id"]], archived_at
|
||||
)
|
||||
assert moved == 1, "the archive moved nothing, so the test proves nothing"
|
||||
assert hits(client, {"query": "钥匙"}) == [], "an archived memory is out of the working set"
|
||||
asked_then = hits(client, {"query": "钥匙", "filters": {"as_of": (archived_at - timedelta(hours=1)).isoformat()}})
|
||||
assert asked_then == [DRAWER], "archival moves a memory between tiers, it does not make it untrue"
|
||||
asked_then = hits(
|
||||
client,
|
||||
{"query": "钥匙", "filters": {"as_of": (archived_at - timedelta(hours=1)).isoformat()}},
|
||||
)
|
||||
assert asked_then == [DRAWER], (
|
||||
"archival moves a memory between tiers, it does not make it untrue"
|
||||
)
|
||||
|
||||
|
||||
def test_asking_as_of_another_moment_does_not_borrow_another_holders_memory(
|
||||
@@ -96,26 +109,111 @@ def test_asking_as_of_another_moment_does_not_borrow_another_holders_memory(
|
||||
) -> None:
|
||||
atom_id, week_ago = store_the_pair(client)
|
||||
client.post("/api/memory/supersede", json={"atom_id": atom_id, "text": CAR})
|
||||
stranger = hits(client, {"query": "钥匙", "filters": {"as_holder": "stranger", "as_of": week_ago.isoformat()}})
|
||||
stranger = hits(
|
||||
client,
|
||||
{"query": "钥匙", "filters": {"as_holder": "stranger", "as_of": week_ago.isoformat()}},
|
||||
)
|
||||
assert stranger == [], "a moment in time does not grant access to somebody else's memory"
|
||||
as_wife = hits(client, {"query": "钥匙", "filters": {"as_holder": "wife", "as_of": week_ago.isoformat()}})
|
||||
as_wife = hits(
|
||||
client, {"query": "钥匙", "filters": {"as_holder": "wife", "as_of": week_ago.isoformat()}}
|
||||
)
|
||||
assert as_wife == [DRAWER]
|
||||
|
||||
|
||||
def test_the_dense_leg_reads_the_same_window_as_the_words_leg(settings: Settings) -> None:
|
||||
"""Both legs answer an as-of question from one rule, or the fusion would mix two moments."""
|
||||
service = MemoryService(settings, Database(settings))
|
||||
def test_the_dense_leg_reads_the_same_window_as_the_words_leg(
|
||||
tmp_path: Path, fake_embeddings: Any
|
||||
) -> None:
|
||||
"""Both legs answer an as-of question from one rule, or the fusion would mix two moments.
|
||||
|
||||
The dense leg is enabled here with a stand-in model: with the default off, this test would
|
||||
pass while the vector side of `as_of` had no coverage at all.
|
||||
"""
|
||||
from ely_eye.retrieval import RetrievalService
|
||||
from ely_eye.vector_index import VectorIndex, expected_vectors
|
||||
|
||||
settings = Settings(home=tmp_path, default_holder="wife", dense_embeddings_enabled=True)
|
||||
database = Database(settings)
|
||||
index = VectorIndex(settings, database, fake_embeddings())
|
||||
service = MemoryService(settings, database, vectors=index)
|
||||
week_ago = datetime.now(UTC) - WEEK
|
||||
drawer = service.remember(
|
||||
DRAWER, holder_id="wife", origin=MemoryOrigin.self_, event_time=week_ago, event_time_precision=TimePrecision.day
|
||||
DRAWER,
|
||||
holder_id="wife",
|
||||
origin=MemoryOrigin.self_,
|
||||
event_time=week_ago,
|
||||
event_time_precision=TimePrecision.day,
|
||||
)
|
||||
service.supersede(drawer.atom_id, text=CAR, holder_id="wife")
|
||||
from ely_eye.retrieval import RetrievalService
|
||||
assert expected_vectors(database) == 2, (
|
||||
"a retired memory keeps its vector, or no question can reach it"
|
||||
)
|
||||
|
||||
retrieval = RetrievalService(settings=settings, database=Database(settings))
|
||||
query = index.embeddings.embed_query("钥匙")
|
||||
now = index.search(query, 10, as_holder="wife")
|
||||
assert list(now) == [f"mem-{_sha(CAR, 'wife')}:text"], (
|
||||
"asked now, the vector side offers the current memory"
|
||||
)
|
||||
then = index.search(
|
||||
query, 10, as_holder="wife", as_of=(week_ago + timedelta(days=1)).isoformat()
|
||||
)
|
||||
assert f"mem-{_sha(DRAWER, 'wife')}:text" in then, (
|
||||
"the retired memory is still reachable through the vectors"
|
||||
)
|
||||
|
||||
# Asked through the retrieval service, so the plumbing that hands `as_of` to the engine is pinned too.
|
||||
criteria = SearchFilters(as_holder="wife", as_of=week_ago + timedelta(days=1)).model_dump(
|
||||
exclude_none=True
|
||||
)
|
||||
criteria["cartridge_ids"] = None
|
||||
open_now = SearchFilters(as_holder="wife").model_dump(exclude_none=True)
|
||||
open_now["cartridge_ids"] = None
|
||||
|
||||
retrieval = RetrievalService(settings, database)
|
||||
retrieval.embeddings = fake_embeddings() # type: ignore[assignment]
|
||||
retrieval.vectors = index # type: ignore[assignment]
|
||||
assert f"mem-{_sha(DRAWER, 'wife')}:text" in retrieval._dense_search("钥匙", 10, criteria)
|
||||
assert f"mem-{_sha(DRAWER, 'wife')}:text" not in retrieval._dense_search("钥匙", 10, open_now)
|
||||
now_page = retrieval.search("钥匙", filters=SearchFilters(as_holder="wife"))
|
||||
assert [hit.atom.text for hit in now_page.hits] == [CAR]
|
||||
then_page = retrieval.search(
|
||||
"钥匙", filters=SearchFilters(as_holder="wife", as_of=week_ago + timedelta(days=1))
|
||||
)
|
||||
assert [hit.atom.text for hit in then_page.hits] == [DRAWER]
|
||||
|
||||
|
||||
def test_a_memory_retired_at_a_future_date_still_answers_now(
|
||||
client: TestClient, settings: Settings
|
||||
) -> None:
|
||||
"""A change booked for next year has not happened, so the old fact is still the current one."""
|
||||
recorded = datetime.now(UTC) - timedelta(days=3)
|
||||
stored = client.post(
|
||||
"/api/memory",
|
||||
json={"text": DRAWER, "event_time": recorded.isoformat(), "event_time_precision": "day"},
|
||||
)
|
||||
assert stored.status_code == 200, stored.text
|
||||
atom_id = stored.json()["atom_id"]
|
||||
future = datetime.now(UTC) + timedelta(days=365)
|
||||
booked = client.post(
|
||||
"/api/memory/supersede",
|
||||
json={
|
||||
"atom_id": atom_id,
|
||||
"text": CAR,
|
||||
"event_time": future.isoformat(),
|
||||
"event_time_precision": "day",
|
||||
},
|
||||
)
|
||||
assert booked.status_code == 200, booked.text
|
||||
assert hits(client, {"query": "钥匙"}) == [DRAWER], (
|
||||
"a retirement dated next year has not taken effect"
|
||||
)
|
||||
listed = client.get("/api/atoms", params={"source_id": f"mem-{_sha(DRAWER, 'wife')}"})
|
||||
assert listed.status_code == 200, listed.text
|
||||
assert listed.json()["total"] == 1, "the listing and the search read the same moment"
|
||||
later = hits(
|
||||
client, {"query": "钥匙", "filters": {"as_of": (future + timedelta(days=1)).isoformat()}}
|
||||
)
|
||||
assert later == [CAR], "asked about the future, the booked change has taken effect"
|
||||
|
||||
|
||||
def _sha(statement: str, holder: str) -> str:
|
||||
return hashlib.sha256(f"{holder}\n{statement}".encode()).hexdigest()[:16]
|
||||
@@ -225,17 +225,23 @@ def test_the_index_survives_a_reopen(tmp_path: Path, fake_embeddings: Callable[[
|
||||
assert reopened.search(reopened.embeddings.embed_query(KEYS), 10)
|
||||
|
||||
|
||||
def test_superseding_moves_the_vector_rather_than_keeping_two(tmp_path: Path, fake_embeddings: Callable[[], Any]) -> None:
|
||||
def test_a_superseded_memory_keeps_its_vector_but_leaves_the_current_answer_alone(
|
||||
tmp_path: Path, fake_embeddings: Callable[[], Any]
|
||||
) -> None:
|
||||
settings, database = _settings(tmp_path), Database(_settings(tmp_path))
|
||||
index = _index(settings, database, fake_embeddings)
|
||||
memory = _memory(settings, database, index)
|
||||
old = memory.remember(KEYS, holder_id="wife", origin=MemoryOrigin.self_)
|
||||
|
||||
memory.supersede(old.atom_id, text=PASSPORT, holder_id="wife")
|
||||
replacement = memory.supersede(old.atom_id, text=PASSPORT, holder_id="wife")
|
||||
|
||||
facts = index.stats()
|
||||
assert facts["vectors"] == 1, "the closed memory lost its vector and the replacement took its place"
|
||||
assert facts["vectors"] == 2, "a closed memory keeps its vector, or a question about the past cannot reach it"
|
||||
assert facts["pending"] == 0
|
||||
current = index.search(index.embeddings.embed_query(KEYS), 10, as_holder="wife")
|
||||
assert list(current) == [replacement.atom_id], "asked now, only the replacement answers"
|
||||
then = index.search(index.embeddings.embed_query(KEYS), 10, as_holder="wife", as_of=old.ingested_at.isoformat())
|
||||
assert list(then) == [old.atom_id], "asked before the retirement, the old memory is still findable"
|
||||
|
||||
|
||||
def test_a_store_that_never_built_its_index_says_so_instead_of_stalling_a_search(
|
||||
|
||||
Reference in new issue
Block a user