Skip to content

Commit

Permalink
fix error messages
Browse files Browse the repository at this point in the history
  • Loading branch information
@tenzen-y
tenzen-y committed Oct 27, 2021
1 parent 45ad4e8 commit 28324da
Show file tree
Hide file tree
Showing 7 changed files with 37 additions and 163 deletions.
6 changes: 1 addition & 5 deletions manifests/v1beta1/components/controller/katib-config.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
6 changes: 3 additions & 3 deletions pkg/controller.v1beta1/experiment/manifest/generator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller.v1beta1/suggestion/composer/composer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/mock/v1beta1/experiment/manifest/generator.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 6 additions & 35 deletions pkg/util/v1beta1/katibconfig/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"encoding/json"
"fmt"
"strconv"
"strings"

corev1 "k8s.io/api/core/v1"
Expand All @@ -47,9 +46,8 @@ 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
Expand Down Expand Up @@ -147,7 +145,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(
Expand All @@ -171,47 +169,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
}

Expand Down
141 changes: 24 additions & 117 deletions pkg/util/v1beta1/katibconfig/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,34 +153,30 @@ 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 {
c := newFakeEarlyStoppingConfig()
c.ImagePullPolicy = corev1.PullIfNotPresent
return c
}(),
inputEarlyStoppingSpec: newFakeEarlyStoppingSpec(),
err: false,
inputAlgorithmName: testAlgorithmName,
err: false,
},
{
testDescription: "There is not katib-config.",
Expand All @@ -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 {
Expand Down Expand Up @@ -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",
},
},
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/webhook/v1beta1/experiment/validator/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down

0 comments on commit 28324da

Please sign in to comment.