-
Notifications
You must be signed in to change notification settings - Fork 25.5k
[ML] Fix model assignment error handling and assignment explanation generation #133916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Fix model assignment error handling and assignment explanation generation #133916
Conversation
…fix/not-enough-memory-exception
Hi @valeriy42, I've created a changelog YAML for you. |
…valeriy42/elasticsearch into fix/not-enough-memory-exception
Pinging @elastic/ml-core (Team:ML) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes model assignment error handling to prevent internal IllegalArgumentException
from leaking to upper layers when there's insufficient memory on a node. The fix adds defensive checks using the canAssign
method before attempting to assign models to nodes.
- Adds
canAssign
checks before model assignments to prevent memory-related exceptions - Changes the visibility of
canAssign
method from package-private to public for broader access - Updates test naming to better reflect the test's purpose of explaining missing allocations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
AssignmentPlan.java | Changes canAssign method visibility from package-private to public |
TrainedModelAssignmentRebalancer.java | Adds canAssign check before assignModelToNode call with proper control flow |
ZoneAwareAssignmentPlanner.java | Adds canAssign check before assignModelToNode call with proper control flow |
TrainedModelAssignmentRebalancerTests.java | Updates test method name to better reflect its purpose |
133916.yaml | Adds changelog entry documenting the bug fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...n/java/org/elasticsearch/xpack/ml/inference/assignment/TrainedModelAssignmentRebalancer.java
Outdated
Show resolved
Hide resolved
...l/src/main/java/org/elasticsearch/xpack/ml/inference/assignment/planning/AssignmentPlan.java
Outdated
Show resolved
Hide resolved
.../test/java/org/elasticsearch/xpack/ml/inference/assignment/planning/AssignmentPlanTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM; just a small issue
…fix/not-enough-memory-exception
…eneration (elastic#133916) This PR ensures that in case of insufficient memory on a node, the internal IllegalArgumentException from assignModelToNode is not leaked to the upper layers of the architecture by first checking that the model can be assigned to the node. The check canAssign() is now moved into the function assignModelToNode().
…eneration (elastic#133916) This PR ensures that in case of insufficient memory on a node, the internal IllegalArgumentException from assignModelToNode is not leaked to the upper layers of the architecture by first checking that the model can be assigned to the node. The check canAssign() is now moved into the function assignModelToNode().
This PR ensures that in case of insufficient memory on a node, the internal
IllegalArgumentException
fromassignModelToNode
is not leaked to the upper layers of the architecture by first checking that the model can be assigned to the node. The checkcanAssign()
is now moved into the functionassignModelToNode()
.