Repository navigation
Conversation
|
|
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60513c8c4b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| client = self._get_client() | ||
| try: | ||
| encodings = await asyncio.to_thread(client.tokenize, texts, self.model_name) | ||
| except Exception as exc: |
There was a problem hiding this comment.
When client.tokenize raises an unrelated programming or SDK-contract error rather than the expected offline tokenizer-download failure, this blanket handler permanently disables token-aware packing and proceeds with count-only batches, which can then exceed the model's token ceiling while hiding the original defect. Catch only the specific load/network failures intended for this fallback and let unexpected exceptions propagate.
AGENTS.md reference: AGENTS.md:L133-L134
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 95ad293: the fallback now catches only OSError (every Hugging Face download failure is one) and ImportError (missing tokenizers); any other exception propagates. test_unexpected_tokenizer_errors_are_not_swallowed covers it.
Add a voyage embedding provider (voyage-4*, voyage-3*, voyage-code-*, voyage-finance-2, voyage-law-2 and the voyage-context-* models) and a voyage reranker provider (rerank-3, rerank-3-lite, rerank-2.5, rerank-2.5-lite) built on the official voyageai SDK, shipped as the optional basic-memory[voyage] extra. Neither provider has a default model: the model must be named explicitly and is validated against the Voyage catalog, along with the supported output dimensions. API errors surface the text of Voyage's detail field. Signed-off-by: fzowl <zoltan@voyageai.com>
60513c8 to
95ad293
Compare
|
Thanks for this. I've been running voyage-4-large + rerank-3 through the LiteLLM path on a mostly Traditional Chinese vault, so a first-class provider is welcome. One data point on why it matters for non-English users. On my own corpus (574 notes, 1,126 generated questions, hybrid search, same reranker), switching embeddings from embeddinggemma-300m to voyage-4-large moved HR@5 from 85.0% to 92.2%. Almost all of the gain was on paraphrased queries without identifiers (69.9% → 88.2%). Queries with an identifier embedded in a long sentence were slightly worse (99.5% → 96.0%). The corpus is private, so I can only share aggregates. On contextualized models: the description says each chunk is sent as its own document. That is the gap I described in #1999. Without grouping chunks per note, voyage-context-4 gets no cross-chunk context. Would it make sense to link #1999 here and treat per-note grouping as a follow-up? If per-note grouping lands, I can rerun the same question set against it and report numbers. |
|
Thanks, and thanks for the benchmark numbers. You're right. In this PR I kept the PR to what the current provider contract allows. Embedding all chunks of one note together would mean extending that contract and grouping chunks per note in the vector sync. That is a larger change to the indexing path, so it is not part of this PR. It is the gap described in #1999, which is the right place to track it. |
Basic Memory can embed notes and rerank search results through LiteLLM, but Voyage AI has no provider of its own: users need LiteLLM's model strings, per-model dimension settings, and no check that a model name is a real Voyage model.
This adds first-class
voyageproviders built on the officialvoyageaiSDK, shipped as the optionalbasic-memory[voyage]extra:semantic_embedding_provider=voyage):voyage-4-large,voyage-4,voyage-4-lite,voyage-code-4,voyage-3-large,voyage-3.5,voyage-3.5-lite,voyage-code-3,voyage-finance-2,voyage-law-2, plus the contextualizedvoyage-context-4andvoyage-context-3. Matryoshka models accept 2048/1024/512/256; the dimension is requested from Voyage only whensemantic_embedding_dimensionsis set. Chunks are embedded asdocument, queries asquery.reranker_provider=voyage):rerank-3,rerank-3-lite,rerank-2.5,rerank-2.5-lite.bge-small-en-v1.5config default), an unsupported dimension, or a batch size outside 1–1000 fails with an error that lists the valid values. Other providers keep their defaults.-lite, 320Kvoyage-4/voyage-3.5, 120K the rest) using the SDK tokenizer, and by the 1000-input limit; if the tokenizer cannot be loaded, only the input limit applies.{"detail": ...}body.VOYAGE_API_KEY(or the existing key setting) supplies the credential.enable_auto_chunking=trueandchunk_size=32000for indexed chunks, and no auto-chunking for queries. Basic Memory hands providers flat chunks already mixed across notes, so each chunk is its own document and gets no cross-chunk context; this is documented.Docs:
docs/semantic-search.mdand the README rerank section.Tested with new unit tests for the failure modes (bad or missing model, dimensions, batch size, API-imposed contextualized constraints, malformed or failed rerank responses; 100% coverage of the new modules), and live through
bm reindexandbm tool search --mode semanticwith every embedding model (native, 256 and 2048 dimensions where supported) and every rerank model.just test-voyage-liveruns an opt-in live check for a plain and a contextualized model.