[ML][Inference] Adding read/del trained models #47882

benwtrent · 2019-10-10T20:17:38Z

Adds two endpoints for DELETE and GET trained models.

We don't allow DELETE for models that are referenced by a ingest pipeline.

elasticmachine · 2019-10-10T20:17:39Z

Pinging @elastic/ml-core (:ml)

…ead-del

przemekwitek · 2019-10-16T07:50:28Z

.../core/src/main/java/org/elasticsearch/xpack/core/ml/notifications/InferenceAuditMessage.java

+import java.util.Date;
+
+
+public class InferenceAuditMessage extends AbstractAuditMessage {


Could you add a unit test, similar to AnomalyDetectionAuditMessageTests?

I will, but it seems like overkill to me as this class essentially does nothing.

przemekwitek · 2019-10-16T07:54:00Z

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

+            super(in);
+        }
+
+        public Response(QueryPage<TrainedModelConfig> analytics) {


Rename analytics to trainedModels or similar?

przemekwitek · 2019-10-16T07:56:55Z

.../plugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/ingest/InferenceProcessor.java

+        private final Client client;
+        private final ClusterService clusterService;
+
+        public Factory(Client client, ClusterService clusterService, Settings settings) {


settings parameter is unused, is it ok to remove it?

@przemekwitek this class is unused, I prefer to just keep it like this in waiting for #47859. If #47859 is merged first, then I can fix the conflicts here. If this PR is merged first, then that PR will have to address conflicts.

przemekwitek · 2019-10-16T07:57:39Z

.../plugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/ingest/InferenceProcessor.java

+    public static class Factory implements Processor.Factory, Consumer<ClusterState> {
+
+        private final Client client;
+        private final ClusterService clusterService;


How is clusterService used?

It will be, neither is client really. This class is just sitting here waiting on: #47859

.../ml/src/main/java/org/elasticsearch/xpack/ml/inference/persistence/TrainedModelProvider.java

przemekwitek · 2019-10-16T08:01:35Z

...ugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportGetTrainedModelsAction.java

+
+    @Override
+    protected String[] getIndices() {
+        return new String[] {InferenceIndexConstants.INDEX_PATTERN };


Suggested change

return new String[] {InferenceIndexConstants.INDEX_PATTERN };

return new String[] { InferenceIndexConstants.INDEX_PATTERN };

przemekwitek · 2019-10-16T08:02:54Z

...in/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteTrainedModelAction.java

+
+/**
+ * The action is a master node action to ensure it reads an up-to-date cluster
+ * state in order to determine whether there is a persistent task for the analytics


analytics? Or trained model?

przemekwitek · 2019-10-16T08:03:43Z

...in/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteTrainedModelAction.java

+        Set<String> referencedModels = getReferencedModelKeys(currentIngestMetadata);
+
+        if (referencedModels.contains(id)) {
+            listener.onFailure(new ElasticsearchStatusException("Cannot delete mode [{}] as it is still referenced by ingest processors",


Suggested change

listener.onFailure(new ElasticsearchStatusException("Cannot delete mode [{}] as it is still referenced by ingest processors",

listener.onFailure(new ElasticsearchStatusException("Cannot delete model [{}] as it is still referenced by ingest processors",

przemekwitek · 2019-10-16T08:06:07Z

...in/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportDeleteTrainedModelAction.java

+
+    private Set<String> getReferencedModelKeys(IngestMetadata ingestMetadata) {
+        Set<String> allReferencedModelKeys = new HashSet<>();
+        if (ingestMetadata != null) {


This is a matter of taste, but I would add this in the beginning of this method:

if (ingestMetadata == null) { return Collections.emptySet(); }

This way it is clear what happens on null case and the rest of the method focuses on non-null case.

przemekwitek · 2019-10-16T08:08:55Z

x-pack/plugin/src/test/resources/rest-api-spec/test/ml/inference_crud.yml

+  - match: { count: 0 }
+  - match: { trained_model_configs: [] }
+
+  - do:


Duplicate stanza?

…ead-del

przemekwitek

LGTM

.../ml/src/main/java/org/elasticsearch/xpack/ml/inference/persistence/TrainedModelProvider.java

* [ML][Inference] adds lazy model loader and inference (#47410) This adds a couple of things: - A model loader service that is accessible via transport calls. This service will load in models and cache them. They will stay loaded until a processor no longer references them - A Model class and its first sub-class LocalModel. Used to cache model information and run inference. - Transport action and handler for requests to infer against a local model Related Feature PRs: * [ML][Inference] Adjust inference configuration option API (#47812) * [ML][Inference] adds logistic_regression output aggregator (#48075) * [ML][Inference] Adding read/del trained models (#47882) * [ML][Inference] Adding inference ingest processor (#47859) * [ML][Inference] fixing classification inference for ensemble (#48463) * [ML][Inference] Adding model memory estimations (#48323) * [ML][Inference] adding more options to inference processor (#48545) * [ML][Inference] handle string values better in feature extraction (#48584) * [ML][Inference] Adding _stats endpoint for inference (#48492) * [ML][Inference] add inference processors and trained models to usage (#47869) * [ML][Inference] add new flag for optionally including model definition (#48718) * [ML][Inference] adding license checks (#49056) * [ML][Inference] Adding memory and compute estimates to inference (#48955)

* [ML][Inference] adds lazy model loader and inference (elastic#47410) This adds a couple of things: - A model loader service that is accessible via transport calls. This service will load in models and cache them. They will stay loaded until a processor no longer references them - A Model class and its first sub-class LocalModel. Used to cache model information and run inference. - Transport action and handler for requests to infer against a local model Related Feature PRs: * [ML][Inference] Adjust inference configuration option API (elastic#47812) * [ML][Inference] adds logistic_regression output aggregator (elastic#48075) * [ML][Inference] Adding read/del trained models (elastic#47882) * [ML][Inference] Adding inference ingest processor (elastic#47859) * [ML][Inference] fixing classification inference for ensemble (elastic#48463) * [ML][Inference] Adding model memory estimations (elastic#48323) * [ML][Inference] adding more options to inference processor (elastic#48545) * [ML][Inference] handle string values better in feature extraction (elastic#48584) * [ML][Inference] Adding _stats endpoint for inference (elastic#48492) * [ML][Inference] add inference processors and trained models to usage (elastic#47869) * [ML][Inference] add new flag for optionally including model definition (elastic#48718) * [ML][Inference] adding license checks (elastic#49056) * [ML][Inference] Adding memory and compute estimates to inference (elastic#48955)

* [ML] ML Model Inference Ingest Processor (#49052) * [ML][Inference] adds lazy model loader and inference (#47410) This adds a couple of things: - A model loader service that is accessible via transport calls. This service will load in models and cache them. They will stay loaded until a processor no longer references them - A Model class and its first sub-class LocalModel. Used to cache model information and run inference. - Transport action and handler for requests to infer against a local model Related Feature PRs: * [ML][Inference] Adjust inference configuration option API (#47812) * [ML][Inference] adds logistic_regression output aggregator (#48075) * [ML][Inference] Adding read/del trained models (#47882) * [ML][Inference] Adding inference ingest processor (#47859) * [ML][Inference] fixing classification inference for ensemble (#48463) * [ML][Inference] Adding model memory estimations (#48323) * [ML][Inference] adding more options to inference processor (#48545) * [ML][Inference] handle string values better in feature extraction (#48584) * [ML][Inference] Adding _stats endpoint for inference (#48492) * [ML][Inference] add inference processors and trained models to usage (#47869) * [ML][Inference] add new flag for optionally including model definition (#48718) * [ML][Inference] adding license checks (#49056) * [ML][Inference] Adding memory and compute estimates to inference (#48955) * fixing version of indexed docs for model inference

[ML][Inference] Adding read/del trained models

f3a8453

benwtrent added >non-issue :ml Machine learning v8.0.0 labels Oct 10, 2019

Merge branch 'feature/ml-inference' into feature/ml-inference-model-r…

829f3c6

…ead-del

przemekwitek reviewed Oct 16, 2019

View reviewed changes

droberts195 added the v7.6.0 label Oct 16, 2019

benwtrent added 2 commits October 16, 2019 09:24

addressing PR comments and fixing tests

c34977f

adding error tests to ml_security blacklist

ee758ee

benwtrent requested a review from przemekwitek October 16, 2019 18:47

benwtrent added 2 commits October 16, 2019 14:47

Merge branch 'feature/ml-inference' into feature/ml-inference-model-r…

c900846

…ead-del

fixing tests

841da05

przemekwitek approved these changes Oct 17, 2019

View reviewed changes

.../ml/src/main/java/org/elasticsearch/xpack/ml/inference/persistence/TrainedModelProvider.java Show resolved Hide resolved

benwtrent merged commit 27d14a0 into elastic:feature/ml-inference Oct 18, 2019

benwtrent mentioned this pull request Nov 18, 2019

[7.x][ML] ML Model Inference Ingest Processor (#49052) #49257

Merged

benwtrent deleted the feature/ml-inference-model-read-del branch November 18, 2019 17:12

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

Uh oh!

[ML][Inference] Adding read/del trained models #47882

[ML][Inference] Adding read/del trained models #47882

Uh oh!

benwtrent commented Oct 10, 2019

Uh oh!

elasticmachine commented Oct 10, 2019

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

benwtrent Oct 16, 2019

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

benwtrent Oct 16, 2019

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

benwtrent Oct 16, 2019

Uh oh!

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

przemekwitek Oct 16, 2019

Uh oh!

przemekwitek left a comment

Uh oh!

Uh oh!

Uh oh!

		import java.util.Date;


		public class InferenceAuditMessage extends AbstractAuditMessage {

	return new String[] {InferenceIndexConstants.INDEX_PATTERN };
	return new String[] { InferenceIndexConstants.INDEX_PATTERN };

	listener.onFailure(new ElasticsearchStatusException("Cannot delete mode [{}] as it is still referenced by ingest processors",
	listener.onFailure(new ElasticsearchStatusException("Cannot delete model [{}] as it is still referenced by ingest processors",

[ML][Inference] Adding read/del trained models #47882

[ML][Inference] Adding read/del trained models #47882

Uh oh!

Conversation

benwtrent commented Oct 10, 2019

Uh oh!

elasticmachine commented Oct 10, 2019

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!

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!

przemekwitek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!