Adding VoyageAI integration#35273
Conversation
bjorncs
left a comment
There was a problem hiding this comment.
Thank you for your contribution, and for taking the time to improve Vespa!
Overall the changes look good and align well with our codebase.
Here are some initial feedback. We'll come back with more later.
model-integration/src/test/java/ai/vespa/embedding/VoyageAIEmbedderIntegrationTest.java
Show resolved
Hide resolved
config-model/src/main/java/com/yahoo/vespa/model/container/component/VoyageAIEmbedder.java
Outdated
Show resolved
Hide resolved
| element model { xsd:string }? & | ||
| element api-key-secret-name { xsd:string } & | ||
| element endpoint { xsd:string }? & | ||
| element max-batch-size { xsd:positiveInteger }? & |
There was a problem hiding this comment.
The max batch size parameter is not used by the implementation. Remove?
There was a problem hiding this comment.
Please also remove the max-batch-size definition here.
There was a problem hiding this comment.
We want to move the user facing documentation to our documentation repository (https://github.com/vespa-engine/documentation, https://docs.vespa.ai/en/embedding.html). Please create a PR there after we have settled on the design.
For any implementation details just add them as (javadoc) comments to VoyageAIEmbedder.
There was a problem hiding this comment.
@bjorncs Sure, i'll check the repo and raise a PR.
There was a problem hiding this comment.
|
@fzowl Some unit tests are failing in the |
model-integration/src/main/java/ai/vespa/embedding/VoyageAIEmbedder.java
Outdated
Show resolved
Hide resolved
| element model { xsd:string }? & | ||
| element api-key-secret-name { xsd:string } & | ||
| element endpoint { xsd:string }? & | ||
| element max-batch-size { xsd:positiveInteger }? & |
There was a problem hiding this comment.
Please also remove the max-batch-size definition here.
| element normalize { xsd:boolean }? & | ||
| element truncate { xsd:boolean }? & | ||
| element pool-size { xsd:positiveInteger }? & | ||
| element cache-size { xsd:nonNegativeInteger }? |
There was a problem hiding this comment.
See my comment regarding caching.
| return new OkHttpClient.Builder() | ||
| .connectTimeout(Duration.ofMillis(config.timeout())) | ||
| .readTimeout(Duration.ofMillis(config.timeout())) | ||
| .writeTimeout(Duration.ofMillis(config.timeout())) |
There was a problem hiding this comment.
Please specify the callTimeout as well for a proper request-response timeout on the client side.
We want to later include a timeout value in the Context, so timeout can be tied to the logical operation (respect overall feed/query time). For now we'll use the timeout configured in the embedder, but later will override the call timeout per request and use the lowest of those two.
model-integration/src/main/java/ai/vespa/embedding/VoyageAIEmbedder.java
Show resolved
Hide resolved
model-integration/src/main/java/ai/vespa/embedding/VoyageAIEmbedder.java
Show resolved
Hide resolved
|
@bjorncs, can you please double-check the PR? |
bjorncs
left a comment
There was a problem hiding this comment.
See comments. The PR build fails with a compilation error:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.13.0:compile (default-compile) on project config-model: Compilation failure: Compilation failure:
--
| [ERROR] /workspace/build/buildkite/vespaai/vespa-engine-vespa/config-model/src/main/java/com/yahoo/vespa/model/container/component/VoyageAIEmbedder.java:[167,20] cannot find symbol
| [ERROR] symbol: method poolSize(java.lang.Integer)
| [ERROR] location: variable builder of type com.yahoo.embedding.voyageai.VoyageAiEmbedderConfig.Builder
| [ERROR] /workspace/build/buildkite/vespaai/vespa-engine-vespa/config-model/src/main/java/com/yahoo/vespa/model/container/component/VoyageAIEmbedder.java:[170,20] cannot find symbol
| [ERROR] symbol: method cacheSize(java.lang.Integer)
| [ERROR] location: variable builder of type com.yahoo.embedding.voyageai.VoyageAiEmbedderConfig.Builder
|
|
||
| VoyageAIEmbedder = | ||
| attribute type { "voyage-ai-embedder" } & | ||
| element model { xsd:string }? & |
There was a problem hiding this comment.
In your API the 'model' parameter is required. We should do the same here to avoid having the default model being defined by Vespa.
| endpoint string default="https://api.voyageai.com/v1/embeddings" | ||
|
|
||
| # VoyageAI model name (e.g., voyage-3.5, voyage-3.5-lite, voyage-code-3, voyage-finance-2) | ||
| model string default="voyage-3.5" |
There was a problem hiding this comment.
Please remove the default value (see my comment regarding <model>).
|
@bjorncs Ah, sorry about these. I corrected the issues and updated the documentation PR as well. vespa-engine/documentation#4237 |
|
FYI some unit tests failed after making |
|
@bjorncs Should be fine now. |
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.