[ML] adding new defer_definition_decompression parameter to put trained model API by benwtrent · Pull Request #77189 · elastic/elasticsearch

benwtrent · 2021-09-02T13:21:32Z

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

…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

elasticmachine · 2021-09-02T13:21:35Z

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

elasticmachine · 2021-09-02T13:33:12Z

Pinging @elastic/clients-team (Team:Clients)

…ained-model-improvements

sethmlarson

Some feedback on the API spec:

docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc

rest-api-spec/src/main/resources/rest-api-spec/api/ml.put_trained_model.json

docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc

rest-api-spec/src/main/resources/rest-api-spec/api/ml.put_trained_model.json

sethmlarson

LGTM from a spec perspective!

benwtrent · 2021-09-02T17:27:25Z

run elasticsearch-ci/packaging-tests-windows-sample

benwtrent · 2021-09-02T17:58:59Z

@elasticmachine update branch

lcawl

Added some minor suggestions, otherwise docs LGTM

lcawl · 2021-09-02T20:05:15Z

docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc

+
+`defer_definition_decompression`::
+(Optional, boolean)
+Should the request defer definition decompression and skip relevant


Suggested change

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

lcawl · 2021-09-02T20:05:54Z

docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc

+`defer_definition_decompression`::
+(Optional, boolean)
+Should the request defer definition decompression and skip relevant
+validations when a `compressed_definition` is provided.


If my first suggestion is accepted, this is the second half:

Suggested change

validations when a `compressed_definition` is provided.

validations.

lcawl · 2021-09-02T20:06:37Z

docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc

+(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


Suggested change

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

lcawl · 2021-09-02T20:06:56Z

docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc

+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.


Suggested change

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.

lcawl · 2021-09-02T20:09:18Z

rest-api-spec/src/main/resources/rest-api-spec/api/ml.put_trained_model.json

+      "defer_definition_decompression": {
+        "required": false,
+        "type": "boolean",
+        "description": "Should the action skip decompressing the definition to validate it and set default values",


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.

Suggested change

"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.",

dimitris-athanasiou · 2021-09-03T08:17:30Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/PutTrainedModelAction.java

+                validationException.addValidationError(
+                    "when ["
+                        + DEFER_DEFINITION_DECOMPRESSION
+                        + "] is true and a compressed definition is provided, estimated_heap_memory_usage_bytes must be set"


Suggested change

+ "] 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"

dimitris-athanasiou · 2021-09-03T08:36:26Z

...lugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutTrainedModelAction.java

                    minCompatibilityVersion.toString()));
                return;
            }
+        } else if (state.nodes().getMinNodeVersion().before(state.nodes().getMaxNodeVersion())


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?

@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

dimitris-athanasiou

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

benwtrent added >enhancement :ml Machine learning v8.0.0 v7.16.0 labels Sep 2, 2021

elasticmachine added the Team:ML Meta label for the ML team label Sep 2, 2021

sethmlarson added the Team:Clients Meta label for clients team label Sep 2, 2021

benwtrent added 2 commits September 2, 2021 11:15

fixing ml with security tests;

c6e01f2

Merge remote-tracking branch 'upstream/master' into feature/ml-put-tr…

b1f9f94

…ained-model-improvements

sethmlarson reviewed Sep 2, 2021

View reviewed changes

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

benwtrent commented Sep 2, 2021

View reviewed changes

docs/reference/ml/df-analytics/apis/put-trained-models.asciidoc Outdated Show resolved Hide resolved

benwtrent commented Sep 2, 2021

View reviewed changes

rest-api-spec/src/main/resources/rest-api-spec/api/ml.put_trained_model.json Outdated Show resolved Hide resolved

Apply suggestions from code review

dcc31e4

sethmlarson approved these changes Sep 2, 2021

View reviewed changes

Merge branch 'master' into feature/ml-put-trained-model-improvements

74dadf8

lcawl approved these changes Sep 2, 2021

View reviewed changes

dimitris-athanasiou reviewed Sep 3, 2021

View reviewed changes

benwtrent added 2 commits September 3, 2021 07:28

Merge remote-tracking branch 'upstream/master' into feature/ml-put-tr…

f349ffb

…ained-model-improvements

addressing PR comments

ef2ad96

benwtrent requested a review from dimitris-athanasiou September 3, 2021 11:35

dimitris-athanasiou approved these changes Sep 3, 2021

View reviewed changes

benwtrent merged commit 02e17c3 into elastic:master Sep 3, 2021

benwtrent deleted the feature/ml-put-trained-model-improvements branch September 3, 2021 13:07

jakelandis added v8.0.0-alpha2 and removed v8.0.0 labels Sep 15, 2021

	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

	validations when a `compressed_definition` is provided.
	validations.

	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

	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.

	"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.",

	+ "] 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"

Conversation

benwtrent commented Sep 2, 2021

Uh oh!

elasticmachine commented Sep 2, 2021

Uh oh!

elasticmachine commented Sep 2, 2021

Uh oh!

sethmlarson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sethmlarson left a comment

Choose a reason for hiding this comment

Uh oh!

benwtrent commented Sep 2, 2021

Uh oh!

benwtrent commented Sep 2, 2021

Uh oh!

lcawl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants