From 171d37e55c005066f61296cf35de56b73ff83263 Mon Sep 17 00:00:00 2001 From: tenzen-y Date: Sun, 10 Oct 2021 15:22:37 +0900 Subject: [PATCH] fix error messages --- .../components/controller/katib-config.yaml | 6 +- .../experiment/manifest/generator.go | 6 +- .../suggestion/composer/composer.go | 2 +- .../v1beta1/experiment/manifest/generator.go | 2 +- pkg/util/v1beta1/katibconfig/config.go | 47 +----- pkg/util/v1beta1/katibconfig/config_test.go | 141 +++--------------- .../v1beta1/experiment/validator/validator.go | 2 +- 7 files changed, 37 insertions(+), 169 deletions(-) diff --git a/manifests/v1beta1/components/controller/katib-config.yaml b/manifests/v1beta1/components/controller/katib-config.yaml index 3e292a7c5cc..0d3079079d1 100644 --- a/manifests/v1beta1/components/controller/katib-config.yaml +++ b/manifests/v1beta1/components/controller/katib-config.yaml @@ -62,10 +62,6 @@ data: early-stopping: |- { "medianstop": { - "image": "docker.io/kubeflowkatib/earlystopping-medianstop:latest", - "algorithmSettings": { - "min_trials_required": "int", - "start_step": "int" - } + "image": "docker.io/kubeflowkatib/earlystopping-medianstop:latest" } } diff --git a/pkg/controller.v1beta1/experiment/manifest/generator.go b/pkg/controller.v1beta1/experiment/manifest/generator.go index ee50161d4f9..762f98c344f 100644 --- a/pkg/controller.v1beta1/experiment/manifest/generator.go +++ b/pkg/controller.v1beta1/experiment/manifest/generator.go @@ -38,7 +38,7 @@ type Generator interface { GetTrialTemplate(instance *experimentsv1beta1.Experiment) (string, error) GetRunSpecWithHyperParameters(experiment *experimentsv1beta1.Experiment, trialName, trialNamespace string, assignments []commonapiv1beta1.ParameterAssignment) (*unstructured.Unstructured, error) GetSuggestionConfigData(algorithmName string) (katibconfig.SuggestionConfig, error) - GetEarlyStoppingConfigData(earlyStoppingSpec *commonapiv1beta1.EarlyStoppingSpec) (katibconfig.EarlyStoppingConfig, error) + GetEarlyStoppingConfigData(algorithmName string) (katibconfig.EarlyStoppingConfig, error) GetMetricsCollectorConfigData(cKind commonapiv1beta1.CollectorKind) (katibconfig.MetricsCollectorConfig, error) } @@ -70,8 +70,8 @@ func (g *DefaultGenerator) GetSuggestionConfigData(algorithmName string) (katibc } // GetEarlyStoppingConfigData returns early stopping configuration for a given algorithm. -func (g *DefaultGenerator) GetEarlyStoppingConfigData(earlyStoppingSpec *commonapiv1beta1.EarlyStoppingSpec) (katibconfig.EarlyStoppingConfig, error) { - return katibconfig.GetEarlyStoppingConfigData(earlyStoppingSpec, g.client.GetClient()) +func (g *DefaultGenerator) GetEarlyStoppingConfigData(algorithmName string) (katibconfig.EarlyStoppingConfig, error) { + return katibconfig.GetEarlyStoppingConfigData(algorithmName, g.client.GetClient()) } // GetRunSpecWithHyperParameters returns the specification for trial with hyperparameters. diff --git a/pkg/controller.v1beta1/suggestion/composer/composer.go b/pkg/controller.v1beta1/suggestion/composer/composer.go index 1733dfd0dab..a4faa0906ea 100644 --- a/pkg/controller.v1beta1/suggestion/composer/composer.go +++ b/pkg/controller.v1beta1/suggestion/composer/composer.go @@ -80,7 +80,7 @@ func (g *General) DesiredDeployment(s *suggestionsv1beta1.Suggestion) (*appsv1.D // If early stopping is used, get the config data. earlyStoppingConfigData := katibconfig.EarlyStoppingConfig{} if s.Spec.EarlyStopping != nil && s.Spec.EarlyStopping.AlgorithmName != "" { - earlyStoppingConfigData, err = katibconfig.GetEarlyStoppingConfigData(s.Spec.EarlyStopping, g.Client) + earlyStoppingConfigData, err = katibconfig.GetEarlyStoppingConfigData(s.Spec.EarlyStopping.AlgorithmName, g.Client) if err != nil { return nil, err } diff --git a/pkg/mock/v1beta1/experiment/manifest/generator.go b/pkg/mock/v1beta1/experiment/manifest/generator.go index 5322c848d88..462a15e58c4 100644 --- a/pkg/mock/v1beta1/experiment/manifest/generator.go +++ b/pkg/mock/v1beta1/experiment/manifest/generator.go @@ -39,7 +39,7 @@ func (m *MockGenerator) EXPECT() *MockGeneratorMockRecorder { } // GetEarlyStoppingConfigData mocks base method. -func (m *MockGenerator) GetEarlyStoppingConfigData(arg0 *v1beta1.EarlyStoppingSpec) (katibconfig.EarlyStoppingConfig, error) { +func (m *MockGenerator) GetEarlyStoppingConfigData(arg0 string) (katibconfig.EarlyStoppingConfig, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetEarlyStoppingConfigData", arg0) ret0, _ := ret[0].(katibconfig.EarlyStoppingConfig) diff --git a/pkg/util/v1beta1/katibconfig/config.go b/pkg/util/v1beta1/katibconfig/config.go index 1b2133c1c96..8eed895ceee 100644 --- a/pkg/util/v1beta1/katibconfig/config.go +++ b/pkg/util/v1beta1/katibconfig/config.go @@ -20,7 +20,6 @@ import ( "context" "encoding/json" "fmt" - "strconv" "strings" corev1 "k8s.io/api/core/v1" @@ -47,17 +46,10 @@ type SuggestionConfig struct { // EarlyStoppingConfig is the JSON early stopping structure in Katib config. type EarlyStoppingConfig struct { - Image string `json:"image"` - ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` - AlgorithmSettings map[string]EarlyStoppingAlgorithmSettingValueType `json:"algorithmSettings,omitempty"` + Image string `json:"image"` + ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` } -type EarlyStoppingAlgorithmSettingValueType string - -const ( - ValueTypeInt EarlyStoppingAlgorithmSettingValueType = "int" -) - // MetricsCollectorConfig is the JSON metrics collector structure in Katib config. type MetricsCollectorConfig struct { Image string `json:"image"` @@ -147,7 +139,7 @@ func GetSuggestionConfigData(algorithmName string, client client.Client) (Sugges } // GetEarlyStoppingConfigData gets the config data for the given early stopping algorithm name. -func GetEarlyStoppingConfigData(earlyStoppingSpec *common.EarlyStoppingSpec, client client.Client) (EarlyStoppingConfig, error) { +func GetEarlyStoppingConfigData(algorithmName string, client client.Client) (EarlyStoppingConfig, error) { configMap := &corev1.ConfigMap{} earlyStoppingConfigData := EarlyStoppingConfig{} err := client.Get( @@ -171,47 +163,20 @@ func GetEarlyStoppingConfigData(earlyStoppingSpec *common.EarlyStoppingSpec, cli } // Try to find EarlyStoppingConfig for the algorithm. - earlyStoppingConfigData, ok = earlyStoppingsConfig[earlyStoppingSpec.AlgorithmName] + earlyStoppingConfigData, ok = earlyStoppingsConfig[algorithmName] if !ok { - return EarlyStoppingConfig{}, fmt.Errorf("failed to find early stopping config for algorithm: %s in ConfigMap: %s", earlyStoppingSpec.AlgorithmName, consts.KatibConfigMapName) + return EarlyStoppingConfig{}, fmt.Errorf("failed to find early stopping config for algorithm: %s in ConfigMap: %s", algorithmName, consts.KatibConfigMapName) } // Get image from config. image := earlyStoppingConfigData.Image if strings.TrimSpace(image) == "" { - return EarlyStoppingConfig{}, fmt.Errorf("required value for image configuration of algorithm name: %s", earlyStoppingSpec.AlgorithmName) + return EarlyStoppingConfig{}, fmt.Errorf("required value for image configuration of algorithm name: %s", algorithmName) } // Set Image Pull Policy. earlyStoppingConfigData.ImagePullPolicy = setImagePullPolicy(earlyStoppingConfigData.ImagePullPolicy) - // Early stopping service add default values. - if earlyStoppingSpec.AlgorithmSettings == nil || earlyStoppingConfigData.AlgorithmSettings == nil { - return earlyStoppingConfigData, nil - } - - // Get algorithmSettings. - algorithmSettingsConfigData := earlyStoppingConfigData.AlgorithmSettings - for _, inputSetting := range earlyStoppingSpec.AlgorithmSettings { - if inputSetting.Name == "" || inputSetting.Value == "" { - return EarlyStoppingConfig{}, fmt.Errorf("required value for early stopping algprithm settings of algorithm name: %s", earlyStoppingSpec.AlgorithmName) - } - - valueTypeConfigData, exist := algorithmSettingsConfigData[inputSetting.Name] - if !exist { - return EarlyStoppingConfig{}, fmt.Errorf("failed to find early stopping config for algorithm settings: %s in ConfigMap: %s", inputSetting.Name, consts.KatibConfigMapName) - } - - switch valueTypeConfigData { - case ValueTypeInt: - if _, err = strconv.Atoi(inputSetting.Value); err != nil { - return EarlyStoppingConfig{}, fmt.Errorf("%s must be integer value for early stopping algorithm name: %s", inputSetting.Value, inputSetting.Name) - } - default: - return EarlyStoppingConfig{}, fmt.Errorf("failed to find early stopping algorithm setings value type: %s in ConfigMap: %s", valueTypeConfigData, consts.KatibConfigMapName) - } - } - return earlyStoppingConfigData, nil } diff --git a/pkg/util/v1beta1/katibconfig/config_test.go b/pkg/util/v1beta1/katibconfig/config_test.go index 9223ce01f72..58707e1c44e 100644 --- a/pkg/util/v1beta1/katibconfig/config_test.go +++ b/pkg/util/v1beta1/katibconfig/config_test.go @@ -153,25 +153,21 @@ func TestGetSuggestionConfigData(t *testing.T) { } -const ( - testEarlyStoppingAlgorithmName = "test-early-stopping-algorithm1" - testEarlyStoppingAlgorithmSettingName = "test-early-stopping-setting1" -) - func TestGetEarlyStoppingConfigData(t *testing.T) { + const testAlgorithmName = "test-early-stopping" tests := []struct { - testDescription string - katibConfig *katibConfig - expected *EarlyStoppingConfig - inputEarlyStoppingSpec *commonv1beta1.EarlyStoppingSpec - err bool + testDescription string + katibConfig *katibConfig + expected *EarlyStoppingConfig + inputAlgorithmName string + err bool }{ { testDescription: "All parameters correctly are specified", katibConfig: func() *katibConfig { - kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}} - kc.earlyStopping[testEarlyStoppingAlgorithmName].ImagePullPolicy = corev1.PullIfNotPresent + kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testAlgorithmName: newFakeEarlyStoppingConfig()}} + kc.earlyStopping[testAlgorithmName].ImagePullPolicy = corev1.PullIfNotPresent return kc }(), expected: func() *EarlyStoppingConfig { @@ -179,8 +175,8 @@ func TestGetEarlyStoppingConfigData(t *testing.T) { c.ImagePullPolicy = corev1.PullIfNotPresent return c }(), - inputEarlyStoppingSpec: newFakeEarlyStoppingSpec(), - err: false, + inputAlgorithmName: testAlgorithmName, + err: false, }, { testDescription: "There is not katib-config.", @@ -193,112 +189,38 @@ func TestGetEarlyStoppingConfigData(t *testing.T) { err: true, }, { - testDescription: "There is not the AlgorithmName", - katibConfig: &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}}, - inputEarlyStoppingSpec: func() *commonv1beta1.EarlyStoppingSpec { - es := newFakeEarlyStoppingSpec() - es.AlgorithmName = "invalid-algorithm-name" - return es - }(), - err: true, + testDescription: "There is not the AlgorithmName", + katibConfig: &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testAlgorithmName: newFakeEarlyStoppingConfig()}}, + inputAlgorithmName: "invalid-algorithm-name", + err: true, }, { testDescription: "Image filed is empty in katib-config configMap", katibConfig: func() *katibConfig { - kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}} - kc.earlyStopping[testEarlyStoppingAlgorithmName].Image = "" + kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testAlgorithmName: newFakeEarlyStoppingConfig()}} + kc.earlyStopping[testAlgorithmName].Image = "" return kc }(), - inputEarlyStoppingSpec: newFakeEarlyStoppingSpec(), - err: true, + inputAlgorithmName: testAlgorithmName, + err: true, }, { testDescription: fmt.Sprintf("GetEarlyStoppingConfigData sets %s to imagePullPolicy", consts.DefaultImagePullPolicy), katibConfig: func() *katibConfig { - kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}} - kc.earlyStopping[testEarlyStoppingAlgorithmName].ImagePullPolicy = "" - return kc - }(), - expected: newFakeEarlyStoppingConfig(), - inputEarlyStoppingSpec: newFakeEarlyStoppingSpec(), - err: false, - }, - { - testDescription: "There is not the EarlyStoppingSettings in Experiment resource", - katibConfig: &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}}, - inputEarlyStoppingSpec: func() *commonv1beta1.EarlyStoppingSpec { - es := newFakeEarlyStoppingSpec() - es.AlgorithmSettings = nil - return es - }(), - err: false, - }, - { - testDescription: "There is not the algorithmSettings field in katib-config ConfigMap", - katibConfig: func() *katibConfig { - kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}} - kc.earlyStopping[testEarlyStoppingAlgorithmName].AlgorithmSettings = nil + kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testAlgorithmName: newFakeEarlyStoppingConfig()}} + kc.earlyStopping[testAlgorithmName].ImagePullPolicy = "" return kc }(), - inputEarlyStoppingSpec: newFakeEarlyStoppingSpec(), - err: false, - }, - { - testDescription: "EarlyStoppingSettings name field is empty in Experiment resource", - katibConfig: &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}}, - inputEarlyStoppingSpec: func() *commonv1beta1.EarlyStoppingSpec { - es := newFakeEarlyStoppingSpec() - es.AlgorithmSettings[0].Name = "" - return es - }(), - err: true, - }, - { - testDescription: "EarlyStoppingSettings value field is empty in Experiment resource", - katibConfig: &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}}, - inputEarlyStoppingSpec: func() *commonv1beta1.EarlyStoppingSpec { - es := newFakeEarlyStoppingSpec() - es.AlgorithmSettings[0].Value = "" - return es - }(), - err: true, - }, - { - testDescription: "Set invalid algorithm setting name for early stopping in Experiment resource", - katibConfig: &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}}, - inputEarlyStoppingSpec: func() *commonv1beta1.EarlyStoppingSpec { - es := newFakeEarlyStoppingSpec() - es.AlgorithmSettings[0].Name = "invalid-algorithm-setting-name" - return es - }(), - err: true, - }, - { - testDescription: "Set invalid algorithm setting value for early stopping in Experiment resource", - katibConfig: &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}}, - inputEarlyStoppingSpec: func() *commonv1beta1.EarlyStoppingSpec { - es := newFakeEarlyStoppingSpec() - es.AlgorithmSettings[0].Value = "invalid-value-type" - return es - }(), - err: true, - }, - { - testDescription: "Set invalid algorithm setting value for early stopping in katib-config ConfigMap", - katibConfig: func() *katibConfig { - kc := &katibConfig{earlyStopping: map[string]*EarlyStoppingConfig{testEarlyStoppingAlgorithmName: newFakeEarlyStoppingConfig()}} - kc.earlyStopping[testEarlyStoppingAlgorithmName].AlgorithmSettings[testEarlyStoppingAlgorithmSettingName] = "invalid-type" - return kc - }(), - inputEarlyStoppingSpec: newFakeEarlyStoppingSpec(), - err: true, + expected: newFakeEarlyStoppingConfig(), + inputAlgorithmName: testAlgorithmName, + err: false, }, } for _, tt := range tests { t.Run(tt.testDescription, func(t *testing.T) { fakeKubeClient := newFakeKubeClient(newFakeKatibConfigMap(tt.katibConfig)) - actual, err := GetEarlyStoppingConfigData(tt.inputEarlyStoppingSpec, fakeKubeClient) + actual, err := GetEarlyStoppingConfigData(tt.inputAlgorithmName, fakeKubeClient) if (err != nil) != tt.err { t.Errorf("want error: %v, actual: %v", tt.err, err) } else if tt.expected != nil { @@ -488,21 +410,6 @@ func newFakeEarlyStoppingConfig() *EarlyStoppingConfig { return &EarlyStoppingConfig{ Image: "early-stopping-image", ImagePullPolicy: consts.DefaultImagePullPolicy, - AlgorithmSettings: map[string]EarlyStoppingAlgorithmSettingValueType{ - testEarlyStoppingAlgorithmSettingName: ValueTypeInt, - }, - } -} - -func newFakeEarlyStoppingSpec() *commonv1beta1.EarlyStoppingSpec { - return &commonv1beta1.EarlyStoppingSpec{ - AlgorithmName: testEarlyStoppingAlgorithmName, - AlgorithmSettings: []commonv1beta1.EarlyStoppingSetting{ - { - Name: testEarlyStoppingAlgorithmSettingName, - Value: "2", - }, - }, } } diff --git a/pkg/webhook/v1beta1/experiment/validator/validator.go b/pkg/webhook/v1beta1/experiment/validator/validator.go index 71e359203ef..04d2d70c231 100644 --- a/pkg/webhook/v1beta1/experiment/validator/validator.go +++ b/pkg/webhook/v1beta1/experiment/validator/validator.go @@ -183,7 +183,7 @@ func (g *DefaultValidator) validateEarlyStopping(es *commonapiv1beta1.EarlyStopp return fmt.Errorf("no spec.earlyStopping.algorithm.algorithmName specified") } - if _, err := g.GetEarlyStoppingConfigData(es); err != nil { + if _, err := g.GetEarlyStoppingConfigData(es.AlgorithmName); err != nil { return fmt.Errorf("unable to get EarlyStopping config data for algorithm %s: %v", es.AlgorithmName, err) }