Repository navigation
perf(atomic-memory): bound related recall candidates in SQL to reduce peak memory and recall cost - #2449
Conversation
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
The bounded recall path introduces correctness regressions described inline. The required upgrade policy check also fails because the persistence compatibility review was not updated; the dependent CI suites were skipped. Please also run the real OceanBase acceptance required by #2448.
| atomic_memory_related_mode: Literal["auto", "fts", "vector", "hybrid"] = "auto" | ||
| atomic_memory_related_max_distance: float = Field(default=1.0, ge=0) | ||
| atomic_memory_related_max_candidates: int | None = Field( | ||
| default=DEFAULT_ATOMIC_MEMORY_RELATED_MAX_CANDIDATES, ge=1, le=1000 |
There was a problem hiding this comment.
[P1] Preserve all threshold-qualified memories during reconciliation. With the new default, the 101st eligible memory is silently omitted even though the processor treats the bounded result as the complete comparison set. If that memory is the existing fact that should be revised or merged, Source processing can instead create a duplicate or retain conflicting facts. RFC 1809 explicitly requires every memory meeting the threshold to enter comparison and forbids a fixed top-N cutoff. Please bound memory through batching/streaming (or otherwise preserve the complete set) rather than silently dropping qualifying memories.
|
|
||
| if limit is None: | ||
| return None | ||
| return limit + ATOMIC_MEMORY_RRF_CONSTANT if fused else limit |
There was a problem hiding this comment.
[P2] The K + 60 window does not guarantee the promised full-RRF prefix. For K=1, put a at rank 62 in both channels, b at FTS rank 1 only, and c at vector rank 1 only, with all remaining IDs distinct. Full fusion gives a and b the same score (2/122 = 1/61), so the existing ID tie-break selects a; limiting each channel to 61 removes a and selects b. I reproduced this with combine_atomic_memory_channels. Use an exact stopping condition or explicitly change the claimed ranking contract; a fixed RRF constant is not a sufficient overfetch bound.
| query_vector=vector, | ||
| embedding_profile=profile, | ||
| max_distance=self.config.related_max_distance, | ||
| max_candidates=self.config.related_max_candidates, |
There was a problem hiding this comment.
[P2] Apply any candidate budget to authorized memories, not before per-Artifact authorization. The index trims to this limit before _load_related_items checks read and write access; its SQL filters do not encode arbitrary authorizer decisions. For example, if the first 100 ranked memories are denied write access and rank 101 is writable, the default path returns no comparison even though an eligible writable memory exists. This can cause a duplicate or conflicting memory to be published. Continue fetching within the same consistency boundary until the authorized comparison set is complete (or an explicitly supported authorized limit is met).
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
The pagination change removes the fixed cap, but I reproduced two new correctness regressions involving concurrent changes, described inline. The current CI run also has two candidate schema replay failures; the upgrade acceptance job was skipped and its gate failed.
| while True: | ||
| async with ( | ||
| deferred_decision_audit(context), | ||
| application.database.transaction(consistent_snapshot=True) as connection, |
There was a problem hiding this comment.
[P1] Keep the complete recall stable across pages, or detect changes and retry. Each iteration starts a new snapshot but the FTS cursor contains the previous snapshot's BM25 score. I reproduced this on SQLite with two matching memories and page size 1: page 1 returns memory-001 at score 0.000002; inserting 100 unrelated memories in a different Scope changes the FTS corpus score to 7.3877, and page 2 returns no rows, silently skipping unchanged memory-002. The model can then create a duplicate or miss a required revision. Please preserve one stable candidate ordering/snapshot or reliably invalidate and restart the whole comparison when it changes.
| if self.origins.get(identity) == resolved: | ||
| del self.origins[identity] | ||
| if self.reads.get(identity) == read: | ||
| del self.reads[identity] |
There was a problem hiding this comment.
[P1] Retain every compared memory's lightweight read dependency until commit. For an unchanged memory that the model compares but does not select as a target, this deletes the entry from workset.reads, which is the read set validated by commit_window. I reproduced a flush where the reconciler reads revision 1 and pauses, another writer publishes revision 2, and the reconciler returns create: the flush succeeds after only one comparison against revision 1 instead of retrying against revision 2. This can commit a decision based on stale evidence. Release the page body if needed, but retain and revalidate all compared identities and revisions at publication.
|
thanks review! Detailed changes: 1. cross-page ranking driftPaging changed from "re-rank every page plus keyset filtering on the previous snapshot" to "one complete lightweight ranking plus offset slicing":
2. lost read dependencies at commit
|
|
The CI failures are unrelated to this PR; however, I have addressed them in PR #2483 |
|
Please don't merge this yet, I'm seeing some metric regressions after re-testing with the new approach, and I'm working on a fix
|
|
I replayed the two previous concurrency counterexamples on There is also a merge conflict with current |
|
Conflict and CI, I merged master Performance: The paged implementation kept the old default page size of 100, so 50k rows meant 500 transactions and system CPU rose to 3.77s. Setting the default to 250 brings that to 200.
All three CPU numbers are now better than before the change. The one metric that got worse. Total bytes allocated went 284 MB → 486 MB. This counts every allocation, including ones freed immediately; it is not memory held at once. Held memory (peak) went 124.7 MB → 33.9 MB. The increase exists because the corpus is now read twice, once to rank and once to fetch bodies, instead of once. I investigated the breakdown of the overhead for this specific metric.: 87% is inside SQLite doing FTS5 scoring and the exact vector scan, not in Python; the remaining Python-side savings are about 1.4%. Removing the second pass would mean dropping the fixed ranking snapshot, which is the fix that was requested. I'm not sure if everyone is okay with accepting this regression in total allocated bytes in exchange for the improvements in other metrics. |
|
On The merge conflict has recurred after |
hireview001
left a comment
hireview001
left a comment
There was a problem hiding this comment.
- Resolve merge conflicts with the target branch.
- CI failed: CI result (failure).
- CI failed: Upgrade release gate (failure).
- CI did not complete: tests (3.12, e2e) (cancelled).
- CI did not complete: tests (3.11, unit) (cancelled).
- CI failed: tests (3.14, unit) (failure).
- CI failed: Main (failure). Failed steps: tests (3.14, unit) / Run unit tests; tests (3.11, unit) / Run unit tests; tests (3.12, e2e) / Run end-to-end tests.
|
The CI is still failing; it might need #2504 to be merged first. |
hireview001
left a comment
hireview001
left a comment
There was a problem hiding this comment.
- Resolve merge conflicts with the target branch.
- CI failed: CI result (failure).
- CI failed: Upgrade release gate (failure).
- CI failed: CI preflight (failure).
- CI failed: Main (failure). Failed steps: CI preflight / Check License Header; Upgrade release gate / Require the selected parallel acceptance workflow; CI result / Require successful selected checks.
knqiufan
left a comment
knqiufan
left a comment
There was a problem hiding this comment.
Reviewed the bounded recall changes and prior correctness findings. The reported completeness, ranking, authorization, read-dependency and snapshot regressions are addressed at this head; focused SQLite regressions passed. This review does not constitute new real-backend acceptance.
Bound related recall with a max_candidates limit pushed into eligibility, over-fetch one RRF window per channel, and trim fused hits and heads to the cap. SQLite and OceanBase enforce the cap in SQL; OceanBase uses one snapshot with a readiness CTE and an explicit sentinel row. Runtime configuration defaults to 100 candidates, and explicit null keeps complete enumeration. Closes: oceanbase#2448
Cover SQLite channel caps, filter-before-limit ordering, the hybrid over-fetch window, OceanBase bindings and UNION widths, sentinel handling, runtime configuration validation, and the flush-to-reconciler chain. Closes: oceanbase#2448
Document that related recall is bounded to 100 candidates by default in both locales. Closes: oceanbase#2448
Take one complete lightweight ranking per related enumeration and slice it by offset, so corpus changes in another Scope can no longer reorder or truncate later pages. A ranked memory that leaves the index before its page is read reports stale recall instead of raising a bare KeyError.
Release only page-local bodies from a workset and retain every compared read, so an artifact revision published mid-window conflicts and retries instead of committing against stale evidence.
Document the frozen lightweight ranking, what the page size bounds, and that a revision published after ranking conflicts and retries the window.
The inherited 100 came from the retired candidate cap, where it bounded the result set. Paging only batches bodies and authorization, so the same number now multiplies a fixed per-page cost across a complete enumeration. At 50k rows, 250 turns 500 page transactions into 200: Sys 3.77s -> 0.69s, User 8.63s -> 5.15s, Wall 10.48s -> 4.81s, with the peak delta unchanged at 159.7 MB. 250 is where the gains stop (250/500/1000 stay within noise) and it leaves headroom below the 1000 validation limit.
Related recall pages no longer issue a rank query when the admission floor exceeds the query's distinct terms; whole-word coverage can never reach it. Refresh the persistence review digest for the resulting tree.
9890d4f to
ce7d18e
Compare
|
Rebased onto
|
Closes: #2448
Motivation
Related recall previously enumerated everything: the SQL had no
LIMIT,tuple(mappings())materialized every row at once, each hit was authorized twice, andheadsretained the full body for every match. Peak memory grew linearly with the number of memories in the Scope—at 50,000 memories it reached 606,764 KB, while the process baseline was only about 15 MiB.This PR adds a per-channel limit in SQL while preserving the completeness contract: invalid vectors ranked outside K are still reported.
What changes are included in this PR?
src/powercontext/builtin/persistence/atomic_memory_index.py:AtomicMemoryRelatedRequest.max_candidates: int | None;atomic_memory_channel_limit(): when the two channels may be reordered by RRF, fetchK + 60per channel; single-channel recall fetches exactlyK;trim_atomic_memory_related(): after fusion, trim hits and the correspondingheadstoK;atomic_memory_heads_select()/atomic_memory_heads_from_rows()so both backends share the same head/body join.sqlite/atomic_memory_index.py:_related_limit_sql()appendsORDER BY score DESC, artifact_id LIMIT :result_limit; the vector channel switches tobounded=True; the readiness sentinel still covers all eligible rows, so invalid vectors outside K still triggerincomplete-vector.oceanbase/atomic_memory_index.py:_related_bounded()builds bounded FTS and vector channels within a single snapshot and includes a whole-Scope readiness check; every UNION branch is parenthesized, otherwise the per-armORDER BY ... LIMITis invalid.runtime/config.pyandruntime/atomic_memory_processing.py: exposeatomic_memory_related_max_candidates(default100, range1..1000, rejects booleans) and wire it from RuntimeConfig into the request.tests/builtin/persistence/test_atomic_memory_candidate_cap_sql.pyandtests/e2e/test_atomic_memory_candidate_cap.py, and extendtests/e2e/test_atomic_memory_related_embedding.pyandtests/builtin/runtime/test_artifact_processing_config.py.max_candidates=Nonethe request path is unchanged; no schema or API changes.Are there any user-visible changes?
Yes. Related recall now returns at most the 100 most relevant memories by default,
atomic_memory_related_max_candidates, default 100, range 1–1000, instead of everything in the Scope. Results are the bounded prefix of the full ordering,headsare trimmed to match, and invalid vectors outside K still raiseincomplete-vector. Set the option to empty to restore complete enumeration. No API, schema, or request-format changes.Performance comparison against baseline
Targeted tests:
85 passed, 8 skipped(SQLite, Python 3.12.16). The benchmark corpus uses identical vectors andmax_distance=1.0, meaning every recall hits the entire Scope in the worst case; bodies are 2 KB; only the SQLite backend was verified, withmax_candidates=60.1. Peak memory across Scope sizes
After the change the curve is flat: 1k → 50k adds only 28 KB (+0.5%), compared with 591,664 KB before.
2. memray allocations
aiosqlite/core.py:63still accounts for 98.5 MB of cumulative allocations (99.5%), but the peak is only 4.8 MB—rows now pass through and are released instead of residing on the heap.3. Single-recall memory curve
Before: resident memory rose by about 125 MB during recall, matching the 123,704 KB delta; after recall it stayed high because the allocator retained arenas.
After: resident memory rose by only about 5.5 MB, the heap dropped immediately after the call, and no plateau remained above the process baseline.
4. py-spy CPU diff
The header reads 27 samples / 8.54% , versus 30.95% for the same line before the change. The hotspot is in the same place, but its share has clearly shrunk.
Correctness coverage
max_candidates=Nonethe SQL has no LIMIT or readiness, preserving complete enumerationAI usage statement
AI-assisted (Trae, DeepSeek-V4.1-Flash) implementation, tests, and web preview; the user performed the tests and performance analysis.
Reference Links