Route search queries through the embedding provider's query side - #760
Route search queries through the embedding provider's query side#760ohdearquant wants to merge 3 commits into
Conversation
Asymmetric embedding models encode queries and passages differently: the query side carries an instruction prefix that the passage side must not have. bge-small-en-v1.5 prefixes queries with "Represent this sentence for searching relevant passages: ", the E5 family uses "query: " against "passage: ". Embedding a query as a passage does not error, it just lowers query-to-passage similarity and quietly costs retrieval quality. EmbeddingProvider had no query method, so AgenticDB embedded every search query through embed(), the passage path. LatticeEmbedding already implements the split, but only as an inherent method, which is unreachable through the Arc<dyn EmbeddingProvider> that AgenticDB holds. The correct behaviour existed and could not be reached from the one place that needed it. - add EmbeddingProvider::embed_query, defaulting to embed so every existing implementor keeps its current behaviour unchanged - override it for LatticeEmbedding so the asymmetry crosses the boundary - route the five search paths through it: retrieve_similar_episodes, search_skills, query_with_utility, find_relevant_turns and the witness log search. The five storing paths keep using embed. Tests: a provider that records which side each call landed on pins all ten paths and fails if any search is routed back through the passage method. A second test pins the default, so the added method stays non-breaking for providers that do not override it.
|
The red The failure. This diff cannot reach that test. It touches exactly two files, both in and
The test also builds its vectors explicitly ( It looks like ANN recall variance. Same job on sibling PRs (#757, #758, #759) is green, so the shard is not broadly broken; this Happy to re-run the job if you would rather see it green before merging. |
|
Triage of the The failing test is Evidence it is not introduced here:
What the numbers say about the failure mode: the test places Filed separately as an issue so it is not lost inside this PR. |
…ide dispatch - Extend the embedding-side routing test to also assert create_skill and add_causal_edge embed on the passage side. - Add a lattice-embeddings-gated regression test proving Arc<dyn EmbeddingProvider>::embed_query dispatches through LatticeEmbedding's query-side override rather than falling back to the passage side, using a small test-only call-recording seam on the provider. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
EmbedKind only needs to be matched, never cloned or copied, so drop the unconditional derive; the existing by-value match in the #[cfg(test)] recording block still compiles since it binds nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
What this changes
EmbeddingProvidergains a defaultedembed_querymethod, andAgenticDBroutes search queries through it.Why
Several embedding models are asymmetric: they expect a different input form for a search query than for a stored passage. BGE models prefix queries with
Represent this sentence for searching relevant passages:, and E5 models usequery:againstpassage:. Embedding a query as a passage does not error. It just places the query vector in a different region of the space than the passages occupy, which costs retrieval quality quietly.LatticeEmbeddingalready implements this split correctly, but only as an inherent method (embeddings.rs:996).AgenticDBholds its provider asBoxedEmbeddingProvider = Arc<dyn EmbeddingProvider>(embeddings.rs:1227), and trait-object dispatch cannot reach an inherent method, so the correct behaviour was unreachable from the one place that needed it. All ten embedding call sites inagenticdb.rsfunnelled throughgenerate_text_embeddingintoembed(), queries included.What it does
EmbeddingProvider::embed_query, defaulted to forward toembed. Every existing implementor keeps its current behaviour with no code change and no API break. Symmetric providers are correct as they stand and stay correct.LatticeEmbedding, so its existing query-instruction handling becomes reachable through a boxed provider.AgenticDB::generate_query_embeddingalongsidegenerate_text_embeddingand routes the five search paths through it (:318,:409,:530,:1047,:1249). The five ingest paths (:272,:367,:481,:1019,:1219) are unchanged and still embed as passages, which is correct for them.Tests
embed_query_defaults_to_embed_for_symmetric_providerspins the default, so a symmetric provider such asHashEmbeddingreturns identical vectors from both methods.searches_embed_their_query_on_the_query_sideuses a recording provider that logs which side each call took, and pins all ten paths: five query, five passage. Both sides return the same vector, so only the call log distinguishes them and the test cannot pass by coincidence.cargo test -p ruvector-core --libgives 230 passed, 0 failed.I also checked that the second test is load-bearing rather than decorative. Reverting
generate_query_embeddingto callembed()makes it fail; restoring the fix makes it pass again.Note on OnnxEmbedding
OnnxEmbeddinghas no prefix handling today, so it inherits the default and behaves exactly as it does now. If query-side instructions are wanted there later, this is the seam to hang them on. Happy to follow up with that if it would be useful.Note on this PR's CI.
Tests (core-and-rest)is red on every branch in this repository,including
main: the job is cancelled at its 240-minute cap while still compiling and neverreaches the test phase. #786 restores the exclusion list that the shard's
packages:value losesto a shell comment, #784 unblocks the
ruvector-filtertest target that the compiler cannotfinish, and #787 fixes a deadlock waiting behind both. That failure is not caused by this branch.
Tests (vector-index)is also red, onruvector-graph typed_graph::tests::indexed_path_finds_top_result_and_traverses, and that one isworth reporting on its own account. It asserts that an HNSW search over 301 points on an arc of
the unit circle returns the exact match at angle 0 first; the CI run got
d2(angle ≈ 0.208 rad,cosine ≈ 0.978) instead. There is no tie to break — it is a recall miss that the over-fetch and
exact rescore did not cover. It passes locally on this branch and on
main(
cargo nextest run -p ruvector-graph -E 'test(indexed_path_finds_top_result_and_traverses)',1 passed on both), so it looks like ANN construction nondeterminism rather than anything this
branch does — this PR touches the embedding provider's query side, and that test passes raw
vectors in without going near a provider.