[ML] Restore delayed string copying #135242

DonalEvans · 2025-09-23T00:14:29Z

This commit restores the behaviour introduced in #125837 which was inadvertently undone by changes in #121041, specifically, delaying copying Strings as part of calling Request.chunkText() until the request is being executed.

In addition to the above change, refactor doChunkedInfer() and its implementations to take a List rather than EmbeddingsInput, since the EmbeddingsInput passed into doChunkedInfer() was immediately discarded after extracting the ChunkInferenceInput list from it. This change allowed the EmbeddingsInput class to be refactored to not know about ChunkInferenceInput, simplifying it significantly.

This commit also simplifies EmbeddingRequestChunker.Request to take only the input String rather than the entire list of all inputs, since only one input is actually needed. This change prevents Requests from retaining a reference to the input list, potentially allowing it to be GC'd faster.

This commit restores the behaviour introduced in elastic#125837 which was inadvertently undone by changes in elastic#121041, specifically, delaying copying Strings as part of calling Request.chunkText() until the request is being executed. In addition to the above change, refactor doChunkedInfer() and its implementations to take a List<ChunkInferenceInput> rather than EmbeddingsInput, since the EmbeddingsInput passed into doChunkedInfer() was immediately discarded after extracting the ChunkInferenceInput list from it. This change allowed the EmbeddingsInput class to be refactored to not know about ChunkInferenceInput, simplifying it significantly. This commit also simplifies EmbeddingRequestChunker.Request to take only the input String rather than the entire list of all inputs, since only one input is actually needed. This change prevents Requests from retaining a reference to the input list, potentially allowing it to be GC'd faster.

elasticsearchmachine · 2025-09-24T16:32:32Z

Pinging @elastic/ml-core (Team:ML)

jonathan-buttner

Thanks for the changes, I left a few suggestions

jonathan-buttner · 2025-09-24T17:03:44Z

...ce/src/main/java/org/elasticsearch/xpack/inference/external/http/sender/EmbeddingsInput.java

