-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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] adding new defer_definition_decompression parameter to put trained model API #77189
[ML] adding new defer_definition_decompression parameter to put trained model API #77189
Conversation
…ed model API This new parameter is a boolean parameter that allows users to put in a compressed model without it having to be inflated on the master node during the put request This is useful for system/module set up and then later having the model validated and fully parsed when it is being loaded on a node for usage
Pinging @elastic/ml-core (Team:ML) |
Pinging @elastic/clients-team (Team:Clients) |
…ained-model-improvements
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.
Some feedback on the API spec:
docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/api/ml.put_trained_model.json
Outdated
Show resolved
Hide resolved
docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/api/ml.put_trained_model.json
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.
LGTM from a spec perspective!
run elasticsearch-ci/packaging-tests-windows-sample |
@elasticmachine update branch |
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.
Added some minor suggestions, otherwise docs LGTM
|
||
`defer_definition_decompression`:: | ||
(Optional, boolean) | ||
Should the request defer definition decompression and skip relevant |
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.
Should the request defer definition decompression and skip relevant | |
If set to `true` and a `compressed_definition` is provided, the request defers definition decompression and skips relevant |
`defer_definition_decompression`:: | ||
(Optional, boolean) | ||
Should the request defer definition decompression and skip relevant | ||
validations when a `compressed_definition` is provided. |
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.
If my first suggestion is accepted, this is the second half:
validations when a `compressed_definition` is provided. | |
validations. |
(Optional, boolean) | ||
Should the request defer definition decompression and skip relevant | ||
validations when a `compressed_definition` is provided. | ||
This would be useful for systems or users that know a good JVM heap size estimate for their |
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.
This would be useful for systems or users that know a good JVM heap size estimate for their | |
This deferral is useful for systems or users that know a good JVM heap size estimate for their |
Should the request defer definition decompression and skip relevant | ||
validations when a `compressed_definition` is provided. | ||
This would be useful for systems or users that know a good JVM heap size estimate for their | ||
model and that their model is valid and likely won't fail during inference. |
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.
model and that their model is valid and likely won't fail during inference. | |
model and know that their model is valid and likely won't fail during inference. |
"defer_definition_decompression": { | ||
"required": false, | ||
"type": "boolean", | ||
"description": "Should the action skip decompressing the definition to validate it and set default values", |
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.
Since ideally the API docs will ultimately be generated from specs, I think this should match what's in the other asciidoc file. e.g.
"description": "Should the action skip decompressing the definition to validate it and set default values", | |
"description": "If set to `true` and a `compressed_definition` is provided, the request defers definition decompression and skips relevant validations.", |
validationException.addValidationError( | ||
"when [" | ||
+ DEFER_DEFINITION_DECOMPRESSION | ||
+ "] is true and a compressed definition is provided, estimated_heap_memory_usage_bytes must be set" |
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.
+ "] is true and a compressed definition is provided, estimated_heap_memory_usage_bytes must be set" | |
+ "] is true and a compressed definition is provided, [" + ESTIMATED_HEAP_MEMORY_USAGE_BYTES + "] must be set" |
@@ -138,11 +141,16 @@ protected void masterOperation(Task task, | |||
minCompatibilityVersion.toString())); | |||
return; | |||
} | |||
} else if (state.nodes().getMinNodeVersion().before(state.nodes().getMaxNodeVersion()) |
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.
Could we move this check at the top of masterOperation
?
In addition, do we really need this check? I'm trying to think what happens if a user starts a rolling upgrade to the cluster and installs a fleet package that tries to put a model with defer_definition_compression
. Is it worth failing the request? If we allowed it what would break? I assume if the model was loaded in an older node it would fail as the definition would be missing. Is that preferable?
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.
@dimitris-athanasiou I am being paranoid for sure. My concern is that we have no way of determining if the model definition can be inflated on the current min node version or not. Previously, we were able to validate that. I guess its "buyer beware" and I can remove this check, but they could get an ugly parsing error. Which, I suppose, is the case anyways.
…ained-model-improvements
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.
LGTM
…ed model API (elastic#77189) This new parameter is a boolean parameter that allows users to put in a compressed model without it having to be inflated on the master node during the put request This is useful for system/module set up and then later having the model validated and fully parsed when it is being loaded on a node for usage
… trained model API (#77189) (#77256) * [ML] adding new defer_definition_decompression parameter to put trained model API (#77189) This new parameter is a boolean parameter that allows users to put in a compressed model without it having to be inflated on the master node during the put request This is useful for system/module set up and then later having the model validated and fully parsed when it is being loaded on a node for usage
* master: (128 commits) Mute DieWithDignityIT (elastic#77283) Fix randomization in MlNodeShutdownIT (elastic#77281) Add target_node_name for REPLACE shutdown type (elastic#77151) [DOCS] Adds information about version compatibility headers (elastic#77096) Fix template equals when mappings are wrapped (elastic#77008) Fix TextFieldMapper Retaining a Reference to its Builder (elastic#77251) Move die with dignity to be a test module (elastic#77136) Update task names for rest compatiblity (elastic#75267) [ML] adjusting bwc serialization for elastic#77256 (elastic#77257) Move `index.hidden` from Static to Dynamic settings (elastic#77218) Handle cgroups v2 in `OsProbe` (elastic#77128) Choose postings format from FieldMapper instead of MappedFieldType (elastic#77234) Add segment sorter for data streams (elastic#75195) Update skip after backport (elastic#77212) [ML] adding new defer_definition_decompression parameter to put trained model API (elastic#77189) [ML] Fix bug in inference stats persister for when feature reset is called Only check replicas in cancelling existing recoveries. (elastic#60564) Format `AbstractFilteringTestCase` (elastic#77217) [DOCS] Fixes line breaks. (elastic#77248) Convert 'routing' values in REST API tests to strings ... # Conflicts: # server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java
This new parameter is a boolean parameter that allows
users to put in a compressed model without it having
to be inflated on the master node during the put
request
This is useful for system/module set up and then later
having the model validated and fully parsed when it
is being loaded on a node for usage
closes #77132