Skip to content

Commit

Permalink
Introduced error constants and replaced reflect with cmp (kubeflow#2289)
Browse files Browse the repository at this point in the history 
* introduced error constants and replaced reflect with cmp

Signed-off-by: tariq-hasan <[email protected]>

* fix order of mock method calls

Signed-off-by: tariq-hasan <[email protected]>

---------

Signed-off-by: tariq-hasan <[email protected]>
  • Loading branch information
@tariq-hasan
tariq-hasan authored Aug 18, 2024
1 parent 2f5bda2 commit abd1c42
Show file tree
Hide file tree
Showing 7 changed files with 258 additions and 282 deletions.
27 changes: 19 additions & 8 deletions pkg/controller.v1beta1/experiment/manifest/generator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package manifest

import (
"errors"
"fmt"
"regexp"
"strings"
Expand All @@ -33,6 +34,15 @@ import (
"github.com/kubeflow/katib/pkg/util/v1beta1/katibconfig"
)

var (
errConfigMapNotFound = errors.New("configMap not found")
errConvertStringToUnstructuredFailed = errors.New("failed to convert string to unstructured")
errConvertUnstructuredToStringFailed = errors.New("failed to convert unstructured to string")
errParamNotFoundInParameterAssignment = errors.New("unable to find non-meta parameter from TrialParameters in ParameterAssignment")
errParamNotFoundInTrialParameters = errors.New("unable to find parameter from ParameterAssignment in TrialParameters")
errTrialTemplateNotFound = errors.New("unable to find trial template in ConfigMap")
)

// Generator is the type for manifests Generator.
type Generator interface {
InjectClient(c client.Client)
Expand Down Expand Up @@ -86,7 +96,7 @@ func (g *DefaultGenerator) GetRunSpecWithHyperParameters(experiment *experiments
// Convert Trial template to unstructured
runSpec, err := util.ConvertStringToUnstructured(replacedTemplate)
if err != nil {
return nil, fmt.Errorf("ConvertStringToUnstructured failed: %v", err)
return nil, fmt.Errorf("%w: %w", errConvertStringToUnstructuredFailed, err)
}

// Set name and namespace for Run Spec
Expand All @@ -108,7 +118,7 @@ func (g *DefaultGenerator) applyParameters(experiment *experimentsv1beta1.Experi
if trialSpec == nil {
trialSpec, err = util.ConvertStringToUnstructured(trialTemplate)
if err != nil {
return "", fmt.Errorf("ConvertStringToUnstructured failed: %v", err)
return "", fmt.Errorf("%w: %w", errConvertStringToUnstructuredFailed, err)
}
}

Expand All @@ -131,7 +141,7 @@ func (g *DefaultGenerator) applyParameters(experiment *experimentsv1beta1.Experi
nonMetaParamCount += 1
continue
} else {
return "", fmt.Errorf("Unable to find parameter: %v in parameter assignment %v", param.Reference, assignmentsMap)
return "", fmt.Errorf("%w: parameter: %v, parameter assignment: %v", errParamNotFoundInParameterAssignment, param.Reference, assignmentsMap)
}
}
metaRefKey = sub[1]
Expand Down Expand Up @@ -172,9 +182,10 @@ func (g *DefaultGenerator) applyParameters(experiment *experimentsv1beta1.Experi
}
}

// Number of parameters must be equal
// Number of assignment parameters must be equal to the number of non-meta trial parameters
// i.e. all parameters in ParameterAssignment must be in TrialParameters
if len(assignments) != nonMetaParamCount {
return "", fmt.Errorf("Number of TrialAssignment: %v != number of nonMetaTrialParameters in TrialSpec: %v", len(assignments), nonMetaParamCount)
return "", fmt.Errorf("%w: parameter assignments: %v, non-meta trial parameter count: %v", errParamNotFoundInTrialParameters, assignments, nonMetaParamCount)
}

// Replacing placeholders with parameter values
Expand All @@ -194,20 +205,20 @@ func (g *DefaultGenerator) GetTrialTemplate(instance *experimentsv1beta1.Experim
if trialSource.TrialSpec != nil {
trialTemplateString, err = util.ConvertUnstructuredToString(trialSource.TrialSpec)
if err != nil {
return "", fmt.Errorf("ConvertUnstructuredToString failed: %v", err)
return "", fmt.Errorf("%w: %w", errConvertUnstructuredToStringFailed, err)
}
} else {
configMapNS := trialSource.ConfigMap.ConfigMapNamespace
configMapName := trialSource.ConfigMap.ConfigMapName
templatePath := trialSource.ConfigMap.TemplatePath
configMap, err := g.client.GetConfigMap(configMapName, configMapNS)
if err != nil {
return "", fmt.Errorf("GetConfigMap failed: %v", err)
return "", fmt.Errorf("%w: %w", errConfigMapNotFound, err)
}
var ok bool
trialTemplateString, ok = configMap[templatePath]
if !ok {
return "", fmt.Errorf("TemplatePath: %v not found in configMap: %v", templatePath, configMap)
return "", fmt.Errorf("%w: TemplatePath: %v, ConfigMap: %v", errTrialTemplateNotFound, templatePath, configMap)
}
}

Expand Down
182 changes: 82 additions & 100 deletions pkg/controller.v1beta1/experiment/manifest/generator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ limitations under the License.
package manifest

import (
"errors"
"math"
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"go.uber.org/mock/gomock"
batchv1 "k8s.io/api/batch/v1"
v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -88,23 +88,18 @@ func TestGetRunSpecWithHP(t *testing.T) {
t.Errorf("ConvertObjectToUnstructured failed: %v", err)
}

tcs := []struct {
instance *experimentsv1beta1.Experiment
parameterAssignments []commonapiv1beta1.ParameterAssignment
expectedRunSpec *unstructured.Unstructured
err bool
testDescription string
cases := map[string]struct {
instance *experimentsv1beta1.Experiment
parameterAssignments []commonapiv1beta1.ParameterAssignment
wantRunSpecWithHyperParameters *unstructured.Unstructured
wantError error
}{
// Valid run
{
instance: newFakeInstance(),
parameterAssignments: newFakeParameterAssignment(),
expectedRunSpec: expectedRunSpec,
err: false,
testDescription: "Run with valid parameters",
"Run with valid parameters": {
instance: newFakeInstance(),
parameterAssignments: newFakeParameterAssignment(),
wantRunSpecWithHyperParameters: expectedRunSpec,
},
// Invalid JSON in unstructured
{
"Invalid JSON in Unstructured Trial template": {
instance: func() *experimentsv1beta1.Experiment {
i := newFakeInstance()
trialSpec := i.Spec.TrialTemplate.TrialSource.TrialSpec
Expand All @@ -114,48 +109,45 @@ func TestGetRunSpecWithHP(t *testing.T) {
return i
}(),
parameterAssignments: newFakeParameterAssignment(),
err: true,
testDescription: "Invalid JSON in Trial template",
wantError: errConvertUnstructuredToStringFailed,
},
// len(parameterAssignment) != len(trialParameters)
{
"Non-meta parameter from TrialParameters not found in ParameterAssignment": {
instance: newFakeInstance(),
parameterAssignments: func() []commonapiv1beta1.ParameterAssignment {
pa := newFakeParameterAssignment()
pa = pa[1:]
pa[0] = commonapiv1beta1.ParameterAssignment{
Name: "invalid-name",
Value: "invalid-value",
}
return pa
}(),
err: true,
testDescription: "Number of parameter assignments is not equal to number of Trial parameters",
wantError: errParamNotFoundInParameterAssignment,
},
// Parameter from assignments not found in Trial parameters
{
// case in which the lengths of trial parameters and parameter assignments are different
"Parameter from ParameterAssignment not found in TrialParameters": {
instance: newFakeInstance(),
parameterAssignments: func() []commonapiv1beta1.ParameterAssignment {
pa := newFakeParameterAssignment()
pa[0] = commonapiv1beta1.ParameterAssignment{
Name: "invalid-name",
Value: "invalid-value",
}
pa = append(pa, commonapiv1beta1.ParameterAssignment{
Name: "extra-name",
Value: "extra-value",
})
return pa
}(),
err: true,
testDescription: "Trial parameters don't have parameter from assignments",
wantError: errParamNotFoundInTrialParameters,
},
}

for _, tc := range tcs {
actualRunSpec, err := p.GetRunSpecWithHyperParameters(tc.instance, "trial-name", "trial-namespace", tc.parameterAssignments)

if tc.err && err == nil {
t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription)
} else if !tc.err {
if err != nil {
t.Errorf("Case: %v failed. Expected nil, got %v", tc.testDescription, err)
} else if !reflect.DeepEqual(tc.expectedRunSpec, actualRunSpec) {
t.Errorf("Case: %v failed. Expected %v\n got %v", tc.testDescription, tc.expectedRunSpec.Object, actualRunSpec.Object)
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got, err := p.GetRunSpecWithHyperParameters(tc.instance, "trial-name", "trial-namespace", tc.parameterAssignments)
if diff := cmp.Diff(tc.wantError, err, cmpopts.EquateErrors()); len(diff) != 0 {
t.Errorf("Unexpected error from GetRunSpecWithHyperParameters (-want,+got):\n%s", diff)
}
}
if diff := cmp.Diff(tc.wantRunSpecWithHyperParameters, got); len(diff) != 0 {
t.Errorf("Unexpected run spec from GetRunSpecWithHyperParameters (-want,+got):\n%s", diff)
}
})
}
}

Expand Down Expand Up @@ -204,25 +196,6 @@ spec:
- --momentum=${trialParameters.momentum}
- --invalidParameter={'num_layers': 2, 'input_sizes': [32, 32, 3]}`

validGetConfigMap1 := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
map[string]string{templatePath: trialSpec}, nil)

invalidConfigMapName := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
nil, errors.New("Unable to get ConfigMap"))

validGetConfigMap3 := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
map[string]string{templatePath: trialSpec}, nil)

invalidTemplate := c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
map[string]string{templatePath: invalidTrialSpec}, nil)

gomock.InOrder(
validGetConfigMap1,
invalidConfigMapName,
validGetConfigMap3,
invalidTemplate,
)

// We can't compare structures, because in ConfigMap trialSpec is a string and creationTimestamp was not added
expectedStr := `apiVersion: batch/v1
kind: Job
Expand All @@ -244,19 +217,23 @@ spec:
- "--momentum=0.9"`

expectedRunSpec, err := util.ConvertStringToUnstructured(expectedStr)
if err != nil {
t.Errorf("ConvertStringToUnstructured failed: %v", err)
if diff := cmp.Diff(nil, err, cmpopts.EquateErrors()); len(diff) != 0 {
t.Errorf("ConvertStringToUnstructured failed (-want,+got):\n%s", diff)
}

tcs := []struct {
instance *experimentsv1beta1.Experiment
parameterAssignments []commonapiv1beta1.ParameterAssignment
err bool
testDescription string
cases := map[string]struct {
mockConfigMapGetter func() *gomock.Call
instance *experimentsv1beta1.Experiment
parameterAssignments []commonapiv1beta1.ParameterAssignment
wantRunSpecWithHyperParameters *unstructured.Unstructured
wantError error
}{
// Valid run
// validGetConfigMap1 case
{
"Run with valid parameters": {
mockConfigMapGetter: func() *gomock.Call {
return c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
map[string]string{templatePath: trialSpec}, nil,
)
},
instance: func() *experimentsv1beta1.Experiment {
i := newFakeInstance()
i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{
Expand All @@ -268,13 +245,15 @@ spec:
}
return i
}(),
parameterAssignments: newFakeParameterAssignment(),
err: false,
testDescription: "Run with valid parameters",
parameterAssignments: newFakeParameterAssignment(),
wantRunSpecWithHyperParameters: expectedRunSpec,
},
// Invalid ConfigMap name
// invalidConfigMapName case
{
"Invalid ConfigMap name": {
mockConfigMapGetter: func() *gomock.Call {
return c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
nil, errConfigMapNotFound,
)
},
instance: func() *experimentsv1beta1.Experiment {
i := newFakeInstance()
i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{
Expand All @@ -285,12 +264,14 @@ spec:
return i
}(),
parameterAssignments: newFakeParameterAssignment(),
err: true,
testDescription: "Invalid ConfigMap name",
wantError: errConfigMapNotFound,
},
// Invalid template path in ConfigMap name
// validGetConfigMap3 case
{
"Invalid template path in ConfigMap name": {
mockConfigMapGetter: func() *gomock.Call {
return c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
map[string]string{templatePath: trialSpec}, nil,
)
},
instance: func() *experimentsv1beta1.Experiment {
i := newFakeInstance()
i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{
Expand All @@ -303,14 +284,16 @@ spec:
return i
}(),
parameterAssignments: newFakeParameterAssignment(),
err: true,
testDescription: "Invalid template path in ConfigMap",
wantError: errTrialTemplateNotFound,
},
// Invalid Trial template spec in ConfigMap
// Trial template is a string in ConfigMap
// Because of that, user can specify not valid unstructured template
// invalidTemplate case
{
"Invalid trial spec in ConfigMap": {
mockConfigMapGetter: func() *gomock.Call {
return c.EXPECT().GetConfigMap(gomock.Any(), gomock.Any()).Return(
map[string]string{templatePath: invalidTrialSpec}, nil,
)
},
instance: func() *experimentsv1beta1.Experiment {
i := newFakeInstance()
i.Spec.TrialTemplate.TrialSource = experimentsv1beta1.TrialSource{
Expand All @@ -323,22 +306,21 @@ spec:
return i
}(),
parameterAssignments: newFakeParameterAssignment(),
err: true,
testDescription: "Invalid Trial spec in ConfigMap",
wantError: errConvertStringToUnstructuredFailed,
},
}

for _, tc := range tcs {
actualRunSpec, err := p.GetRunSpecWithHyperParameters(tc.instance, "trial-name", "trial-namespace", tc.parameterAssignments)
if tc.err && err == nil {
t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription)
} else if !tc.err {
if err != nil {
t.Errorf("Case: %v failed. Expected nil, got %v", tc.testDescription, err)
} else if !reflect.DeepEqual(expectedRunSpec, actualRunSpec) {
t.Errorf("Case: %v failed. Expected %v\n got %v", tc.testDescription, expectedRunSpec.Object, actualRunSpec.Object)
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
tc.mockConfigMapGetter()
got, err := p.GetRunSpecWithHyperParameters(tc.instance, "trial-name", "trial-namespace", tc.parameterAssignments)
if diff := cmp.Diff(tc.wantError, err, cmpopts.EquateErrors()); len(diff) != 0 {
t.Errorf("Unexpected error from GetRunSpecWithHyperParameters (-want,+got):\n%s", diff)
}
if diff := cmp.Diff(tc.wantRunSpecWithHyperParameters, got); len(diff) != 0 {
t.Errorf("Unexpected run spec from GetRunSpecWithHyperParameters (-want,+got):\n%s", diff)
}
}
})
}
}

Expand Down
Loading

0 comments on commit abd1c42

Please sign in to comment.