-
Couldn't load subscription status.
- Fork 58
Voyageai refactoring #412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Voyageai refactoring #412
Conversation
- contextual model - removing the model default value - token counting, ie. more effective use of batches
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
- contextual model - removing the model default value - token counting, ie. more effective use of batches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the VoyageAI integration to support contextual embedding models, improve token management, and enhance testing coverage. The changes include removing the default model value (requiring explicit model specification), implementing token-aware batching, adding support for the voyage-context-3 model, and updating tests to cover the new rerank-2.5 model.
Key Changes:
- Added token counting functionality and token-aware batching based on model-specific token limits
- Introduced support for contextualized embedding models (voyage-context-3) with automatic API endpoint detection
- Updated VoyageAI package dependency from 0.2.2 to 0.3.5
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/integration/test_vectorizers.py | Added VoyageAI-specific tests for token counting, context models, and batching; updated model references to voyage-3.5 |
| tests/integration/test_rerankers.py | Enhanced reranker test fixture to test both rerank-lite-1 and rerank-2.5 models |
| redisvl/utils/vectorize/text/voyageai.py | Implemented token-aware batching, context model support, removed default model value, added token limits dictionary |
| pyproject.toml | Updated voyageai dependency version requirement from 0.2.2 to 0.3.5 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| def __init__( | ||
| self, | ||
| model: str = "voyage-large-2", | ||
| model: str, |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the default model value is a breaking API change. Existing code that instantiates VoyageAITextVectorizer without specifying a model will fail. Consider adding a deprecation warning in a previous release or documenting this as a breaking change in the release notes.
| # Tokenize all texts in one API call for efficiency | ||
| try: | ||
| token_counts = self.count_tokens(texts) | ||
| except Exception as e: | ||
| raise ValueError(f"Failed to count tokens for batching: {e}") |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The token-aware batching makes an additional API call to count_tokens() for every embed operation. This adds latency and API usage overhead. Consider adding a parameter to allow users to opt out of token-aware batching or implement local tokenization if the VoyageAI client supports it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count_token() is a low-latency local function.
| texts: List of texts to embed | ||
| batch_size: Number of texts to process in each API call | ||
| **kwargs: Additional parameters to pass to the VoyageAI API | ||
| batch_size: Deprecated. Token-aware batching is now always used. |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The batch_size parameter is marked as deprecated in the docstring, but the method signature still accepts it without any deprecation warning being raised. Consider adding a runtime deprecation warning when batch_size is provided, or remove the parameter entirely if it's no longer used.
|
@fzowl There are some failures in CI: The job failed due to mypy type errors in redisvl/utils/vectorize/text/voyageai.py:
Solution:
This will resolve the mypy errors and allow the type checks to pass. Relevant file: redisvl/utils/vectorize/text/voyageai.py (ref: 90340c52bfc47b4944b2095e4e1b492b80bf2507) |
Voyageai refactoring: