Re-embed the stored notes and facts (#378, part 2) #29

Closed
claude wants to merge 3 commits from overnight/embedder-backfill-pr into overnight/embedder-marker-pr
Contributor

Two commits. Second half of Vikunja #378 — the marker (PR 28) says the vectors are stale, this rewrites them.

Run it once, by hand: mavend -reembed.

That's the part worth arguing with, so here's the reasoning. Re-embedding is ONNX on the laptop CPU — a few thousand rows is minutes. Doing that silently inside a normal start would be indistinguishable from a boot hang, and you'd kill it. So a normal start still just warns, and the warning now names the fix. -reembed does the work during startup and then the daemon carries on serving — no separate binary, no second restart.

It rewrites both places a vector lives — the notes table column and the memory_vectors rows. A partial rewrite would be worse than none: it leaves the two indexes disagreeing, and note recall (PR 27) now searches the unified index first.

Passage side, not query. e5 is asymmetric — stored text is a passage:, a question is a query:. Stored notes are the thing being searched for, so the rewrite uses EmbedPassage, matching what both write paths already do. Getting this backwards would quietly halve recall while looking like it worked.

One transaction, and the marker is inside it. Every vector update and the marker write commit together or not at all. If it dies halfway: nothing changed, no marker written, the warning still fires on the next start, and re-running does the whole job. The marker can never exist without a completed rewrite — recording a lie is the one outcome worth engineering against here.

Re-running is free. It reads the marker first; if it already matches, it returns immediately — zero embed calls, zero writes.

The second commit is a separate warning. A row with no stored text can't be re-embedded, so its vector stays as the old model's noise while the marker now claims everything is current. Both write paths always store the text, so this should be zero — but it was reported as one number inside the success line, where you'd never see it. It gets its own WARNING now.

Tests use a fake embedder returning a recognisable vector, so the assertion is that the rewrite actually happened, not that the code ran: every vector rewritten and the marker set, a second run skipped with no embed calls, and a failure on the second row leaving the marker unset with the first vector unchanged.

Recall behaviour is untouched. Whether recall should refuse to answer while the marker mismatches is still your open decision from PR 28.

Two commits. Second half of Vikunja #378 — the marker (PR 28) says the vectors are stale, this rewrites them. **Run it once, by hand: `mavend -reembed`.** That's the part worth arguing with, so here's the reasoning. Re-embedding is ONNX on the laptop CPU — a few thousand rows is minutes. Doing that silently inside a normal start would be indistinguishable from a boot hang, and you'd kill it. So a normal start still just warns, and the warning now names the fix. `-reembed` does the work during startup and then the daemon carries on serving — no separate binary, no second restart. **It rewrites both places a vector lives** — the `notes` table column *and* the `memory_vectors` rows. A partial rewrite would be worse than none: it leaves the two indexes disagreeing, and note recall (PR 27) now searches the unified index first. **Passage side, not query.** e5 is asymmetric — stored text is a `passage:`, a question is a `query:`. Stored notes are the thing being searched *for*, so the rewrite uses `EmbedPassage`, matching what both write paths already do. Getting this backwards would quietly halve recall while looking like it worked. **One transaction, and the marker is inside it.** Every vector update and the marker write commit together or not at all. If it dies halfway: nothing changed, no marker written, the warning still fires on the next start, and re-running does the whole job. **The marker can never exist without a completed rewrite** — recording a lie is the one outcome worth engineering against here. **Re-running is free.** It reads the marker first; if it already matches, it returns immediately — zero embed calls, zero writes. **The second commit is a separate warning.** A row with no stored text can't be re-embedded, so its vector stays as the old model's noise while the marker now claims everything is current. Both write paths always store the text, so this should be zero — but it was reported as one number inside the success line, where you'd never see it. It gets its own WARNING now. **Tests** use a fake embedder returning a recognisable vector, so the assertion is that the rewrite *actually happened*, not that the code ran: every vector rewritten and the marker set, a second run skipped with no embed calls, and a failure on the second row leaving the marker unset with the first vector unchanged. **Recall behaviour is untouched.** Whether recall should refuse to answer while the marker mismatches is still your open decision from PR 28.
claude added 3 commits 2026-07-31 11:53:27 +02:00
The embedder swap left every stored vector in the old model's space, so cosine against a new query vector is noise. Add the one-shot backfill: store.ReembedAll re-embeds every note and fact text with the currently configured embedder (the passage side, which is the side stored text was written with) and rewrites both places a vector lives — the notes table embedding column and the memory_vectors rows.

All of it plus the embedder marker happens in one transaction, so a failure partway changes nothing and writes no marker: re-run it. A run against a DB whose marker already names the current embedder does nothing.

Triggered explicitly with `mavend -reembed`, not automatically on mismatch: ONNX on the laptop CPU makes this minutes of work, and a silent multi-minute stall on boot would look like a hang. The mismatch warning now tells the user to run it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Owner

Superseded by #47, which landed this whole stack on master as one reviewed integration merge. This PR head is an ancestor of master — its commits are in, nothing here is lost. Closing as merged-by-proxy rather than merged, since the merge came in through #47.

Review threads on this PR were answered or acted on before the merge; the Russian wording fixes went in as #48.

Superseded by #47, which landed this whole stack on master as one reviewed integration merge. This PR head is an ancestor of master — its commits are in, nothing here is lost. Closing as merged-by-proxy rather than merged, since the merge came in through #47. Review threads on this PR were answered or acted on before the merge; the Russian wording fixes went in as #48.
kami closed this pull request 2026-07-31 20:22:20 +02:00

Pull request closed

Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kami/Maven#29