Skip to content

feat(search): cache query embeddings per searcher - #436

Open
ASuresh0524 wants to merge 1 commit into
mainfrom
cache-pr
Open

ASuresh0524 wants to merge 1 commit into
mainfrom
cache-pr

Conversation

@ASuresh0524

Copy link
Copy Markdown
Collaborator

Split out of #226, which merged the ZMQ connection reuse on its own. This is the query-embedding cache, with the review points from that thread applied.

What it does

BaseSearcher.compute_query_embedding recomputes the query vector on every call. Within one process the result is deterministic for a given (query, template, model), so a repeated query can reuse it.

Scope — what this does not do

This does not address #177. That repro is three separate leann search invocations: the cache starts empty in each, and the three queries differ anyway. The 13–19s there is server startup, model load, and per-node recompute, none of which this touches. Going at #177 directly means per-invocation startup — keeping the embedding server warm across CLI calls, or caching the vector on disk keyed by (model, template, query) — which survives the process boundary that an in-memory cache does not.

Where this can pay off is a process that issues the same query more than once: LeannChat, the LlamaIndex retrievers, an agent loop.

Changes since #226

eviction was FIFO — get never reordered, so a hot query could be dropped while a never-reused one survived. Now OrderedDict with move_to_end on read and popitem(last=False) on evict
key was {query, template}; now includes the embedding model, so an entry can never be served to a different model
query_cache_size=0 was a crash. Eviction ran against an empty mapping and raised KeyError: 'dictionary is empty' on the first put. Now disables the cache cleanly — which also makes it usable as the control arm in a benchmark

Nine tests: LRU ordering, model scoping, the disabled case, and the existing round-trip behaviour.

What is still missing

@andylizf — you asked for a benchmark on the real search path before this earns its place, and that is not here. I did not write one rather than write another misleading one: the benchmark in #226 never imported leann (it redeclared the cache and time.sleep()ed in place of embedding, so its reported 2.0× was the sleep it skipped), and I removed it rather than replace it.

So this PR is the corrected code and its tests, with the measurement still owed. If you would rather it wait for that, leave it open — the code is in a reviewable state and the benchmark can land on top. query_cache_size=0 gives the control arm.

🤖 Generated with Claude Code

Split out of #226, which merged the ZMQ connection reuse on its own.

`BaseSearcher.compute_query_embedding` recomputes the query vector on every
call. Within one process the result is deterministic for a given (query,
template, model), so a repeated query can reuse it. This caches it.

Scope, stated plainly: this helps a process that issues the same query more
than once -- `LeannChat`, the LlamaIndex retrievers, an agent loop. It does
**not** address #177, whose repro is three separate `leann search`
invocations: the cache starts empty in each, and the three queries differ
anyway. The 13-19s there is server startup, model load, and per-node
recompute, none of which this touches.

- `OrderedDict` with `move_to_end` on read and `popitem(last=False)` on
  evict, so eviction is LRU rather than insertion order
- the key is (query, template, embedding model), so an entry can never be
  served to a different model
- `query_cache_size=0` disables it; eviction previously ran against an empty
  mapping and raised `KeyError: 'dictionary is empty'` on the first put

9 tests, covering LRU ordering, model scoping, the disabled case, and the
existing round-trip behaviour.

Still open, and the reason this is its own PR: there is no benchmark on the
real search path. The one in #226 never imported leann -- it redeclared the
cache and slept in place of embedding -- so it measured the sleep it skipped,
and it was removed rather than replaced.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@andylizf

Copy link
Copy Markdown
Collaborator

I ran the measurement you said was still owed. LeannSearcher.search on a 2,000-passage IVF index, all-MiniLM-L6-v2, top-5, 40 calls to the same query and 30 unseen queries, cache on vs query_cache_size=0, on an Apple-silicon CPU:

path same query, cache on cache off
ZMQ embedding server 0.45 ms 6.37 ms
direct model (recompute_embeddings=False) 0.30 ms 4.21 ms

That is 14.2x on the repeated query in both paths. Unseen queries were 5.23 vs 5.94 ms on the server path and 4.69 vs 4.15 ms on the direct path, so the miss path costs under a millisecond either way. Top-5 IDs were identical with and without the cache. This matches the scope you stated: a process that repeats a query. As you say, it does not address #177.

One thing to restore before this lands: the diff replaces the ReusableZMQConnection docstring on main with a one-line summary. That drops the warning that a zmq.Socket is not thread-safe and that a searcher must not have search called concurrently from multiple threads. The warning came in with the ZMQ change and still applies. The cache adds another per-searcher structure with the same constraint. With that restored, I have no other code comments.

Please also document query_cache_size; right now the control arm is only reachable as a constructor kwarg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants