Skip to content

fix: check if parameter references exist in experiment parameters #1726

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

Merged
merged 5 commits into from
Nov 9, 2021
Merged

fix: check if parameter references exist in experiment parameters #1726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
488a935
fix: check if parameter references exist in experiment parameters
henrysecond1 Oct 29, 2021
b1a3eac
Fix validator test
henrysecond1 Oct 29, 2021
059cbbf
Update some comments and test descriptions
henrysecond1 Nov 1, 2021
81cf7de
Check trial parameter reference only when experiment parameters are n…
henrysecond1 Nov 2, 2021
38b1770
Add a test for the case 'spec.parameters' is mepty
henrysecond1 Nov 6, 2021
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
12 changes: 12 additions & 0 deletions pkg/webhook/v1beta1/experiment/validator/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,11 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex
return fmt.Errorf("unable to parse spec.trialTemplate: %v", err)
}

experimentParameterNames := make(map[string]bool)
for _, parameter := range instance.Spec.Parameters {
experimentParameterNames[parameter.Name] = true
}

trialParametersNames := make(map[string]bool)
trialParametersRefs := make(map[string]bool)

Expand All @@ -286,6 +291,13 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex
trialParametersNames[parameter.Name] = true
trialParametersRefs[parameter.Reference] = true

// Check if parameter reference exist in experiment parameters
if len(experimentParameterNames) > 0 {
if _, ok := experimentParameterNames[parameter.Reference]; !ok {
return fmt.Errorf("parameter reference %v does not exist in spec.parameters: %v", parameter.Reference, instance.Spec.Parameters)
}
}

// Check if trialParameters contains all substitution for Trial template
if !strings.Contains(trialTemplateStr, fmt.Sprintf(consts.TrialTemplateParamReplaceFormat, parameter.Name)) {
return fmt.Errorf("parameter name: %v in spec.trialParameters not found in spec.trialTemplate: %v", parameter.Name, trialTemplateStr)
Expand Down
27 changes: 27 additions & 0 deletions pkg/webhook/v1beta1/experiment/validator/validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,8 @@ spec:
validTemplate2 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)
validTemplate3 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)
validTemplate4 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)
validTemplate5 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)
validTemplate6 := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(validJobStr, nil)

missedParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(missedParameterJobStr, nil)
oddParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(oddParameterJobStr, nil)
Expand All @@ -406,6 +408,8 @@ spec:
validTemplate2,
validTemplate3,
validTemplate4,
validTemplate5,
validTemplate6,
missedParameterTemplate,
oddParameterTemplate,
invalidParameterTemplate,
Expand Down Expand Up @@ -524,6 +528,27 @@ spec:
Err: true,
testDescription: "Duplicate reference in Trial parameters",
},
// Trial template contains Trial parameters which weren't referenced from spec.parameters
{
Instance: func() *experimentsv1beta1.Experiment {
i := newFakeInstance()
i.Spec.TrialTemplate.TrialParameters[1].Reference = "wrong-ref"
return i
}(),
Err: true,
testDescription: "Trial template contains Trial parameters which weren't referenced from spec.parameters",
},
// Trial template contains Trial parameters when spec.parameters is empty
{
Instance: func() *experimentsv1beta1.Experiment {
i := newFakeInstance()
i.Spec.Parameters = nil
i.Spec.TrialTemplate.TrialParameters[1].Reference = "wrong-ref"
return i
}(),
Err: false,
testDescription: "Trial template contains Trial parameters when spec.parameters is empty",
},
// Trial Template doesn't contain parameter from trialParameters
// missedParameterTemplate case
{
Expand Down Expand Up @@ -1019,13 +1044,15 @@ func newFakeInstance() *experimentsv1beta1.Experiment {
},
Parameters: []experimentsv1beta1.ParameterSpec{
{
Name: "lr",
ParameterType: experimentsv1beta1.ParameterTypeInt,
FeasibleSpace: experimentsv1beta1.FeasibleSpace{
Max: "5",
Min: "1",
},
},
{
Name: "num-layers",
ParameterType: experimentsv1beta1.ParameterTypeCategorical,
FeasibleSpace: experimentsv1beta1.FeasibleSpace{
List: []string{"1", "2", "3"},
Expand Down