-        return getInputs().stream().map(ChunkInferenceInput::input).collect(Collectors.toList());
+    public List<String> getInputs() {
+        // The supplier should only be invoked once
+        assert inputListSupplier != null;


The assert is removed in production so if this is called twice I believe it'll produce a null pointer. What if we wrapped the call in an atomic boolean and did a compareAndSet and if it has already been called we through an exception explaining why that's not allowed?

I assume we don't want to cache the result of inputListSupplier.get() in memory within this class right? If we were ok with that then we could do something with an atomic reference and locking to ensure it only gets set once and just returned after that.

This probably overkill but I think we only care about the situation when the class is initialized with a supplier. If the constructor were passed a List<String>, I don't think we need to guard against that supplier we create being called multiple times. If we did want to allow multiple calls in that scenario we could use a custom internal class that wraps the List<String> and allows multiple calls. For the supplier scenario we'd have a different internal class that does the atomic boolean check and throws.

...ce/src/main/java/org/elasticsearch/xpack/inference/external/http/sender/EmbeddingsInput.java

jonathan-buttner

Thanks for the changes!

elasticsearchmachine · 2025-09-25T17:29:42Z

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	9.1	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 135242

This commit restores the behaviour introduced in elastic#125837 which was inadvertently undone by changes in elastic#121041, specifically, delaying copying Strings as part of calling Request.chunkText() until the request is being executed. In addition to the above change, refactor doChunkedInfer() and its implementations to take a List<ChunkInferenceInput> rather than EmbeddingsInput, since the EmbeddingsInput passed into doChunkedInfer() was immediately discarded after extracting the ChunkInferenceInput list from it. This change allowed the EmbeddingsInput class to be refactored to not know about ChunkInferenceInput, simplifying it significantly. This commit also simplifies EmbeddingRequestChunker.Request to take only the input String rather than the entire list of all inputs, since only one input is actually needed. This change prevents Requests from retaining a reference to the input list, potentially allowing it to be GC'd faster. (cherry picked from commit f3447d3) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/ai21/Ai21Service.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/llama/LlamaService.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/llama/action/LlamaActionCreator.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/mistral/MistralService.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/SenderServiceTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/alibabacloudsearch/AlibabaCloudSearchCompletionRequestManagerTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/alibabacloudsearch/action/AlibabaCloudSearchActionCreatorTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureaistudio/action/AzureAiStudioActionAndCreatorTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/llama/action/LlamaActionCreatorTests.java

DonalEvans · 2025-09-25T17:54:03Z

💔 Some backports could not be created

Status	Branch	Result
✅	9.1
❌	8.19	Conflict resolution was aborted by the user

Manual backport

To create the backport manually run:

backport --pr 135242

Questions ?

Please refer to the Backport tool documentation

This commit restores the behaviour introduced in elastic#125837 which was inadvertently undone by changes in elastic#121041, specifically, delaying copying Strings as part of calling Request.chunkText() until the request is being executed. In addition to the above change, refactor doChunkedInfer() and its implementations to take a List<ChunkInferenceInput> rather than EmbeddingsInput, since the EmbeddingsInput passed into doChunkedInfer() was immediately discarded after extracting the ChunkInferenceInput list from it. This change allowed the EmbeddingsInput class to be refactored to not know about ChunkInferenceInput, simplifying it significantly. This commit also simplifies EmbeddingRequestChunker.Request to take only the input String rather than the entire list of all inputs, since only one input is actually needed. This change prevents Requests from retaining a reference to the input list, potentially allowing it to be GC'd faster. (cherry picked from commit f3447d3) # Conflicts: # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/ai21/Ai21Service.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/llama/LlamaService.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/llama/action/LlamaActionCreator.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/mistral/MistralService.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/SenderServiceTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/alibabacloudsearch/AlibabaCloudSearchCompletionRequestManagerTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/alibabacloudsearch/action/AlibabaCloudSearchActionCreatorTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/azureaistudio/action/AzureAiStudioActionAndCreatorTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/services/llama/action/LlamaActionCreatorTests.java

DonalEvans · 2025-09-25T18:04:18Z

💚 All backports created successfully

Status	Branch	Result
✅	9.1
✅	8.19

Questions ?

Please refer to the Backport tool documentation

This commit restores the behaviour introduced in #125837 which was inadvertently undone by changes in #121041, specifically, delaying copying Strings as part of calling Request.chunkText() until the request is being executed. In addition to the above change, refactor doChunkedInfer() and its implementations to take a List<ChunkInferenceInput> rather than EmbeddingsInput, since the EmbeddingsInput passed into doChunkedInfer() was immediately discarded after extracting the ChunkInferenceInput list from it. This change allowed the EmbeddingsInput class to be refactored to not know about ChunkInferenceInput, simplifying it significantly. This commit also simplifies EmbeddingRequestChunker.Request to take only the input String rather than the entire list of all inputs, since only one input is actually needed. This change prevents Requests from retaining a reference to the input list, potentially allowing it to be GC'd faster. (cherry picked from commit f3447d3)

DonalEvans added >refactoring :ml Machine learning Team:ML Meta label for the ML team auto-backport Automatically create backport pull requests when merged v9.2.0 v8.19.5 v9.1.5 labels Sep 23, 2025

DonalEvans force-pushed the restore-delayed-string-copying branch from 311cbb7 to a6609a3 Compare September 23, 2025 15:44

elasticsearchmachine and others added 3 commits September 23, 2025 16:12

[CI] Update transport version definitions

8ef0b4a

Merge branch 'main' into restore-delayed-string-copying

782351d

Merge branch 'main' into restore-delayed-string-copying

832444f

DonalEvans marked this pull request as ready for review September 24, 2025 16:32

DonalEvans requested review from davidkyle and jonathan-buttner and removed request for davidkyle September 24, 2025 16:32

jonathan-buttner reviewed Sep 24, 2025

View reviewed changes

Use AtomicBoolean check instead of nulling out supplier, add tests

4d08a87

DonalEvans requested a review from jonathan-buttner September 24, 2025 20:05

jonathan-buttner approved these changes Sep 25, 2025

View reviewed changes

DonalEvans merged commit f3447d3 into elastic:main Sep 25, 2025
34 checks passed

DonalEvans deleted the restore-delayed-string-copying branch September 25, 2025 17:28

elasticsearchmachine added the backport pending label Sep 25, 2025

DonalEvans mentioned this pull request Sep 25, 2025

[9.1] [ML] Restore delayed string copying (#135242) #135463

Closed

DonalEvans mentioned this pull request Sep 25, 2025

[9.1] [ML] Restore delayed string copying (#135242) #135464

Merged

DonalEvans mentioned this pull request Sep 25, 2025

[8.19] [ML] Restore delayed string copying (#135242) #135469

Merged

DonalEvans removed the backport pending label Sep 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ML] Restore delayed string copying #135242

[ML] Restore delayed string copying #135242

Uh oh!

DonalEvans commented Sep 23, 2025

Uh oh!

elasticsearchmachine commented Sep 24, 2025

Uh oh!

jonathan-buttner left a comment

Uh oh!

jonathan-buttner Sep 24, 2025

Uh oh!

Uh oh!

jonathan-buttner left a comment

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 25, 2025

Uh oh!

DonalEvans commented Sep 25, 2025

Uh oh!

DonalEvans commented Sep 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[ML] Restore delayed string copying #135242

[ML] Restore delayed string copying #135242

Uh oh!

Conversation

DonalEvans commented Sep 23, 2025

Uh oh!

elasticsearchmachine commented Sep 24, 2025

Uh oh!

jonathan-buttner left a comment

Choose a reason for hiding this comment

Uh oh!

jonathan-buttner Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonathan-buttner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Sep 25, 2025

💔 Backport failed

Uh oh!

DonalEvans commented Sep 25, 2025

💔 Some backports could not be created

Manual backport

Questions ?

Uh oh!

DonalEvans commented Sep 25, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants