[ML] Limit in flight requests when indexing model download parts#112992
Merged
[ML] Limit in flight requests when indexing model download parts#112992
Conversation
Collaborator
|
Pinging @elastic/ml-core (Team:ML) |
pxsalehi
reviewed
Sep 17, 2024
Member
pxsalehi
left a comment
There was a problem hiding this comment.
LGTM for the newer changes. Although, ideally there should be a test that shows the number of parallel downloads is limited. In any case, I'm gonna leave the final LGTM to ML, if that's ok.
prwhelan
approved these changes
Sep 17, 2024
| true | ||
| ); | ||
|
|
||
| client.execute(PutTrainedModelDefinitionPartAction.INSTANCE, modelPartRequest).actionGet(); |
Member
There was a problem hiding this comment.
This is the part that is blocking and/or slowing down the indexing, correct? We'll wait for the client response rather than continue asynchronously?
Member
Author
There was a problem hiding this comment.
The gain is from using 5 threads to stream the download and index the parts. The non-blocking write meant that we had more than 5 in flight requests (the download is faster than the indexing) and that was causing the OOM. In order to limit the number of requests to at most 5 there has to be some element of blocking. Model download uses it's a dedicated thread pool so the block does not starve other parts of the code of resources
Comment on lines
+128
to
+134
| for (int i = 0; i < ranges.size() - 1; i++) { | ||
| assertThat(ranges.get(i).rangeStart(), is(startBytes)); | ||
| long end = startBytes + ((long) ranges.get(i).numParts() * chunkSize) - 1; | ||
| assertThat(ranges.get(i).rangeEnd(), is(end)); | ||
| long expectedNumBytesInRange = (long) chunkSize * ranges.get(i).numParts() - 1; | ||
| assertThat(ranges.get(i).rangeEnd() - ranges.get(i).rangeStart(), is(expectedNumBytesInRange)); | ||
| assertThat(ranges.get(i).startPart(), is(startPartIndex)); |
Member
Author
Member
Author
|
@elasticmachine update branch |
davidkyle
added a commit
to davidkyle/elasticsearch
that referenced
this pull request
Sep 25, 2024
…stic#112992) Restores the changes from elastic#111684 which uses multiple streams to improve the time to download and install the built in ml models. The first iteration has a problem where the number of in-flight requests was not properly limited which is fixed here. Additionally there are now circuit breaker checks on allocating the buffer used to store the model definition.
Collaborator
💔 Backport failed
You can use sqren/backport to manually backport by running |
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 25, 2024
…2992) (#113514) Restores the changes from #111684 which uses multiple streams to improve the time to download and install the built in ml models. The first iteration has a problem where the number of in-flight requests was not properly limited which is fixed here. Additionally there are now circuit breaker checks on allocating the buffer used to store the model definition.
davidkyle
added a commit
to davidkyle/elasticsearch
that referenced
this pull request
Sep 27, 2024
…stic#112992) Restores the changes from elastic#111684 which uses multiple streams to improve the time to download and install the built in ml models. The first iteration has a problem where the number of in-flight requests was not properly limited which is fixed here. Additionally there are now circuit breaker checks on allocating the buffer used to store the model definition. # Conflicts: # x-pack/plugin/ml-package-loader/src/main/java/org/elasticsearch/xpack/ml/packageloader/action/TransportLoadTrainedModelPackage.java # x-pack/plugin/ml-package-loader/src/test/java/org/elasticsearch/xpack/ml/packageloader/action/TransportLoadTrainedModelPackageTests.java
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 27, 2024
…2992) (#113710) Restores the changes from #111684 which uses multiple streams to improve the time to download and install the built in ml models. The first iteration has a problem where the number of in-flight requests was not properly limited which is fixed here. Additionally there are now circuit breaker checks on allocating the buffer used to store the model definition. # Conflicts: # x-pack/plugin/ml-package-loader/src/main/java/org/elasticsearch/xpack/ml/packageloader/action/TransportLoadTrainedModelPackage.java # x-pack/plugin/ml-package-loader/src/test/java/org/elasticsearch/xpack/ml/packageloader/action/TransportLoadTrainedModelPackageTests.java
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
#111684 improved the model install time by using multiple streams and threads to download and write the model parts. The change was reverted in #112961 after it was discovered the be the cause of Out Of Memory exceptions.
The design relied on using a fixed size thread pool to limit the concurrent downloads and hence also manage memory usage. However, the indexing of the downloaded document was performed async which meant a new download request would be forked and executed while the write request was still in flight leading to large numbers of in flight requests. The fix here is to block on the index write.
The first commit is the revert of the revert, the later commits introduce the blocking write and reuse a byte buffer that was being recreated for every downloaded part. Allocating that byte buffer is now protected by a circuit breaker.
Labelled as a non issue because the code that caused the OOM was reverted before it made it to a production environment.