Fixing ModelLoaderUtils.split() to pass tests #126009
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
Prior to these changes, the split method would fail tests. Additionally, the method had code which could be refactored. A new variable (numRanges) was introduced to replace the direct usage of numStreams. The method was refactored to make the code easier to understand. Javadocs were updated. Tests for this method now pass.
elasticsearchmachine
added
v9.1.0
needs:triage
elasticsearchmachine
removed
the
needs:triage
|
Pinging @elastic/ml-core (Team:ML) |
|
@elasticmachine test this please |
|
Thank you for the contribution, and welcome. The code looks good, and I verified the tests pass now. Can you add a changelog to Example: https://github.com/elastic/elasticsearch/blob/main/docs/changelog/125650.yaml |
|
@elasticmachine test this please |
|
@elasticmachine test this please |
prwhelan
approved these changes
Apr 9, 2025
prwhelan
pushed a commit
to prwhelan/elasticsearch
that referenced
this pull request
Apr 9, 2025
Prior to these changes, the split method would fail tests. Additionally, the method had code which could be refactored. A new variable (numRanges) was introduced to replace the direct usage of numStreams. The method was refactored to make the code easier to understand. Javadocs were updated. Tests for this method now pass.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Apr 9, 2025
Prior to these changes, the split method would fail tests. Additionally, the method had code which could be refactored. A new variable (numRanges) was introduced to replace the direct usage of numStreams. The method was refactored to make the code easier to understand. Javadocs were updated. Tests for this method now pass. Co-authored-by: Jason-Whitmore <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The primary changes are to the split() method's functionality. A new variable,
numberOfRangesis created to ensure the return list has the correct length. The variablesbaseChunksPerRangeandremainderare now calculated differently to fit the new code structure.The new code structure completes all but the final range in a single for loop and removes the need for an if statement that adds the second to last range in some cases.
Changes are also made to the method Javadoc to accurately state what the method does.
A good way to test this change is to run:
./gradlew ":x-pack:plugin:ml-package-loader:test" --tests "org.elasticsearch.xpack.ml.packageloader.action.ModelLoaderUtilsTests.testSplitIntoRanges" -Dtests.iters=100000This test will fail on main, but passes on this PR.
Closes #121799