[ML] Remove tasktype any from supportedStreamingTasks #121460
Conversation
jonathan-buttner
added
>refactoring
:ml
|
|
||
| private void inferOnService(Model model, Request request, InferenceService service, ActionListener<InferenceServiceResults> listener) { | ||
| if (request.isStreaming() == false || service.canStream(request.getTaskType())) { | ||
| if (request.isStreaming() == false || service.canStream(model.getTaskType())) { |
There was a problem hiding this comment.
Use the model's task type to determine streaming (can never be any).
|
|
||
| @Override | ||
| public boolean canHandleStreamingResponses() { | ||
| return canHandleStreamingResponses; |
There was a problem hiding this comment.
Moved here to the base class.
| public abstract class AmazonBedrockResponseHandler implements ResponseHandler { | ||
|
|
||
| @Override | ||
| public boolean canHandleStreamingResponses() { |
There was a problem hiding this comment.
This is a bit unfortunate. This response handler is a workaround and isn't really used because this service uses its own client. AmazonBedrock does support streaming the responses but its handled in a separate place.
There was a problem hiding this comment.
Eh we need to separate Bedrock from everything after Sender anyway, since it has its own internal threadpool and all the stuff we wrap around the http client
| } | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Moved this class to the test directory since that's the only place it's referenced.
|
Pinging @elastic/ml-core (Team:ML) |
…asticsearch into ml-remove-stream-any
| public abstract class AmazonBedrockResponseHandler implements ResponseHandler { | ||
|
|
||
| @Override | ||
| public boolean canHandleStreamingResponses() { |
There was a problem hiding this comment.
Eh we need to separate Bedrock from everything after Sender anyway, since it has its own internal threadpool and all the stuff we wrap around the http client
* Refactoring supported streaming functionality * Moving always retrying handler to the tests * Fixing comment
* Refactoring supported streaming functionality * Moving always retrying handler to the tests * Fixing comment (cherry picked from commit 7f284d3) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/external/openai/OpenAiResponseHandler.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/elastic/ElasticInferenceServiceTests.java
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…#123927) * [ML] Remove tasktype any from supportedStreamingTasks (#121460) * Refactoring supported streaming functionality * Moving always retrying handler to the tests * Fixing comment (cherry picked from commit 7f284d3) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/external/openai/OpenAiResponseHandler.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/elastic/ElasticInferenceServiceTests.java * Fixing test
3 participants
This PR refactors some logic that currently requires
TaskType.ANYto be returned in thesupportedStreamingTaskscall. If the task type is not included in the request action in the REST API call then the task type is set toANY.We perform validation on the request and backing model to ensure that in the request task type is the same as the backing model or the request task type is
ANY, indicating that it wasn't included.This means that the request task type must match the model (aside from being
ANY). So we can pass in the backing model task type instead when determining if the service can support streaming.There were a number of classes that relied on the field:
I moved this into the response handle's base class.