Skip to content

Route search queries through the embedding provider's query side - #760

Open
ohdearquant wants to merge 3 commits into
ruvnet:mainfrom
ohdearquant:fix/embedding-provider-query-side
Open

Route search queries through the embedding provider's query side#760
ohdearquant wants to merge 3 commits into
ruvnet:mainfrom
ohdearquant:fix/embedding-provider-query-side

Conversation

@ohdearquant

@ohdearquant ohdearquant commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

What this changes

EmbeddingProvider gains a defaulted embed_query method, and AgenticDB routes 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 use query: against passage: . 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.

LatticeEmbedding already implements this split correctly, but only as an inherent method (embeddings.rs:996). AgenticDB holds its provider as BoxedEmbeddingProvider = 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 in agenticdb.rs funnelled through generate_text_embedding into embed(), queries included.

What it does

  • Adds EmbeddingProvider::embed_query, defaulted to forward to embed. 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.
  • Overrides it on LatticeEmbedding, so its existing query-instruction handling becomes reachable through a boxed provider.
  • Adds AgenticDB::generate_query_embedding alongside generate_text_embedding and 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_providers pins the default, so a symmetric provider such as HashEmbedding returns identical vectors from both methods.
  • searches_embed_their_query_on_the_query_side uses 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 --lib gives 230 passed, 0 failed.

I also checked that the second test is load-bearing rather than decorative. Reverting generate_query_embedding to call embed() makes it fail; restoring the fix makes it pass again.

Note on OnnxEmbedding

OnnxEmbedding has 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 never
reaches the test phase. #786 restores the exclusion list that the shard's packages: value loses
to a shell comment, #784 unblocks the ruvector-filter test target that the compiler cannot
finish, and #787 fixes a deadlock waiting behind both. That failure is not caused by this branch.

Tests (vector-index) is also red, on
ruvector-graph typed_graph::tests::indexed_path_finds_top_result_and_traverses, and that one is
worth 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.

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.
@ohdearquant

Copy link
Copy Markdown
Contributor Author

The red Tests (vector-index) on this PR is not caused by this change. Evidence, so it can be
checked rather than taken on trust:

The failure. ruvector_graph::typed_graph::tests::indexed_path_finds_top_result_and_traverses,
typed_graph.rs:809:

assertion `left == right` failed
  left: "d2"
 right: "winner"

This diff cannot reach that test. It touches exactly two files, both in ruvector-core:

crates/ruvector-core/src/agenticdb.rs
crates/ruvector-core/src/embeddings.rs

and ruvector-graph references neither surface. Grepped with positive controls on the same
invocation, so an empty result is a real absence rather than a broken query:

pattern in ruvector-graph/src control elsewhere
EmbeddingProvider / embed_query 0 28 in ruvector-core/src
AgenticDB / agenticdb 0 3 files in ruvector-core/src

The test also builds its vectors explicitly (vec![1.0, 0.0, 0.0] and an arc of the unit circle)
and queries with a literal slice, so no embedding provider is involved on any path.

It looks like ANN recall variance. winner sits at angle 0 and the returned d2 at
≈0.208 rad, so the index returned a near-neighbour instead of the exact match. Run solo,
single-threaded, at this PR's head, the test passed 12 out of 12 times locally. That is an
aarch64 host rather than the CI runner, so it does not prove the CI behaviour by itself — the
reachability argument above is the load-bearing part, and the repeat run is corroboration.

Same job on sibling PRs (#757, #758, #759) is green, so the shard is not broadly broken; this
is a specific test whose assertion depends on exact-match recall from an approximate index.

Happy to re-run the job if you would rather see it green before merging.

@ohdearquant
ohdearquant marked this pull request as draft August 2, 2026 14:37
@ohdearquant

Copy link
Copy Markdown
Contributor Author

Triage of the Tests (vector-index) failure on this PR's head, since it is the only red check.

The failing test is typed_graph::tests::indexed_path_finds_top_result_and_traverses in
ruvector-graph (typed_graph.rs:809), asserting res[0].seed_id == "winner" and getting
"d2". This PR changes only ruvector-core/src/agenticdb.rs and
ruvector-core/src/embeddings.rs; nothing in ruvector-graph is touched.

Evidence it is not introduced here:

  • The same test passes on this PR's exact head (738206ef) locally on aarch64/macOS:
    cargo test -p ruvector-graph --lib typed_graph::tests::indexed_path_finds_top_result_and_traverses
    1 passed; 0 failed.
  • It also passes on current main (74870553) on the same host, same command.
  • A main baseline from CI itself is not obtainable right now: the last eight Workspace CI
    runs on main all ended cancelled, so the branch has no completed run of this job to
    compare against.

What the numbers say about the failure mode: the test places winner exactly on the query
(score 1.0) and the decoys at ≥0.2 rad, so the runner-up scores cos(0.2) ≈ 0.980 and d2
scores ≈ 0.978. A result of "d2" therefore cannot be a scoring tie — winner, d0 and d1
all outscore it — it means the ANN candidate set handed to exact rescore did not contain them.
That is an HNSW recall failure in the search_then_traverse push-down on that runner, and the
0.022 margin rules out f32 rounding as the cause.

Filed separately as an issue so it is not lost inside this PR.

@ohdearquant
ohdearquant marked this pull request as ready for review August 3, 2026 18:08
ohdearquant and others added 2 commits August 3, 2026 14:58
…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>
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.

1 participant