fix(embeddings): token budget overflow — every multi-chunk file failed to embed [stacks on #7] - #10
Closed
NestorCanales wants to merge 5 commits into
Closed
Conversation
…efensively Every file larger than ~1 chunk failed to embed on every platform — ORT "Attempting to broadcast ... 512 by 516". Root cause: buildChunker passed MaxInputTokens (512) straight through as the chunk budget, but the embedder prepends the E5 prefix and the tokenizer adds special tokens AFTER chunking, so a chunk cut at exactly 512 reached the model at 516 tokens. The epic specified effective chunk size ≈ 480 for exactly this reason; the implementation skipped the reservation. Found via a real-world vault on Windows (all previous platform verifications used single-chunk test files); reproduced identically on macOS — cross-platform, latent since Phase 3. Two-layer fix: - app.go buildChunker: budget = MaxInputTokens − local.EmbedTokenReserve (32 → effective 480, matching the epic). - LocalEmbedder.embedBatch: defensively truncate any tokenized input to modelMaxTokens, preserving the trailing EOS token. This also protects EmbedQuery, which has no chunker — an over-long search query crashed inference on every platform. Tests: unit (truncateTokens table; fake-session proof the model never receives >512 tokens) + TestIntegrationLargeDocument, the real-pipeline multi-chunk + over-long-query case every platform smoke was missing. Failure exit criteria verified: previously-failing scenario now passes on macOS (16 chunks embedded); existing tests unchanged (cosine ordering 0.134/0.170/0.283 identical). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dropped) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…/10), observations recorded Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ent recorded Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…passes, status Complete-pending-review Orphan grep: no old Embed() callers; all model/dimension literals confined to the OpenAI provider path; store's documented dim<=0 fallback kept. Architecture: inward-only imports verified package-by-package, wiring in main.go, mocks for all domain interfaces, thin delivery layer. Formal pass: vet + untagged suite + tagged integration (incl. large-doc regression) + make build + stdio-mode semantic search, all green. One stale comment fixed (SearchOptions.Threshold → provider-aware DefaultThreshold). Final measured numbers recorded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Consolidated into #2 (single reviewable branch per Bo's review-workflow preference) — all commits from this PR are now in |
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
1 participant
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.
The bug the Windows field test caught
Indexing a real 109-file vault silently produced 26 files — exactly the ones ≤1.6 KB. Every file needing more than one chunk failed on every platform, latent since Phase 3: all previous platform verifications used tiny single-chunk test files.
Root cause (
512 by 516):buildChunkerusedMaxInputTokens()(512) as the chunk budget, but the E5 prefix + tokenizer special tokens are added after chunking → 516 tokens reach a 512-limit model → ORT hard failure, invisible thanks to stderr-only error logging (fixed separately in #8). The epic specified effective chunk size ≈ 480 for exactly this reason. The unchunked query path was equally exposed — a long search query crashed inference.Full report:
Documentation/Bugs/local-embed-token-budget-overflow.md.Fix
MaxInputTokens − EmbedTokenReserve(32) → effective 480, per the epic.embedBatchtruncates any tokenized input to the model limit (EOS-preserving) — protectsEmbedQueryand any future caller.Verification
TestIntegrationLargeDocument(multi-chunk doc + over-long query): failed before on macOS and Windows, passes on bothStacks on #7 (touches epic code:
buildChunker,local.go). Review order: #2→#3→#4→#6→#7→this.🤖 Generated with Claude Code