[ML] Remove Voyageai request manager classes #124512
Conversation
jonathan-buttner
added
>non-issue
:ml
…ence-remove-voyage-request-managers
| @@ -34,5 +34,9 @@ public static URI buildUri(URI accountUri, String service, CheckedSupplier<URI, | |||
| } | |||
| } | |||
|
|
|||
| 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.
| @@ -77,11 +71,7 @@ public boolean[] getTruncationInfo() { | |||
| return null; | |||
| } | |||
|
|
|||
| public VoyageAIEmbeddingsTaskSettings getTaskSettings() { | |||
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
||
| 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
| @@ -73,22 +88,20 @@ public SecureString apiKey() { | |||
| return apiKey; | |||
| } | |||
|
|
|||
| public VoyageAIRateLimitServiceSettings rateLimitServiceSettings() { | |||
|
|
||
| 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 service, | ||
| String url, |
There was a problem hiding this comment.
For testing we allow a string url.
| @@ -339,26 +341,6 @@ public void testExecute_ThrowsElasticsearchException_WhenSenderOnFailureIsCalled | |||
| 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) |
jonathan-buttner
deleted the
ml-inference-remove-voyage-request-managers
branch
💚 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