From 4be366396840b9d819801a234c36caf801419bf1 Mon Sep 17 00:00:00 2001
From: Benjamin Trent <ben.w.trent@gmail.com>
Date: Fri, 17 Apr 2020 14:45:02 -0400
Subject: [PATCH] [7.x] [ML] fix bugs with prediction field value settings
 (#55333) (#55394)

* [ML] fix bugs with prediction field value settings (#55333)

This fixes two unreleased bugs:

1. Prediction value type of `number` might show unexpected classes

Analytics created models may have class labels like `1, 5, 10` (or some collection of discrete, whole numbers). These labels are passed to the inference model config in the `classification_labels` field.

When the predicted value format is `numeric` it should attempt to see if the classification labels are provided and are numeric. If so, use those. If not, use the underlying value.

2. When supplying an update overwrite, inference was losing the default prediction field value. This is because it was not copied over in the copy ctor in the ClassificationConfig.Builder class.

closes #55332
---
 .../ml/action/InternalInferModelAction.java   | 10 +++++++
 .../trainedmodel/ClassificationConfig.java    |  1 +
 .../ClassificationConfigUpdate.java           |  8 +++--
 .../trainedmodel/PredictionFieldType.java     | 14 +++++++++
 .../InternalInferModelActionRequestTests.java |  4 +--
 .../ClassificationConfigTests.java            |  3 ++
 .../ClassificationConfigUpdateTests.java      | 29 +++++++++++++++++--
 .../PredictionFieldTypeTests.java             |  3 ++
 .../RegressionConfigUpdateTests.java          | 24 +++++++++++++--
 9 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/InternalInferModelAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/InternalInferModelAction.java
index abb6cfb06b1..4233110abfe 100644
--- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/InternalInferModelAction.java
+++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/InternalInferModelAction.java
@@ -10,6 +10,7 @@ import org.elasticsearch.action.ActionRequest;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.action.ActionType;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.xpack.core.ml.inference.TrainedModelConfig;
@@ -137,6 +138,15 @@ public class InternalInferModelAction extends ActionType<InternalInferModelActio
             return Objects.hash(modelId, objectsToInfer, update, previouslyLicensed);
         }
 
+        @Override
+        public String toString() {
+            return "Request{" +
+                "modelId='" + modelId + '\'' +
+                ", objectsToInfer=" + objectsToInfer +
+                ", update=" + Strings.toString(update) +
+                ", previouslyLicensed=" + previouslyLicensed +
+                '}';
+        }
     }
 
     public static class Response extends ActionResponse {
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfig.java
index b25e78677b3..09953818919 100644
--- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfig.java
+++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfig.java
@@ -214,6 +214,7 @@ public class ClassificationConfig implements LenientlyParsedInferenceConfig, Str
             this.topClassesResultsField = config.topClassesResultsField;
             this.resultsField = config.resultsField;
             this.numTopFeatureImportanceValues = config.numTopFeatureImportanceValues;
+            this.predictionFieldType = config.predictionFieldType;
         }
 
         public Builder setNumTopClasses(Integer numTopClasses) {
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigUpdate.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigUpdate.java
index bb12c04ff5b..290bbd01c49 100644
--- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigUpdate.java
+++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigUpdate.java
@@ -262,11 +262,15 @@ public class ClassificationConfigUpdate implements InferenceConfigUpdate {
             return this;
         }
 
-        private Builder setPredictionFieldType(String predictionFieldType) {
-            this.predictionFieldType = PredictionFieldType.fromString(predictionFieldType);
+        public Builder setPredictionFieldType(PredictionFieldType predictionFieldtype) {
+            this.predictionFieldType = predictionFieldtype;
             return this;
         }
 
+        private Builder setPredictionFieldType(String predictionFieldType) {
+            return setPredictionFieldType(PredictionFieldType.fromString(predictionFieldType));
+        }
+
         public ClassificationConfigUpdate build() {
             return new ClassificationConfigUpdate(numTopClasses,
                 resultsField,
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/PredictionFieldType.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/PredictionFieldType.java
index 88dfd211403..f0a8f08aa00 100644
--- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/PredictionFieldType.java
+++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/PredictionFieldType.java
@@ -6,6 +6,7 @@
 
 package org.elasticsearch.xpack.core.ml.inference.trainedmodel;
 
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.common.io.stream.Writeable;
@@ -57,6 +58,19 @@ public enum PredictionFieldType implements Writeable {
                 }
                 return areClose(value, 1.0D);
             case NUMBER:
+                if (Strings.isNullOrEmpty(stringRep)) {
+                    return value;
+                }
+                // Quick check to verify that the string rep is LIKELY a number
+                // Still handles the case where it throws and then returns the underlying value
+                if (stringRep.charAt(0) == '-' || Character.isDigit(stringRep.charAt(0))) {
+                    try {
+                        return Long.parseLong(stringRep);
+                    } catch (NumberFormatException nfe) {
+                        return value;
+                    }
+                }
+                return value;
             default:
                 return value;
         }
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/InternalInferModelActionRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/InternalInferModelActionRequestTests.java
index 80693c94be9..5b736307c01 100644
--- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/InternalInferModelActionRequestTests.java
+++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/action/InternalInferModelActionRequestTests.java
@@ -48,8 +48,8 @@ public class InternalInferModelActionRequestTests extends AbstractBWCWireSeriali
     }
 
     private static InferenceConfigUpdate randomInferenceConfigUpdate() {
-        return randomFrom(RegressionConfigUpdateTests.randomRegressionConfig(),
-            ClassificationConfigUpdateTests.randomClassificationConfig());
+        return randomFrom(RegressionConfigUpdateTests.randomRegressionConfigUpdate(),
+            ClassificationConfigUpdateTests.randomClassificationConfigUpdate());
     }
 
     private static Map<String, Object> randomMap() {
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigTests.java
index 8ffd1d7ff85..71658d74a12 100644
--- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigTests.java
+++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigTests.java
@@ -29,6 +29,9 @@ public class ClassificationConfigTests extends AbstractBWCSerializationTestCase<
 
     public static ClassificationConfig mutateForVersion(ClassificationConfig instance, Version version) {
         ClassificationConfig.Builder builder = new ClassificationConfig.Builder(instance);
+        if (version.before(Version.V_7_8_0)) {
+            builder.setPredictionFieldType(PredictionFieldType.STRING);
+        }
         if (version.before(Version.V_7_7_0)) {
             builder.setNumTopFeatureImportanceValues(0);
         }
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigUpdateTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigUpdateTests.java
index ff8bbe13251..c0c43f210e4 100644
--- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigUpdateTests.java
+++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/ClassificationConfigUpdateTests.java
@@ -16,11 +16,12 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
+import static org.elasticsearch.xpack.core.ml.inference.trainedmodel.ClassificationConfigTests.randomClassificationConfig;
 import static org.hamcrest.Matchers.equalTo;
 
 public class ClassificationConfigUpdateTests extends AbstractBWCSerializationTestCase<ClassificationConfigUpdate> {
 
-    public static ClassificationConfigUpdate randomClassificationConfig() {
+    public static ClassificationConfigUpdate randomClassificationConfigUpdate() {
         return new ClassificationConfigUpdate(randomBoolean() ? null : randomIntBetween(-1, 10),
             randomBoolean() ? null : randomAlphaOfLength(10),
             randomBoolean() ? null : randomAlphaOfLength(10),
@@ -49,9 +50,33 @@ public class ClassificationConfigUpdateTests extends AbstractBWCSerializationTes
         assertThat(ex.getMessage(), equalTo("Unrecognized fields [some_key]."));
     }
 
+    public void testApply() {
+        ClassificationConfig originalConfig = randomClassificationConfig();
+
+        assertThat(originalConfig, equalTo(ClassificationConfigUpdate.EMPTY_PARAMS.apply(originalConfig)));
+
+        assertThat(new ClassificationConfig.Builder(originalConfig).setNumTopClasses(5).build(),
+            equalTo(new ClassificationConfigUpdate.Builder().setNumTopClasses(5).build().apply(originalConfig)));
+        assertThat(new ClassificationConfig.Builder()
+            .setNumTopClasses(5)
+            .setNumTopFeatureImportanceValues(1)
+            .setPredictionFieldType(PredictionFieldType.BOOLEAN)
+            .setResultsField("foo")
+            .setTopClassesResultsField("bar").build(),
+            equalTo(new ClassificationConfigUpdate.Builder()
+                .setNumTopClasses(5)
+                .setNumTopFeatureImportanceValues(1)
+                .setPredictionFieldType(PredictionFieldType.BOOLEAN)
+                .setResultsField("foo")
+                .setTopClassesResultsField("bar")
+                .build()
+                .apply(originalConfig)
+            ));
+    }
+
     @Override
     protected ClassificationConfigUpdate createTestInstance() {
-        return randomClassificationConfig();
+        return randomClassificationConfigUpdate();
     }
 
     @Override
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/PredictionFieldTypeTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/PredictionFieldTypeTests.java
index ff8dd83cde2..0901ee62e74 100644
--- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/PredictionFieldTypeTests.java
+++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/PredictionFieldTypeTests.java
@@ -38,6 +38,9 @@ public class PredictionFieldTypeTests extends ESTestCase {
             is(nullValue()));
         assertThat(PredictionFieldType.NUMBER.transformPredictedValue(1.0, "foo"), equalTo(1.0));
         assertThat(PredictionFieldType.NUMBER.transformPredictedValue(1.0, null), equalTo(1.0));
+        assertThat(PredictionFieldType.NUMBER.transformPredictedValue(1.0, ""), equalTo(1.0));
+        long expected = randomLong();
+        assertThat(PredictionFieldType.NUMBER.transformPredictedValue(1.0, String.valueOf(expected)), equalTo(expected));
     }
 
 }
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/RegressionConfigUpdateTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/RegressionConfigUpdateTests.java
index 0d4212de9bd..fa9d33c617e 100644
--- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/RegressionConfigUpdateTests.java
+++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/trainedmodel/RegressionConfigUpdateTests.java
@@ -16,11 +16,12 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
+import static org.elasticsearch.xpack.core.ml.inference.trainedmodel.RegressionConfigTests.randomRegressionConfig;
 import static org.hamcrest.Matchers.equalTo;
 
 public class RegressionConfigUpdateTests extends AbstractBWCSerializationTestCase<RegressionConfigUpdate> {
 
-    public static RegressionConfigUpdate randomRegressionConfig() {
+    public static RegressionConfigUpdate randomRegressionConfigUpdate() {
         return new RegressionConfigUpdate(randomBoolean() ? null : randomAlphaOfLength(10),
             randomBoolean() ? null : randomIntBetween(0, 10));
     }
@@ -40,9 +41,28 @@ public class RegressionConfigUpdateTests extends AbstractBWCSerializationTestCas
         assertThat(ex.getMessage(), equalTo("Unrecognized fields [some_key]."));
     }
 
+    public void testApply() {
+        RegressionConfig originalConfig = randomRegressionConfig();
+
+        assertThat(originalConfig, equalTo(RegressionConfigUpdate.EMPTY_PARAMS.apply(originalConfig)));
+
+        assertThat(new RegressionConfig.Builder(originalConfig).setNumTopFeatureImportanceValues(5).build(),
+            equalTo(new RegressionConfigUpdate.Builder().setNumTopFeatureImportanceValues(5).build().apply(originalConfig)));
+        assertThat(new RegressionConfig.Builder()
+                .setNumTopFeatureImportanceValues(1)
+                .setResultsField("foo")
+                .build(),
+            equalTo(new RegressionConfigUpdate.Builder()
+                .setNumTopFeatureImportanceValues(1)
+                .setResultsField("foo")
+                .build()
+                .apply(originalConfig)
+            ));
+    }
+
     @Override
     protected RegressionConfigUpdate createTestInstance() {
-        return randomRegressionConfig();
+        return randomRegressionConfigUpdate();
     }
 
     @Override