[ML] Make Streaming Results Writeable #122527
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make streaming elements extend Writeable and create StreamInput constructors so we can publish elements across nodes using the transport layer. Additional notes: - Moved optional methods into the InferenceServiceResults interface and default them - StreamingUnifiedChatCompletionResults elements are now all records
prwhelan
added
>refactoring
:ml
Collaborator
|
Pinging @elastic/ml-core (Team:ML) |
jonathan-buttner
approved these changes
Feb 14, 2025
Contributor
jonathan-buttner
left a comment
jonathan-buttner
left a comment
There was a problem hiding this comment.
Should we backport this to 8.19?
| * <p>For other results like SparseEmbeddingResults, this method can be a pass through to the transformToLegacyFormat.</p> | ||
| */ | ||
| List<? extends InferenceResults> transformToCoordinationFormat(); | ||
| default List<? extends InferenceResults> transformToCoordinationFormat() { |
| public boolean equals(Object o) { | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| Results other = (Results) o; | ||
| return dequeEquals(this.results, other.results()); |
Contributor
There was a problem hiding this comment.
Ah was this why you were saying deque was a bad idea haha?
Member
Author
There was a problem hiding this comment.
Yeah, it took me a few minutes to realize most of the Deque implementations do not have an equals method =(
|
|
||
| public String getModel() { | ||
| return model; | ||
| private static boolean dequeEquals(Deque<?> thisDeque, Deque<?> otherDeque) { |
Contributor
There was a problem hiding this comment.
Looks like we use this in a couple places, do you think it'd be worth moving it to a utility function?
|
|
||
| public Usage getUsage() { | ||
| return usage; | ||
| private static int dequeHashCode(Deque<?> deque) { |
Contributor
There was a problem hiding this comment.
Same here maybe move to a utility function.
prwhelan
added a commit
to prwhelan/elasticsearch
that referenced
this pull request
Feb 21, 2025
Make streaming elements extend Writeable and create StreamInput constructors so we can publish elements across nodes using the transport layer. Additional notes: - Moved optional methods into the InferenceServiceResults interface and default them - StreamingUnifiedChatCompletionResults elements are now all records
elasticsearchmachine
pushed a commit
that referenced
this pull request
Feb 21, 2025
* [ML] Make Streaming Results Writeable (#122527) Make streaming elements extend Writeable and create StreamInput constructors so we can publish elements across nodes using the transport layer. Additional notes: - Moved optional methods into the InferenceServiceResults interface and default them - StreamingUnifiedChatCompletionResults elements are now all records * [CI] Auto commit changes from spotless --------- Co-authored-by: elasticsearchmachine <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make streaming elements extend Writeable and create StreamInput constructors so we can publish elements across nodes using the transport layer.
Additional notes: