Merge #38: configurable embedding probe timeout - #41
Merged
Merged
Conversation
`memory_status` reported `Embeddings: ? unknown` on healthy setups where
qmd, the models, and semantic search all worked correctly.
`probeEmbeddings()` raced `qmd vsearch` against a hardcoded 4000ms timer.
That probe is not cheap: it runs LLM query expansion plus an embed and
rerank pass. Measured latency on a working install (n=12, warm, idle):
min 2.35s median 2.91s max 3.55s
That leaves only ~0.45s of headroom. Writes schedule a background
`qmd update`/`qmd embed`, which competes for CPU and the embedding model,
so the probe crosses 4s exactly when a write just happened - which is
when users are most likely to run `memory_status`. Reproduced against
real qmd with a concurrent re-index:
old hardcoded 4s -> unknown 4005ms
new default 15s -> ready 6048ms
The `catch` maps any failure to "unknown", so a slow probe was
indistinguishable from a broken one and the status output implied the
embeddings were at fault when they were fine.
Also note `PI_MEMORY_QMD_SEARCH_TIMEOUT_MS` did not apply here: real
searches honor it, but the probe's literal `4_000` overrode it, so the
knob that looked like it should fix this had no effect.
Changes:
- add DEFAULT_EMBED_PROBE_TIMEOUT_MS (15s) and getEmbedProbeTimeoutMs(),
overridable via PI_MEMORY_EMBED_PROBE_TIMEOUT_MS
- pass the probe budget to runQmdSearch via an optional timeout override
so an abandoned probe cannot leave a 60s LLM query running
- surface the effective timeout in the "unknown" hint and in the
memory_status configuration block
- document the variable in README.md
Considered parsing `qmd status` instead, since it needs no LLM and is
~14x faster (0.21s vs 2.91s). Rejected: its `Vectors: N embedded` count
is index-global, not per-collection, so it would report "ready" from
another collection's embeddings while pi-memory had none. `qmd status`
also has no --json mode, making the parse fragile. Correctness over speed.
Tests: 8 new cases covering the default floor, env override, invalid
values, ready/missing detection, child-process bounding, and a genuine
timeout still yielding "unknown". Verified red/green: reverting only the
probe change fails 3 of them.
bun test test/unit.test.ts 190 pass, 0 fail (was 182 pass)
npm run build clean
npm run lint clean
No change to on-disk memory formats. qmd invocation is unchanged apart
from the child timeout value.
Signed-off-by: Jinserk Baik <jvby@novonordisk.com>
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.
Conflict-resolution PR for #38 after #35 landed. Preserves both histories and keeps the #38 behavior/tests while retaining the exit-summary reasoning-effort changes from #35.