fix(embeddings): fail loud instead of returning zero-vector HF embeddings (closes #167)#184
Merged
Merged
Conversation
…ings Closes #167. `HuggingFaceEmbeddings::embed` previously returned `Ok(vec![0.0; dims])` behind the `neural-embeddings` feature flag, with only a `log::warn!` to hint at the placeholder behaviour. Zero vectors pass cleanly through the pipeline (chunking, vector store, similarity), producing semantically-meaningless retrieval results that look structurally fine. This was misleading enough that the README quickstart had to be switched away from HuggingFace in a follow-up to PR #164. Now `embed` returns `Err(GraphRAGError::Embedding)` whenever Candle inference is invoked, with a message pointing at the working `HttpEmbeddingProvider` alternatives. Adds two regression tests covering both the not-initialized and the marked-initialized paths. `neural.rs` rewritten from a one-line `// TODO` to a module-level doc comment that documents the gap, so the feature flag's promise lines up with the surface the crate actually offers. Drive-by: convert `///!` outer doc comments at the top of `huggingface.rs` to `//!` inner doc comments (clippy `suspicious_doc_comments`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `nix-check` fmt derivation (`cargo fmt -- --check`) failed on huggingface.rs:307 — a block match arm needs a trailing comma per the repo's rustfmt config. This was the only formatting diff; fixing it unblocks the fmt check (and lets the cancelled doc/tests/benches checks actually run). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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
2 participants
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.
Summary
HuggingFaceEmbeddings::embednow returnsErr(GraphRAGError::Embedding)instead of silently emittingvec![0.0; dims]when Candle inference is invoked. Closes graphrag-core: HuggingFace embeddings + neural.rs are stubs (return zero vectors) #167.graphrag-core/src/embeddings/neural.rsrewritten from a one-line// TODOinto a module-level doc that names the gap and points callers at the workingHttpEmbeddingProvideralternatives.///!→//!at the top ofhuggingface.rs(clippysuspicious_doc_comments).Why
The previous behaviour passed zero vectors through the rest of the pipeline (chunking, vector store, similarity), producing structurally-clean retrieval results with no semantic meaning. The only signal was a
log::warn!, which is easy to miss in production logs. This was misleading enough that the README quickstart had to be rewritten away from HuggingFace in a follow-up to PR #164.Real Candle-backed inference is the next step on the same issue and on phase 1.1 of the meta-epic (#182 / TDD). Failing loud is the prerequisite — it stops silently-wrong embeddings from shipping while the real implementation gets written.
Test plan
cargo test -p graphrag-core --features huggingface-hub --lib embeddings::huggingface— 5/5 pass (3 pre-existing + 2 new regression tests)cargo test -p graphrag-core --lib— 363/363 passcargo clippy -p graphrag-core --features huggingface-hub --all-targets -- -D warningscleanRefs
docs/tdd-multimodal-and-service.md🤖 Generated with Claude Code