From 6c74c524b1e29fba742e322d219e8da24bd0ffa8 Mon Sep 17 00:00:00 2001 From: Andrey Velichkevich Date: Tue, 30 Jun 2020 13:38:06 +0100 Subject: [PATCH] String type for metric values (#1245) * Change metric type from float to string * Check unavailable latest objective metric in isTrialObservationAvailable * Fix e2e test * Delete consts.UnavailableMetricValue from e2e --- .../controller/common/v1beta1/common_types.go | 8 +- pkg/apis/v1beta1/openapi_generated.go | 171 +++++++++++++----- pkg/apis/v1beta1/swagger.json | 94 +++++++--- pkg/controller.v1beta1/consts/const.go | 3 + .../experiment/util/status_util.go | 34 ++-- .../suggestionclient/suggestionclient.go | 13 +- .../suggestionclient/suggestionclient_test.go | 97 ++++++++-- .../trial/trial_controller_test.go | 8 +- .../trial/trial_controller_util.go | 27 ++- test/e2e/v1beta1/resume-e2e-experiment.go | 10 +- test/e2e/v1beta1/run-e2e-experiment.go | 9 +- 11 files changed, 337 insertions(+), 137 deletions(-) diff --git a/pkg/apis/controller/common/v1beta1/common_types.go b/pkg/apis/controller/common/v1beta1/common_types.go index 73f1ccc05a0..642ac4dcf16 100644 --- a/pkg/apis/controller/common/v1beta1/common_types.go +++ b/pkg/apis/controller/common/v1beta1/common_types.go @@ -83,10 +83,10 @@ type MetricStrategy struct { } type Metric struct { - Name string `json:"name,omitempty"` - Min float64 `json:"min,omitempty"` - Max float64 `json:"max,omitempty"` - Latest string `json:"latest,omitempty"` + Name string `json:"name,omitempty"` + Min string `json:"min,omitempty"` + Max string `json:"max,omitempty"` + Latest string `json:"latest,omitempty"` } // +k8s:deepcopy-gen=true diff --git a/pkg/apis/v1beta1/openapi_generated.go b/pkg/apis/v1beta1/openapi_generated.go index 184ddc99c98..2785f0ba00e 100644 --- a/pkg/apis/v1beta1/openapi_generated.go +++ b/pkg/apis/v1beta1/openapi_generated.go @@ -212,8 +212,8 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, "min": { SchemaProps: spec.SchemaProps{ - Type: []string{"number"}, - Format: "double", + Type: []string{"string"}, + Format: "", }, }, "max": { @@ -233,6 +233,27 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, Dependencies: []string{}, }, + "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.MetricStrategy": { + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Properties: map[string]spec.Schema{ + "name": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + "value": { + SchemaProps: spec.SchemaProps{ + Type: []string{"string"}, + Format: "", + }, + }, + }, + }, + }, + Dependencies: []string{}, + }, "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.MetricsCollectorSpec": { Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ @@ -292,12 +313,11 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "metricStrategies": { SchemaProps: spec.SchemaProps{ Description: "This field is allowed to missing, experiment defaulter (webhook) will fill it.", - Type: []string{"object"}, - AdditionalProperties: &spec.SchemaOrBool{ + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ Schema: &spec.Schema{ SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", + Ref: ref("github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.MetricStrategy"), }, }, }, @@ -306,7 +326,8 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, }, }, - Dependencies: []string{}, + Dependencies: []string{ + "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.MetricStrategy"}, }, "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.Observation": { Schema: spec.Schema{ @@ -381,6 +402,37 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Dependencies: []string{ "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.FileSystemPath", "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.FilterSpec", "k8s.io/api/core/v1.HTTPGetAction"}, }, + "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.ConfigMapSource": { + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Description: "ConfigMapSource references the config map where Trial template is located", + Properties: map[string]spec.Schema{ + "configMapName": { + SchemaProps: spec.SchemaProps{ + Description: "Name of config map where Trial template is located", + Type: []string{"string"}, + Format: "", + }, + }, + "configMapNamespace": { + SchemaProps: spec.SchemaProps{ + Description: "Namespace of config map where Trial template is located", + Type: []string{"string"}, + Format: "", + }, + }, + "templatePath": { + SchemaProps: spec.SchemaProps{ + Description: "Path in config map where Trial template is located", + Type: []string{"string"}, + Format: "", + }, + }, + }, + }, + }, + Dependencies: []string{}, + }, "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.Experiment": { Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ @@ -795,27 +847,6 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, Dependencies: []string{}, }, - "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.GoTemplate": { - Schema: spec.Schema{ - SchemaProps: spec.SchemaProps{ - Properties: map[string]spec.Schema{ - "templateSpec": { - SchemaProps: spec.SchemaProps{ - Ref: ref("github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.TemplateSpec"), - }, - }, - "rawTemplate": { - SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", - }, - }, - }, - }, - }, - Dependencies: []string{ - "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.TemplateSpec"}, - }, "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.GraphConfig": { Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ @@ -980,26 +1011,30 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA Dependencies: []string{ "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.FeasibleSpace"}, }, - "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.TemplateSpec": { + "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.TrialParameterSpec": { Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ + Description: "TrialParameterSpec describes parameters that must be replaced in Trial template", Properties: map[string]spec.Schema{ - "configMapName": { + "name": { SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", + Description: "Name of the parameter that must be replaced in Trial template", + Type: []string{"string"}, + Format: "", }, }, - "configMapNamespace": { + "description": { SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", + Description: "Description of the parameter", + Type: []string{"string"}, + Format: "", }, }, - "templatePath": { + "reference": { SchemaProps: spec.SchemaProps{ - Type: []string{"string"}, - Format: "", + Description: "Reference to the parameter in search space", + Type: []string{"string"}, + Format: "", }, }, }, @@ -1007,26 +1042,71 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, Dependencies: []string{}, }, + "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.TrialSource": { + Schema: spec.Schema{ + SchemaProps: spec.SchemaProps{ + Description: "TrialSource represent the source for Trial template Only one source can be specified", + Properties: map[string]spec.Schema{ + "trialSpec": { + SchemaProps: spec.SchemaProps{ + Description: "TrialSpec represents Trial template in unstructured format", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.Unstructured"), + }, + }, + "configMap": { + SchemaProps: spec.SchemaProps{ + Description: "ConfigMap spec represents a reference to ConfigMap", + Ref: ref("github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.ConfigMapSource"), + }, + }, + }, + }, + }, + Dependencies: []string{ + "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.ConfigMapSource", "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.Unstructured"}, + }, "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.TrialTemplate": { Schema: spec.Schema{ SchemaProps: spec.SchemaProps{ + Description: "TrialTemplate describes structure of Trial template", Properties: map[string]spec.Schema{ "retain": { SchemaProps: spec.SchemaProps{ - Type: []string{"boolean"}, - Format: "", + Description: "Retain indicates that Trial resources must be not cleanup", + Type: []string{"boolean"}, + Format: "", }, }, - "goTemplate": { + "trialSpec": { SchemaProps: spec.SchemaProps{ - Ref: ref("github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.GoTemplate"), + Description: "TrialSpec represents Trial template in unstructured format", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.Unstructured"), + }, + }, + "configMap": { + SchemaProps: spec.SchemaProps{ + Description: "ConfigMap spec represents a reference to ConfigMap", + Ref: ref("github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.ConfigMapSource"), + }, + }, + "trialParameters": { + SchemaProps: spec.SchemaProps{ + Description: "List of parameres that are used in Trial template", + Type: []string{"array"}, + Items: &spec.SchemaOrArray{ + Schema: &spec.Schema{ + SchemaProps: spec.SchemaProps{ + Ref: ref("github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.TrialParameterSpec"), + }, + }, + }, }, }, }, }, }, Dependencies: []string{ - "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.GoTemplate"}, + "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.ConfigMapSource", "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1.TrialParameterSpec", "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.Unstructured"}, }, "github.com/kubeflow/katib/pkg/apis/controller/suggestions/v1beta1.Suggestion": { Schema: spec.Schema{ @@ -1454,8 +1534,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "runSpec": { SchemaProps: spec.SchemaProps{ Description: "Raw text for the trial run spec. This can be any generic Kubernetes runtime object. The trial operator should create the resource as written, and let the corresponding resource controller (e.g. tf-operator) handle the rest.", - Type: []string{"string"}, - Format: "", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.Unstructured"), }, }, "retainRun": { @@ -1476,7 +1555,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA }, }, Dependencies: []string{ - "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.MetricsCollectorSpec", "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.ObjectiveSpec", "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.ParameterAssignment"}, + "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.MetricsCollectorSpec", "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.ObjectiveSpec", "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1.ParameterAssignment", "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured.Unstructured"}, }, "github.com/kubeflow/katib/pkg/apis/controller/trials/v1beta1.TrialStatus": { Schema: spec.Schema{ diff --git a/pkg/apis/v1beta1/swagger.json b/pkg/apis/v1beta1/swagger.json index b6e6c12104c..d7effab3ff2 100644 --- a/pkg/apis/v1beta1/swagger.json +++ b/pkg/apis/v1beta1/swagger.json @@ -268,7 +268,7 @@ }, "runSpec": { "description": "Raw text for the trial run spec. This can be any generic Kubernetes runtime object. The trial operator should create the resource as written, and let the corresponding resource controller (e.g. tf-operator) handle the rest.", - "type": "string" + "$ref": "#/definitions/v1.unstructured.Unstructured" } } }, @@ -340,6 +340,23 @@ } } }, + "v1beta1.ConfigMapSource": { + "description": "ConfigMapSource references the config map where Trial template is located", + "properties": { + "configMapName": { + "description": "Name of config map where Trial template is located", + "type": "string" + }, + "configMapNamespace": { + "description": "Namespace of config map where Trial template is located", + "type": "string" + }, + "templatePath": { + "description": "Path in config map where Trial template is located", + "type": "string" + } + } + }, "v1beta1.EarlyStoppingSetting": { "properties": { "name": { @@ -627,16 +644,6 @@ } } }, - "v1beta1.GoTemplate": { - "properties": { - "rawTemplate": { - "type": "string" - }, - "templateSpec": { - "$ref": "#/definitions/v1beta1.TemplateSpec" - } - } - }, "v1beta1.GraphConfig": { "description": "GraphConfig contains a config of DAG", "properties": { @@ -666,18 +673,26 @@ "type": "string" }, "max": { - "type": "number", - "format": "double" + "type": "string" }, "min": { - "type": "number", - "format": "double" + "type": "string" }, "name": { "type": "string" } } }, + "v1beta1.MetricStrategy": { + "properties": { + "name": { + "type": "string" + }, + "value": { + "type": "string" + } + } + }, "v1beta1.MetricsCollectorSpec": { "properties": { "collector": { @@ -717,9 +732,9 @@ }, "metricStrategies": { "description": "This field is allowed to missing, experiment defaulter (webhook) will fill it.", - "type": "object", - "additionalProperties": { - "type": "string" + "type": "array", + "items": { + "$ref": "#/definitions/v1beta1.MetricStrategy" } }, "objectiveMetricName": { @@ -821,26 +836,57 @@ } } }, - "v1beta1.TemplateSpec": { + "v1beta1.TrialParameterSpec": { + "description": "TrialParameterSpec describes parameters that must be replaced in Trial template", "properties": { - "configMapName": { + "description": { + "description": "Description of the parameter", "type": "string" }, - "configMapNamespace": { + "name": { + "description": "Name of the parameter that must be replaced in Trial template", "type": "string" }, - "templatePath": { + "reference": { + "description": "Reference to the parameter in search space", "type": "string" } } }, + "v1beta1.TrialSource": { + "description": "TrialSource represent the source for Trial template Only one source can be specified", + "properties": { + "configMap": { + "description": "ConfigMap spec represents a reference to ConfigMap", + "$ref": "#/definitions/v1beta1.ConfigMapSource" + }, + "trialSpec": { + "description": "TrialSpec represents Trial template in unstructured format", + "$ref": "#/definitions/v1.unstructured.Unstructured" + } + } + }, "v1beta1.TrialTemplate": { + "description": "TrialTemplate describes structure of Trial template", "properties": { - "goTemplate": { - "$ref": "#/definitions/v1beta1.GoTemplate" + "configMap": { + "description": "ConfigMap spec represents a reference to ConfigMap", + "$ref": "#/definitions/v1beta1.ConfigMapSource" }, "retain": { + "description": "Retain indicates that Trial resources must be not cleanup", "type": "boolean" + }, + "trialParameters": { + "description": "List of parameres that are used in Trial template", + "type": "array", + "items": { + "$ref": "#/definitions/v1beta1.TrialParameterSpec" + } + }, + "trialSpec": { + "description": "TrialSpec represents Trial template in unstructured format", + "$ref": "#/definitions/v1.unstructured.Unstructured" } } } diff --git a/pkg/controller.v1beta1/consts/const.go b/pkg/controller.v1beta1/consts/const.go index 67acf1c1921..b90d7795435 100644 --- a/pkg/controller.v1beta1/consts/const.go +++ b/pkg/controller.v1beta1/consts/const.go @@ -135,6 +135,9 @@ const ( // TrialTemplateReplaceFormatRegex is the regex for Trial template format TrialTemplateReplaceFormatRegex = "\\{trialParameters\\..+?\\}" + + // UnavailableMetricValue is the value when metric was not reported or metric value can't be converted to float64 + UnavailableMetricValue = "unavailable" ) var ( diff --git a/pkg/controller.v1beta1/experiment/util/status_util.go b/pkg/controller.v1beta1/experiment/util/status_util.go index f0beebe735e..57f5b6dff70 100644 --- a/pkg/controller.v1beta1/experiment/util/status_util.go +++ b/pkg/controller.v1beta1/experiment/util/status_util.go @@ -16,14 +16,14 @@ limitations under the License. package util import ( - "fmt" - "math" - logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" "strconv" + logf "sigs.k8s.io/controller-runtime/pkg/runtime/log" + commonv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1" experimentsv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1" trialsv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/trials/v1beta1" + "github.com/kubeflow/katib/pkg/controller.v1beta1/consts" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -80,11 +80,11 @@ func updateTrialsSummary(instance *experimentsv1beta1.Experiment, trials *trials } objectiveMetricValueStr := getObjectiveMetricValue(trial) - if objectiveMetricValueStr == nil { + if objectiveMetricValueStr == consts.UnavailableMetricValue { continue } - objectiveMetricValue, err := strconv.ParseFloat(*objectiveMetricValueStr, 64) + objectiveMetricValue, err := strconv.ParseFloat(objectiveMetricValueStr, 64) // For string metrics values best trial is the latest if err != nil { bestTrialIndex = index @@ -140,9 +140,9 @@ func updateTrialsSummary(instance *experimentsv1beta1.Experiment, trials *trials return isObjectiveGoalReached } -func getObjectiveMetricValue(trial trialsv1beta1.Trial) *string { +func getObjectiveMetricValue(trial trialsv1beta1.Trial) string { if trial.Status.Observation == nil { - return nil + return consts.UnavailableMetricValue } var objectiveStrategy commonv1beta1.MetricStrategyType objectiveMetricName := trial.Spec.Objective.ObjectiveMetricName @@ -156,25 +156,21 @@ func getObjectiveMetricValue(trial trialsv1beta1.Trial) *string { if objectiveMetricName == metric.Name { switch objectiveStrategy { case commonv1beta1.ExtractByMin: - if math.IsNaN(metric.Min) { - return &metric.Latest + if metric.Min == consts.UnavailableMetricValue { + return metric.Latest } - value := fmt.Sprintf("%f", metric.Min) - return &value + return metric.Min case commonv1beta1.ExtractByMax: - if math.IsNaN(metric.Max) { - return &metric.Latest + if metric.Max == consts.UnavailableMetricValue { + return metric.Latest } - value := fmt.Sprintf("%f", metric.Max) - return &value + return metric.Max case commonv1beta1.ExtractByLatest: - return &metric.Latest - default: - return nil + return metric.Latest } } } - return nil + return consts.UnavailableMetricValue } // UpdateExperimentStatusCondition updates the experiment status. diff --git a/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient.go b/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient.go index a17b0fe1d98..3977098f8ec 100644 --- a/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient.go +++ b/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient.go @@ -3,7 +3,6 @@ package suggestionclient import ( "context" "fmt" - "math" "time" "google.golang.org/grpc" @@ -18,6 +17,7 @@ import ( suggestionsv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/suggestions/v1beta1" trialsv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/trials/v1beta1" suggestionapi "github.com/kubeflow/katib/pkg/apis/manager/v1beta1" + "github.com/kubeflow/katib/pkg/controller.v1beta1/consts" "github.com/kubeflow/katib/pkg/controller.v1beta1/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -267,21 +267,19 @@ func convertTrialObservation(strategies []commonapiv1beta1.MetricStrategy, obser var value string switch strategy, _ := strategyMap[m.Name]; strategy { case commonapiv1beta1.ExtractByMin: - if math.IsNaN(m.Min) { + if m.Min == consts.UnavailableMetricValue { value = m.Latest } else { - value = fmt.Sprintf("%f", m.Min) + value = m.Min } case commonapiv1beta1.ExtractByMax: - if math.IsNaN(m.Max) { + if m.Max == consts.UnavailableMetricValue { value = m.Latest } else { - value = fmt.Sprintf("%f", m.Max) + value = m.Max } case commonapiv1beta1.ExtractByLatest: value = m.Latest - default: - value = m.Latest } resObservation.Metrics = append(resObservation.Metrics, &suggestionapi.Metric{ Name: m.Name, @@ -289,7 +287,6 @@ func convertTrialObservation(strategies []commonapiv1beta1.MetricStrategy, obser }) } } - return resObservation } diff --git a/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient_test.go b/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient_test.go index 8c69d71bc43..2b17ad89938 100644 --- a/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient_test.go +++ b/pkg/controller.v1beta1/suggestion/suggestionclient/suggestionclient_test.go @@ -3,7 +3,7 @@ package suggestionclient import ( "errors" "fmt" - "strconv" + "reflect" "testing" "time" @@ -21,6 +21,7 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "github.com/kubeflow/katib/pkg/controller.v1beta1/consts" "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -385,29 +386,91 @@ func TestConvertParameterType(t *testing.T) { } func TestConvertTrialObservation(t *testing.T) { - g := gomega.NewGomegaWithT(t) - var strategies = []commonv1beta1.MetricStrategy{ + + tcs := []struct { + strategies []commonv1beta1.MetricStrategy + inObservation *commonv1beta1.Observation + expectedObservation *suggestionapi.Observation + testDescription string + }{ + { + strategies: newFakeStrategies(), + inObservation: newFakeTrialObservation(), + expectedObservation: newFakeRequestObservation(), + testDescription: "Run with min, max and latest metrics extract", + }, + { + strategies: newFakeStrategies(), + inObservation: func() *commonapiv1beta1.Observation { + obsIn := newFakeTrialObservation() + obsIn.Metrics[0].Min = consts.UnavailableMetricValue + return obsIn + }(), + expectedObservation: func() *suggestionapi.Observation { + obsOut := newFakeRequestObservation() + obsOut.Metrics[0].Value = "0.05" + return obsOut + }(), + testDescription: "Observation doesn't have min metric, latest is assigned", + }, + { + strategies: newFakeStrategies(), + inObservation: func() *commonapiv1beta1.Observation { + obsIn := newFakeTrialObservation() + obsIn.Metrics[1].Max = consts.UnavailableMetricValue + return obsIn + }(), + expectedObservation: func() *suggestionapi.Observation { + obsOut := newFakeRequestObservation() + obsOut.Metrics[1].Value = "0.90" + return obsOut + }(), + testDescription: "Observation doesn't have max metric, latest is assigned", + }, + } + for _, tc := range tcs { + actualObservation := convertTrialObservation(tc.strategies, tc.inObservation) + if !reflect.DeepEqual(actualObservation, tc.expectedObservation) { + t.Errorf("Case: %v failed.\nExpected observation: %v \ngot: %v", tc.testDescription, tc.expectedObservation, actualObservation) + } + } +} + +func newFakeStrategies() []commonv1beta1.MetricStrategy { + return []commonv1beta1.MetricStrategy{ {Name: "error", Value: commonv1beta1.ExtractByMin}, {Name: "auc", Value: commonv1beta1.ExtractByMax}, {Name: "accuracy", Value: commonv1beta1.ExtractByLatest}, } - var observation = &commonv1beta1.Observation{ +} + +func newFakeTrialObservation() *commonv1beta1.Observation { + return &commonv1beta1.Observation{ Metrics: []commonv1beta1.Metric{ - {Name: "error", Min: 0.01, Max: 0.08, Latest: "0.05"}, - {Name: "auc", Min: 0.70, Max: 0.95, Latest: "0.90"}, - {Name: "accuracy", Min: 0.8, Max: 0.94, Latest: "0.93"}, + {Name: "error", Min: "0.01", Max: "0.08", Latest: "0.05"}, + {Name: "auc", Min: "0.70", Max: "0.95", Latest: "0.90"}, + {Name: "accuracy", Min: "0.8", Max: "0.94", Latest: "0.93"}, + }, + } +} + +func newFakeRequestObservation() *suggestionapi.Observation { + return &suggestionapi.Observation{ + Metrics: []*suggestionapi.Metric{ + { + Name: "error", + Value: "0.01", + }, + { + Name: "auc", + Value: "0.95", + }, + { + Name: "accuracy", + Value: "0.93", + }, }, } - obsPb := convertTrialObservation(strategies, observation) - g.Expect(obsPb.Metrics[0].Name).To(gomega.Equal("error")) - value, _ := strconv.ParseFloat(obsPb.Metrics[0].Value, 64) - g.Expect(value).To(gomega.Equal(0.01)) - g.Expect(obsPb.Metrics[1].Name).To(gomega.Equal("auc")) - value, _ = strconv.ParseFloat(obsPb.Metrics[1].Value, 64) - g.Expect(value).To(gomega.Equal(0.95)) - g.Expect(obsPb.Metrics[2].Name).To(gomega.Equal("accuracy")) - value, _ = strconv.ParseFloat(obsPb.Metrics[2].Value, 64) - g.Expect(value).To(gomega.Equal(0.93)) } func newFakeObjective() *commonapiv1beta1.ObjectiveSpec { diff --git a/pkg/controller.v1beta1/trial/trial_controller_test.go b/pkg/controller.v1beta1/trial/trial_controller_test.go index 7d233200a9b..58b7ef9022a 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_test.go +++ b/pkg/controller.v1beta1/trial/trial_controller_test.go @@ -280,11 +280,11 @@ func TestGetObjectiveMetricValue(t *testing.T) { errMetric, accMetric, err := getMetricsFromLogs(metricStrategies) g.Expect(err).ShouldNot(gomega.HaveOccurred()) g.Expect(errMetric.Latest).To(gomega.Equal("0.07")) - g.Expect(errMetric.Max).To(gomega.Equal(0.1)) - g.Expect(errMetric.Min).To(gomega.Equal(0.01)) + g.Expect(errMetric.Max).To(gomega.Equal("0.1")) + g.Expect(errMetric.Min).To(gomega.Equal("0.01")) g.Expect(accMetric.Latest).To(gomega.Equal("0.67")) - g.Expect(accMetric.Max).To(gomega.Equal(0.72)) - g.Expect(accMetric.Min).To(gomega.Equal(0.6)) + g.Expect(accMetric.Max).To(gomega.Equal("0.72")) + g.Expect(accMetric.Min).To(gomega.Equal("0.6")) } func newFakeTrialWithTFJob() *trialsv1beta1.Trial { diff --git a/pkg/controller.v1beta1/trial/trial_controller_util.go b/pkg/controller.v1beta1/trial/trial_controller_util.go index 2327bc4a0e6..d5275f90ac5 100644 --- a/pkg/controller.v1beta1/trial/trial_controller_util.go +++ b/pkg/controller.v1beta1/trial/trial_controller_util.go @@ -18,7 +18,6 @@ package trial import ( "context" "fmt" - "math" "strconv" "time" @@ -30,6 +29,7 @@ import ( commonv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1" trialsv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/trials/v1beta1" api_pb "github.com/kubeflow/katib/pkg/apis/manager/v1beta1" + "github.com/kubeflow/katib/pkg/controller.v1beta1/consts" commonv1 "github.com/kubeflow/tf-operator/pkg/apis/common/v1" ) @@ -130,7 +130,7 @@ func isTrialObservationAvailable(instance *trialsv1beta1.Trial) bool { objectiveMetricName := instance.Spec.Objective.ObjectiveMetricName if instance.Status.Observation != nil && instance.Status.Observation.Metrics != nil { for _, metric := range instance.Status.Observation.Metrics { - if metric.Name == objectiveMetricName { + if metric.Name == objectiveMetricName && metric.Latest != consts.UnavailableMetricValue { return true } } @@ -172,9 +172,9 @@ func getMetrics(metricLogs []*api_pb.MetricLog, strategies []commonv1beta1.Metri timestamps[strategy.Name] = nil metrics[strategy.Name] = &commonv1beta1.Metric{ Name: strategy.Name, - Min: math.NaN(), - Max: math.NaN(), - Latest: "", + Min: consts.UnavailableMetricValue, + Max: consts.UnavailableMetricValue, + Latest: consts.UnavailableMetricValue, } } @@ -186,11 +186,18 @@ func getMetrics(metricLogs []*api_pb.MetricLog, strategies []commonv1beta1.Metri strValue := metricLog.Metric.Value floatValue, err := strconv.ParseFloat(strValue, 64) if err == nil { - if math.IsNaN(metric.Min) || floatValue < metric.Min { - metric.Min = floatValue - } - if math.IsNaN(metric.Max) || floatValue > metric.Max { - metric.Max = floatValue + if metric.Min == consts.UnavailableMetricValue { + metric.Min = strValue + metric.Max = strValue + } else { + // We can't get error here, because we parsed this value before + minMetric, _ := strconv.ParseFloat(metric.Min, 64) + maxMetric, _ := strconv.ParseFloat(metric.Max, 64) + if floatValue < minMetric { + metric.Min = strValue + } else if floatValue > maxMetric { + metric.Max = strValue + } } } currentTime, err := time.Parse(time.RFC3339Nano, metricLog.TimeStamp) diff --git a/test/e2e/v1beta1/resume-e2e-experiment.go b/test/e2e/v1beta1/resume-e2e-experiment.go index cc0137c262e..3805337ad0c 100644 --- a/test/e2e/v1beta1/resume-e2e-experiment.go +++ b/test/e2e/v1beta1/resume-e2e-experiment.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "log" "os" + "strconv" "time" "k8s.io/apimachinery/pkg/api/errors" @@ -142,9 +143,12 @@ func main() { if exp.Spec.Objective.Goal != nil { goal = *exp.Spec.Objective.Goal } - if (exp.Spec.Objective.Goal != nil && objectiveType == commonv1beta1.ObjectiveTypeMinimize && metric.Min < goal) || - (exp.Spec.Objective.Goal != nil && objectiveType == commonv1beta1.ObjectiveTypeMaximize && metric.Max > goal) { - log.Print("Objective Goal reached") + // If min metric is set, max be set also + minMetric, err := strconv.ParseFloat(metric.Min, 64) + maxMetric, _ := strconv.ParseFloat(metric.Max, 64) + if err == nil && + ((exp.Spec.Objective.Goal != nil && objectiveType == commonv1beta1.ObjectiveTypeMinimize && minMetric < goal) || + (exp.Spec.Objective.Goal != nil && objectiveType == commonv1beta1.ObjectiveTypeMaximize && maxMetric > goal)) { } else { if exp.Status.Trials != *exp.Spec.MaxTrialCount { diff --git a/test/e2e/v1beta1/run-e2e-experiment.go b/test/e2e/v1beta1/run-e2e-experiment.go index f72f26a3137..7d32e711d67 100644 --- a/test/e2e/v1beta1/run-e2e-experiment.go +++ b/test/e2e/v1beta1/run-e2e-experiment.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "log" "os" + "strconv" "time" "k8s.io/apimachinery/pkg/api/errors" @@ -125,8 +126,12 @@ func main() { if exp.Spec.Objective.Goal != nil { goal = *exp.Spec.Objective.Goal } - if (exp.Spec.Objective.Goal != nil && objectiveType == commonv1beta1.ObjectiveTypeMinimize && metric.Min < goal) || - (exp.Spec.Objective.Goal != nil && objectiveType == commonv1beta1.ObjectiveTypeMaximize && metric.Max > goal) { + // If min metric is set, max be set also + minMetric, err := strconv.ParseFloat(metric.Min, 64) + maxMetric, _ := strconv.ParseFloat(metric.Max, 64) + if err == nil && + ((exp.Spec.Objective.Goal != nil && objectiveType == commonv1beta1.ObjectiveTypeMinimize && minMetric < goal) || + (exp.Spec.Objective.Goal != nil && objectiveType == commonv1beta1.ObjectiveTypeMaximize && maxMetric > goal)) { log.Print("Objective Goal reached") } else {