[ML][Inference] PUT API #50852

benwtrent · 2020-01-10T12:38:48Z

This adds the PUT API for creating trained models that support our format.

This includes

HLRC change for the API
API creation
Validations of model format and call

elasticmachine · 2020-01-10T12:38:52Z

Pinging @elastic/ml-core (:ml)

benwtrent · 2020-01-10T16:15:13Z

run elasticsearch-ci/docs-check

davidkyle

I wonder if we should version the format of the actual model definition JSON in case we want to change it later. One for discussion not necessary for this change

davidkyle · 2020-01-11T10:24:14Z

client/rest-high-level/src/main/java/org/elasticsearch/client/MachineLearningClient.java

+    }
+
+    /**
+     * Get trained model config asynchronously and notifies listener upon completion


Suggested change

* Get trained model config asynchronously and notifies listener upon completion

* Put trained model config asynchronously and notifiy listener upon completion

davidkyle · 2020-01-11T10:33:10Z

...level/src/main/java/org/elasticsearch/client/ml/inference/InferenceToXContentCompressor.java

+        return deflate(reference);
+    }
+
+    public static <T> T inflate(String compressedString,


I think inflate isn't called anywhere apart from InferenceToXContentCompressorTests. If it goes SimpleBoundedInputStream could also be removed?

@davidkyle possibly...but I was thinking of users requesting the compressed format from elasticsearch (for faster retrieval) and only inflating locally.

davidkyle · 2020-01-11T10:36:54Z

.../rest-high-level/src/main/java/org/elasticsearch/client/ml/inference/TrainedModelConfig.java

@@ -111,7 +112,7 @@ public static TrainedModelConfig fromXContent(XContentParser parser) throws IOEx
        this.modelId = modelId;
        this.createdBy = createdBy;
        this.version = version;
-        this.createTime = Instant.ofEpochMilli(createTime.toEpochMilli());
+        this.createTime = createTime == null ? null : Instant.ofEpochMilli(createTime.toEpochMilli());


createTime can't be null on the server side class. Are we going to set createTime automatically on PUT if not set?

I scrolled down 👍

davidkyle · 2020-01-11T11:02:57Z

.../plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/TrainedModelConfig.java

+                }
+            }
+
+            validationException = checkIllegalSetting(version, VERSION.getPreferredName(), validationException);


validate() can only be called once on PUT as the builder will set these fields so if called on a built config it will error. Maybe add a isCreateTime flag or similar.

PutJobAction has the same issue and does the validation of create time settings in the action

davidkyle · 2020-01-11T11:21:39Z

...multi-node-tests/src/test/java/org/elasticsearch/xpack/ml/integration/InferenceIngestIT.java

        "  \"version\": \"8.0.0\",\n" +
        "  \"license_level\": \"platinum\",\n" +
-        "  \"created_by\": \"benwtrent\",\n" +
+        "  \"created_by\": \"es_test\",\n" +


Suggested change

" \"created_by\": \"es_test\",\n" +

" \"created_by\": \"benwtrent\",\n" +

Take the glory dude

davidkyle · 2020-01-11T11:31:43Z

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

+        TrainedModelConfig trainedModelConfig = new TrainedModelConfig.Builder(request.getTrainedModelConfig())
+            .setVersion(Version.CURRENT)
+            .setCreateTime(Instant.now())
+            .setCreatedBy("user")


I thought created by would be the users name (if security is enabled) but now I realise not. Maybe api rather than user. Or api_user. Our included models are createdBy: ml_admin

api_user seems good to me.

Yeah, I don't think we should include the actual user name for security reasons. It seems like a potential to leak info. Somebody could have permissions to read trained models, but not permission to see all configured users.

Maybe it would be best to match what we did for system created annotations. Otherwise we’re just creating a plethora of imaginary users and eventually someone will ask why they’re all different.

@droberts195 for our internal created annotations, we use XPackUser.NAME which is _xpack. I am fine switching it to that in a subsequent PR.

davidkyle · 2020-01-11T11:37:42Z

...in/ml/src/main/java/org/elasticsearch/xpack/ml/rest/inference/RestPutTrainedModelAction.java

+
+    @Override
+    public String getName() {
+        return "xpack_ml_put_data_frame_analytics_action";


Suggested change

return "xpack_ml_put_data_frame_analytics_action";

return "xpack_ml_put_trained_model_action";

This adds the `PUT` API for creating trained models that support our format. This includes * HLRC change for the API * API creation * Validations of model format and call

* [ML][Inference] PUT API (#50852) This adds the `PUT` API for creating trained models that support our format. This includes * HLRC change for the API * API creation * Validations of model format and call * fixing backport

This adds the `PUT` API for creating trained models that support our format. This includes * HLRC change for the API * API creation * Validations of model format and call

[ML][Inference] PUT API

5b5d212

benwtrent added >non-issue :ml Machine learning v8.0.0 v7.6.0 labels Jan 10, 2020

benwtrent added 4 commits January 10, 2020 08:15

fixing tests

2067386

adding compression logic to hlrc for inference definitions

325ec13

fixing yaml test

48f2e5f

fixing yaml tests more

f04193f

davidkyle approved these changes Jan 11, 2020

View reviewed changes

addressing PR comments

b102d37

benwtrent merged commit 4cecb7a into elastic:master Jan 11, 2020

benwtrent deleted the feature/ml-inference-add-put-model-action branch January 11, 2020 21:03

benwtrent mentioned this pull request Jan 11, 2020

[7.x] [ML][Inference] PUT API (#50852) #50887

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML][Inference] PUT API #50852

[ML][Inference] PUT API #50852

benwtrent commented Jan 10, 2020

elasticmachine commented Jan 10, 2020

benwtrent commented Jan 10, 2020

davidkyle left a comment

davidkyle Jan 11, 2020

davidkyle Jan 11, 2020

benwtrent Jan 11, 2020

davidkyle Jan 11, 2020

davidkyle Jan 11, 2020

davidkyle Jan 11, 2020

davidkyle Jan 11, 2020

benwtrent Jan 11, 2020

davidkyle Jan 11, 2020

benwtrent Jan 11, 2020

droberts195 Jan 11, 2020

benwtrent Jan 11, 2020

davidkyle Jan 11, 2020

	* Get trained model config asynchronously and notifies listener upon completion
	* Put trained model config asynchronously and notifiy listener upon completion

	" \"created_by\": \"es_test\",\n" +
	" \"created_by\": \"benwtrent\",\n" +

	return "xpack_ml_put_data_frame_analytics_action";
	return "xpack_ml_put_trained_model_action";

[ML][Inference] PUT API #50852

[ML][Inference] PUT API #50852

Conversation

benwtrent commented Jan 10, 2020

elasticmachine commented Jan 10, 2020

benwtrent commented Jan 10, 2020

davidkyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment