feat(embeddings): send a task prefix for models that expect one - #23
Conversation
embeddinggemma is trained with instruction prefixes and scores noticeably worse without one. The prefix is part of the embedding text, so it belongs in prepareEmbeddingText rather than at each call site, and it bumps EMBEDDING_TEXT_VERSION to 3 so the config hash separates prefixed vectors from unprefixed ones and a stale store cannot be mixed with a fresh one. Models with no known prefix are unaffected: taskPrefixFor returns an empty string, so their embedding text is byte-identical to before. Wired at the three places that build embedding text: both pipeline paths and the benchmark runner.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b180623ec6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return body ? `${title}\n\n${body}` : title; | ||
| const text = body ? `${title}\n\n${body}` : title; | ||
| // Model-specific task instruction, empty for models that do not use one. | ||
| return `${taskPrefixFor(model)}${text}`; |
There was a problem hiding this comment.
Keep vision embeddings in the same task space
When EMBEDDING_MODEL=embeddinggemma, scanned PR/issue vectors now include the clustering prefix, but loadAndEmbedVisionDoc still embeds raw document chunks at src/vision.ts:46-61; scoreVisionAlignment then compares these differently prompted vectors against fixed cosine thresholds. This can change alignment scores and misclassify items whenever the vision command runs with this model, so the vision chunks need an appropriate EmbeddingGemma task prompt (or both sides must otherwise use a consistent task format).
Useful? React with 👍 / 👎.
embeddinggemmais trained with instruction prefixes and scores worse without one. This sendstask: clustering | query:for models with a known prefix.The prefix is part of the embedding text, so it lives in
prepareEmbeddingTextrather than at each call site. Wired at the three places that build embedding text: both pipeline paths and the benchmark runner.EMBEDDING_TEXT_VERSIONgoes to 3, so the config hash separates prefixed vectors from unprefixed ones and a stale store cannot be silently mixed with a fresh one.Models with no known prefix are unaffected.
taskPrefixForreturns an empty string, so their embedding text is byte-identical to before.honest result
Measured on odysseus-dev/odysseus (5285 items): the prefix made no difference to clustering at all. Identical cluster counts and identical overlap, 32/60 at 0.82 and 26/39 at 0.85, same as the unprefixed run.
i checked the prefix was actually being applied before concluding that. A constant prefix shifts every vector in the same direction, so relative cosine similarity survives. It may still matter for retrieval; it does not matter for this workload.
Keeping it because it is what the model documents, it costs nothing measurable, and the version bump means the choice is recorded.