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 3 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
10 changes: 10 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,11 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex
trialParametersNames[parameter.Name] = true
trialParametersRefs[parameter.Reference] = true

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for NAS Experiments that doesn't work.
For example, for the ENAS Experiment, the Suggestion returns architecture and nn_config which don't exist in spec.Parameters.
For HP Experiments, currently all parameters that Suggestions return exist in spec.parameters, but I am not sure if for other AutoML techniques that always be true (for example for PBT, AutoMC, etc.).

I think for now, we can check if experimentParameterNames is not empty than we can apply this check.

WDYT @henrysecond1 @anencore94 @gaocegege @johnugeorge ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreyvelich Thank you for pointing it out. Sounds good to me, too :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that make sense ! Thanks


// 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
14 changes: 14 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,7 @@ 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)

missedParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(missedParameterJobStr, nil)
oddParameterTemplate := p.EXPECT().GetTrialTemplate(gomock.Any()).Return(oddParameterJobStr, nil)
Expand All @@ -406,6 +407,7 @@ spec:
validTemplate2,
validTemplate3,
validTemplate4,
validTemplate5,
missedParameterTemplate,
oddParameterTemplate,
invalidParameterTemplate,
Expand Down Expand Up @@ -524,6 +526,16 @@ 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 doesn't contain parameter from trialParameters
// missedParameterTemplate case
{
Expand Down Expand Up @@ -1019,13 +1031,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