[ML] Remove Voyageai request manager classes#124512
[ML] Remove Voyageai request manager classes#124512jonathan-buttner merged 3 commits intoelastic:mainfrom
Conversation
…ence-remove-voyage-request-managers
| } | ||
|
|
||
| public static URI buildUri(String service, CheckedSupplier<URI, URISyntaxException> uriBuilder) { | ||
| return buildUri(null, service, uriBuilder); |
There was a problem hiding this comment.
Just a helper that converts a URI exception to an ElasticsearchStatusException.
| return null; | ||
| } | ||
|
|
||
| public VoyageAIEmbeddingsTaskSettings getTaskSettings() { |
|
|
||
| public abstract class VoyageAIModel extends Model { | ||
| public abstract class VoyageAIModel extends RateLimitGroupingModel { | ||
| private static final String DEFAULT_MODEL_FAMILY = "default_model_family"; |
There was a problem hiding this comment.
Moving the rate limiting logic inside the model class
| return apiKey; | ||
| } | ||
|
|
||
| public VoyageAIRateLimitServiceSettings rateLimitServiceSettings() { |
| String modelFamily = MODEL_TO_MODEL_FAMILY.getOrDefault(modelId, DEFAULT_MODEL_FAMILY); | ||
|
|
||
| public abstract ExecutableAction accept(VoyageAIActionVisitor creator, Map<String, Object> taskSettings, InputType inputType); | ||
| return Objects.hash(modelFamily, apiKey); |
There was a problem hiding this comment.
This is a notable change. Previously the request manager was not including the apiKey in the grouping. This didn't seem right to me though because it'd mean that all users who were using the same model id family would be rate limited together. From the voyageai docs it does seem like you can have more granular rate limits per project. This doesn't accomplish that but at least it's a step in that direction because we shouldn't be grouping all users together.
| // should only be used for testing | ||
| VoyageAIEmbeddingsModel( | ||
| String modelId, | ||
| String inferenceId, |
There was a problem hiding this comment.
Fixing naming
| String modelId, | ||
| String inferenceId, | ||
| String service, | ||
| String url, |
There was a problem hiding this comment.
For testing we allow a string url.
| MatcherAssert.assertThat(thrownException.getMessage(), is("Failed to send VoyageAI embeddings request. Cause: failed")); | ||
| } | ||
|
|
||
| public void testExecute_ThrowsElasticsearchException_WhenSenderOnFailureIsCalled_WhenUrlIsNull() { |
There was a problem hiding this comment.
I think this was copied from openai. The voyageai url cannot be specified in the service settings so it should never be null.
|
Pinging @elastic/ml-core (Team:ML) |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
* Removing voyage request managers * Fixing tests (cherry picked from commit 1bee2cc)
This PR refactors some of the VoyageAI logic:
VoyageAIModelbase classFollows the same pattern as the OpenAI PR: #124144