Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduced error constants and replaced reflect with cmp #2289

Merged
merged 2 commits into from
Aug 18, 2024
Merged

Introduced error constants and replaced reflect with cmp #2289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7648fb7
introduced error constants and replaced reflect with cmp
tariq-hasan Aug 18, 2024
0cd9b85
fix order of mock method calls
tariq-hasan Aug 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions pkg/controller.v1beta1/experiment/manifest/generator.go
View file
Open in desktop
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
Open in desktop
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