[ML][Inference] Adding read/del trained models by benwtrent · Pull Request #47882 · elastic/elasticsearch

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

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants