fix(layers): fetch the most recent drawers for L1 wake-up - #2168
Conversation
|
With 800 rows and L1 does not hit this, since See also #660, which also replaces L1's candidate fetch. |
2611d4e to
c0cc7f7
Compare
|
You're right, and thanks for the precise repro, that made it a five-minute confirmation instead of an argument.
Your second observation is also correct and I want to be explicit about it rather than let it look like I'm minimising the first: L1 never reaches this path. The fix is in a separate commit so you can read just the delta. The predicate still can't ride along, but the ordering can, so instead of one unbounded scan the local-filter branch now walks the table newest-first one SQL page at a time and stops at the first page that completes the answer. On your case that's a single Three things I'd rather you hear from me than find:
One more caveat I'd rather write down than leave implicit: Rebased on current develop. Full suite 3875 passed / 31 skipped against 3850 on develop in the same environment; ruff 0.16.1 clean. The paged branch is also checked against a brute-force reference over 1,900 randomised cases (row counts straddling every page boundary, six filter shapes, duplicate and missing |
c0cc7f7 to
429679e
Compare
|
The fix described above is pushed as the second commit. The non-pushdown-safe path now walks LIMIT'd pages newest-first (500–5000 per page), dedupes by id, and stops at Rebased onto |
PR MemPalace#1630 fixed the L1 wake-up ordering by adding filed_at as a secondary sort key, and documented what it could not fix: "The fetch still scans up to MAX_SCAN=2000 drawers in collection (insertion) order, so on a very large unscoped wing the most-recent drawers may fall outside the cap before sorting... A SQL-side most-recent-N by filed_at fetch would fix the unscoped case but is a larger change left for a follow-up." This is that follow-up. On a 149k-drawer palace the sort was correct and the input was not: the 2000 drawers Layer1 scored were the oldest backfill slice, so wake-up permanently opened on the first files ever mined and never on this week's sessions. Backend capability rather than a pgvector special case: - BaseCollection.get_recent(limit, where, order_field, include) returns up to limit records newest-first on an ISO-8601 metadata field. The ABC default pages through get() and sorts the window locally, which is exactly what Layer1 did inline, so every backend that does not override it behaves as before. - PgVectorCollection overrides it with ORDER BY metadata->>%s DESC NULLS LAST, id pushed into the scan, and PgVectorBackend advertises the supports_recency_order capability token. That is exact at any table size. Filters that pgvector cannot push down exactly keep the existing local post-filter path. - EmbeddingCollection forwards get_recent explicitly. Without the forwarder, MRO would resolve the concrete ABC default on the wrapper and shadow the inner backend's pushdown (the invariant test_wrapper_forwards_all_concrete_basecollection_methods guards). - Layer1._fetch_candidates uses the capability and falls back to the previous paged scan when a collection predates get_recent or the backend errors, so wake-up degrades instead of failing. recency_sort_key is shared so every local sort orders identically: records missing the field, holding an empty string, or holding a non-string sort last instead of raising on a str/None comparison. No ordering semantics change for Layer1 itself. importance stays the primary key and filed_at the tiebreak; only the candidate window changes, from "the first 2000 rows the backend hands back" to "the 2000 most recently filed". Measured on the 149k-drawer palace this was written for: wake-up now leads with the newest sessions and renders in 0.72s. Tests: base default (ordering, missing/odd timestamps, window cap, where passthrough, dict-shaped get), pgvector pushdown (SQL text and bind order, filter pushdown, local-filter fallback, include, zero limit, custom order field), and Layer1 (capability used, wing filter forwarded, fallback when the capability is missing or raises).
…own-safe
PgVectorCollection.get_recent pushed the ORDER BY into SQL but passed
limit=None to _scroll whenever _requires_local_filter(where) was true, so
a filter like {"$or": [{"wing": "w1"}, {"wing": "w2"}]} dragged the whole
table across the wire to keep `limit` rows. Layer 1 never hits this (it
passes {"wing": ...} or None, both pushdown-safe), but get_recent is
public API and this is the shape a caller reaches for first.
The predicate still cannot ride along, but the ordering can, so instead
of one LIMIT-less scan the local-filter branch now walks the table
newest-first a SQL page at a time and stops at the first page that
completes the answer. On the common shape (a filter most rows match)
that is a single page. Details:
- Page stability. ORDER BY metadata->>field DESC NULLS LAST, id is a
total order because id is the primary key, so OFFSET paging is well
defined, matching the guarantee the existing ORDER BY id paging in
scroll_rows relies on. Rows are deduped by id so a concurrent insert
that shifts a row across a page boundary cannot return it twice. A
concurrent delete can still skip one row, which is inherent to OFFSET
paging and unchanged from the pre-existing paged get.
- Cap. The walk stops after 50,000 rows, so a filter matching almost
nothing in a huge table returns fewer than `limit` rows rather than
reading the table. Because the walk is newest-first, what it returns
is still the newest matching rows within the newest 50,000 records.
The pushdown branch keeps its unchanged SQL and stays exact.
- Projection. The post-filter reads only metadata, and this branch can
scan far more rows than it returns, so the document column is
projected out unless the caller asked for it.
Because the cap makes the non-pushdown path approximate where it used to
be exact, supports_recency_order is now spelled out rather than left to
read as a blanket promise: it covers the filters the backend can
evaluate in storage, which is every filter Layer 1 uses, and a backend
that bounds its non-pushdown walk must document the bound.
Also fixes two things the same diff introduced:
- BaseCollection.get_recent returned padding values for projections the
caller excluded, so include=["metadatas"] answered with a list of
empty strings for documents where pgvector answers with []. Worse,
include=["documents"] meant metadatas never came back, every sort key
collapsed to the same value and the sort silently did nothing.
metadatas are now always fetched because the sort reads order_field
out of them, and only returned when requested.
- The docstrings claimed the inexact branch re-sorted locally after
filtering, which it never did, and asserted filed_at is always UTC. It
is not: diary_ingest writes datetime.now(timezone.utc).isoformat() and
every other writer uses datetime.now().isoformat(), so on a host off
UTC the two sort against each other skewed by the local offset. That
predates this change (Layer 1 has compared filed_at as text since
MemPalace#1630) and standardising the writers needs its own migration, so the
docs now name the limitation instead of denying it. The list of places
where the SQL order and recency_sort_key disagree also now includes
database collation, which the Python test double cannot emulate.
Tests: the reviewer's exact scenario (800 rows, an $or filter, limit=5)
asserting every scroll carries a SQL LIMIT and the rows requested stay
well under the table; a selective filter that has to page three times,
checking OFFSET advances and the order stays newest-first; the cap
stopping a filter that matches nothing; a row shifted across a page
boundary by a concurrent insert, which returns a duplicate without the
id dedupe; the document projection; and the base-class include
projection.
429679e to
1acdf7b
Compare
What does this PR do?
#1630 fixed the L1 wake-up ordering by adding
filed_atas a secondary sort key, and documented the part it could not fix:This is that follow-up. Above the 2,000-drawer cap the sort was correct and the input was not:
Layer1.generatescored whatever the backend handed back first, which in practice is the oldest slice of the palace. On a 149k-drawer palace, wake-up permanently opened on the first files ever mined and never on this week's sessions, no matter how the sort was written.A backend capability rather than a pgvector special case, so every backend keeps working and capable backends get correctness at scale:
BaseCollection.get_recent(limit, where, order_field, include)returns up tolimitrecords newest-first on an ISO-8601 metadata field. The ABC default pages throughget()and sorts the window locally, which is exactly whatLayer1did inline, so any backend that does not override it behaves as it did before this PR. The docstring states that the default is exact when no more thanlimitrecords matchwhere, and approximate above that.PgVectorCollection.get_recentoverrides it withORDER BY metadata->>%s DESC NULLS LAST, idpushed into the scan, andPgVectorBackendadvertisessupports_recency_order. That token means the backend really does return the toplimitunder the stored text ordering, at any table size, for every filter it can evaluate in storage, which is every filter L1 uses ($eq,$ne,$in,$nin,$and). It says nothing about whether that text ordering matches wall-clock order, which is a property of what the writers store, and filters the backend cannot push down are bounded by a documented scan cap. A backend that bounds its non-pushdown walk has to document the bound.filed_atbelow.limitrows match, capped at 50,000 rows scanned.EmbeddingCollection.get_recentforwards explicitly. Without the forwarder, MRO resolves the concrete ABC default on the wrapper and shadows the inner backend's pushdown. The existing invariant testtest_wrapper_forwards_all_concrete_basecollection_methodscatches exactly this and did.Layer1._fetch_candidatesuses the capability and falls back to the previous paged scan when a collection predatesget_recentor the backend errors, so a plugin backend or a transient failure degrades wake-up instead of emptying it.recency_sort_keyis shared so every local sort orders identically: records with a missing, empty, or non-string timestamp sort last instead of raising on astr/Nonecomparison.L1's own ranking semantics do not change.
importancestays the primary key andfiled_atthe tiebreak. Only the candidate window changes, from "the first 2,000 rows the backend hands back" to "the 2,000 most recently filed".Measured on the 149k-drawer palace this was written for: wake-up leads with the newest sessions and renders in 0.72s.
The bounded scan, and what it costs (second commit)
@mvalentsev is right, and the second commit is the answer to his review.
get_recentpushed the ORDER BY into SQL but passedlimit=Noneto_scrollwhenever_requires_local_filter(where)was true, so$or,$containsand every comparison operator dragged the whole table across the wire to keeplimitrows. His 800-row case issued exactly one LIMIT-less scroll. That is now a regression test asserting onscroll_calls.His second observation is also correct: L1 never reaches that path.
Layer1._fetch_candidatespasses{"wing": ...}orNone, both pushdown-safe, so wake-up always took the exactLIMIT nbranch. It was a latent bug in the public method, not in the feature this PR is about, and my own testtest_pgvector_get_recent_local_filter_still_orderswas asserting"limit": Noneas expected, which is how it got through.Three things about the fix that are better heard from me than found:
limitrows rather than reading the table. The unbounded version was genuinely exact there. The approximation is much tighter than the base class default's, because the walk is newest-first, so what comes back is the newest matching rows within the newest 50,000 records rather than the newest within an arbitrary storage-order window. The pushdown branch keeps no cap and stays exact.supports_recency_orderwas rewritten to stop reading as a blanket promise.ORDER BY metadata->>field DESC NULLS LAST, idis a total order becauseidis the primary key, so OFFSET paging is well defined, the same guarantee the existingORDER BY idpaging relies on. Rows are deduped byid, so a concurrent insert that shifts a row across a page boundary cannot return it twice. A concurrent delete can still skip one row; that is inherent to OFFSET paging and unchanged from the pre-existing pagedget. A keyset cursor would close it, needs plumbing throughscroll_rows, and is deliberately not smuggled in here.BaseCollection.get_recentreturned padding for projections the caller excluded, soinclude=["metadatas"]answereddocuments=["", "", ...]where pgvector answers[]; worse,include=["documents"]meant metadatas never came back, every sort key collapsed to(0, ""), and the sort silently did nothing while the method claimed newest-first. Fixed, with a test.includeprojections the caller excluded now come back empty from the base default too, matchinggetand pgvector.Honest caveat:
filed_atis not written in one offset form todayThe pgvector docstring used to claim
filed_atis always UTC. It is not. Ten of eleven production writers usedatetime.now().isoformat()(naive local):miner.py:1393,1607,convo_miner.py:157,576,format_miner.py:573,639,closet_llm.py:342,sweeper.py:280,mcp_server.py:2749(add_drawer),mcp_server.py:3712(diary_write). One,diary_ingest.py:174(+191, 302), usesdatetime.now(timezone.utc).isoformat()and writes...+00:00. Both land in the same collection through the same accessor with no wing separation, and the split runs inside one feature:diary_writeis naive,diary_ingestis UTC.So text order between those two groups is skewed by the host's UTC offset, and the sign flips with the hemisphere:
This predates the PR.
layers.pyhas comparedfiled_atas raw text since #1630, andmcp_server.py::_filed_at_in_windowparses withfromisoformatthen strips tzinfo, absorbing the same skew. NoORDER BY filed_atexisted in SQL before this PR.But this PR changes the failure mode, which is why it is in the body rather than buried. Before, the mis-ordering only affected ranking within a storage-order window. With the pushdown,
ORDER BY ... LIMIT ndecides membership of the candidate window, so on a palace aboveMAX_SCAN=2000the skew can push drawers out of the L1 candidate set entirely rather than merely mis-rank them inside it. Bounded by the host's offset, but a new consequence of an old bug. Standardising the writers on UTC needs a migration for palaces already holding both forms; happy to open that separately if you agree that is the right split.One further caveat that no test in this repo can catch:
metadata->>%sand theidtiebreak both sort under the database collation, whilerecency_sort_keysorts by Python codepoint. Underen_US.UTF-8/ICU those disagree on strings differing only in punctuation, which is exactly the axis the twofiled_atforms differ on, and the in-repo test double emulates ordering in Python. Named in the docstring.How to test
Baseline
origin/develop@906b918(develop after the 3.7.1 sync) in the same environment: 4295 passed, 31 skipped, so +25 tests and no existing test deleted. ruff 0.16.1 (the repo pin), Python 3.12.13. One existing test,test_pgvector_get_recent_local_filter_still_orders, had its assertion corrected from"limit": Noneto"limit": 500, which is the whole point of the second commit.Reviewer's scenario, run against the unfixed source:
One scroll,
limit=None, 800 rows fetched to return 5. After: a singleLIMIT 500scroll.The base-class projection defect, against the unfixed base:
New tests, all six of which fail on the parent commit:
test_pgvector_get_recent_local_filter_does_not_fetch_whole_table— his 800-row$orcase; asserts every scroll carries a SQL LIMIT and total rows requested stays under the table sizetest_pgvector_get_recent_local_filter_pages_until_enough_match— selective filter, 1,200 rows, 3 matches at the far end; asserts three pages, each bounded, OFFSET advancingNone → 500 → 1000, order still newest-firsttest_pgvector_get_recent_local_filter_caps_pathological_scan— filter matching nothing, cap monkeypatched to 20; stops at 20 of 100 rowstest_pgvector_get_recent_local_filter_dedupes_rows_shifted_by_a_write— a newer row inserted mid-scan shifts the page boundary; asserts no duplicate. Proven to bite: with theseencheck stubbed out the result is['d5','d4','d4','d3','d2','d1']test_pgvector_get_recent_local_filter_projects_out_document—with_document=Falseon the local branchtest_base_get_recent_default_honours_include_projection— unrequested projections empty, metadatas fetched anyway so the sort worksPlus the original coverage: base default (ordering, missing/empty/non-string timestamps sorting last, the window cap with 500-record paging asserted,
wherepassthrough, zero limit, Chroma-shaped dictget()); pgvector (capability token, SQL text and bound-parameter order, LIMIT pushdown, exact-filter pushdown,includeprojection, zero limit, customorder_field); Layer1 (capability used withlimit=MAX_SCANand no scan issued, wing filter forwarded aswhere, fallback whenget_recentis absent, fallback when it raises, and a capable backend surfacing a drawer filed beyond the scan window).Differential harness for the paged path. The paged branch was checked against a brute-force reference (sort all rows by
(present, filed_at)desc thenidasc, filter, takelimit) over 1,900 randomised cases: row counts 0 to 1,001 straddling every page boundary, six filter shapes,limit1 to 1,000, page sizes 1 to 500, caps 1 to 50,000, duplicatefiled_atforcing theidtiebreak, missing and empty-stringfiled_at, and a mode where matches exist only at the far end. Zero divergences. The cap-boundary arithmetic (want = min(page_size, cap - scanned)) was separately confirmed not to fire the short-page break spuriously atcap=97 page=10,cap=13 page=5,cap=7 page=3.Landing order with #2169. The two branches were merged in a scratch worktree.
mempalace/layers.pyauto-merges with no conflict. The only textual conflicts are additive and both resolve by union: the shared import block at the top oftests/test_layers.py, and the## [Unreleased]anchor inCHANGELOG.md. The composed tree was verified green at the base where the compose was run (3903 passed, 31 skipped, exactly3850 + 25 + 28, ruff clean), so nothing is lost or duplicated by combining them; both branches now sit rebased on the same post-3.7.1 develop (906b918) and the conflict shape is unchanged. Whichever lands second needs that two-hunk rebase and nothing more.Checklist
python -m pytest tests/ -v) — 4320 passed, 31 skipped, against 4295 on develop in the same envruff check .) — clean on ruff 0.16.1, along withruff format --check .Docs:
website/concepts/memory-stack.mdL1 generation steps, and a CHANGELOG entry under Unreleased. No new dependencies.