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

henrysecond1 · 2021-10-29T03:38:39Z

What this PR does / why we need it:

Hi, we experienced that the users tend to make mistakes like below when creating experiments using new UI (Thanks for the nice UI update!)

When users set parameter references in trial templates like below,

they need to enter the parameter reference as same as the hyperparameter defined in the experiment spec.
The experiment is created without errors even if the users mistakenly set the wrong parameter reference.

I think it would be better to check the wrong parameter reference in the validating webhook
so that experiments are not created with the wrong parameter reference.

Please take a look, thanks!

aws-kf-ci-bot · 2021-10-29T03:38:53Z

Hi @henrysecond1. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-oss-robot · 2021-10-29T03:38:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: henrysecond1
To complete the pull request process, please assign gaocegege after the PR has been reviewed.
You can assign the PR to them by writing /assign @gaocegege in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coveralls · 2021-10-29T04:09:39Z

Coverage increased (+0.1%) to 73.71% when pulling 38b1770 on henrysecond1:fix/wrong-parameter-reference into 0a38e87 on kubeflow:master.

anencore94

Thank you for such a nice contribution :) I think this feature helps many others

anencore94 · 2021-11-01T13:10:29Z

pkg/webhook/v1beta1/experiment/validator/validator.go

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


IMHO, How about move these codes after

trialParametersNames[parameter.Name] = true trialParametersRefs[parameter.Reference] = true

It may not important, but I feel like these should be right after codes right after checking duplicates.

Thanks for the suggestion, done :)

anencore94 · 2021-11-01T13:11:45Z

pkg/webhook/v1beta1/experiment/validator/validator.go

@@ -283,6 +288,10 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex
 		if _, ok := trialParametersRefs[parameter.Reference]; ok {
 			return fmt.Errorf("parameter reference %v can't be duplicated in spec.trialTemplate.trialParameters: %v", parameter.Reference, trialTemplate.TrialParameters)
 		}
+		// Check if parameter references exist in experiment parameters
+		if _, ok := experimentParameterNames[parameter.Reference]; !ok {
+			return fmt.Errorf("parameter reference %v does not exists in spec.Parameters: %v", parameter.Reference, instance.Spec.Parameters)


nit: does not exist

anencore94 · 2021-11-01T13:12:01Z

pkg/webhook/v1beta1/experiment/validator/validator.go

@@ -283,6 +288,10 @@ func (g *DefaultValidator) validateTrialTemplate(instance *experimentsv1beta1.Ex
 		if _, ok := trialParametersRefs[parameter.Reference]; ok {
 			return fmt.Errorf("parameter reference %v can't be duplicated in spec.trialTemplate.trialParameters: %v", parameter.Reference, trialTemplate.TrialParameters)
 		}
+		// Check if parameter references exist in experiment parameters


nit: parameter reference exist

anencore94 · 2021-11-01T13:32:49Z

pkg/webhook/v1beta1/experiment/validator/validator_test.go

+		// Wrong parameter reference in trialParameters
+		{
+			Instance: func() *experimentsv1beta1.Experiment {
+				i := newFakeInstance()
+				i.Spec.TrialTemplate.TrialParameters[1].Reference = "wrong-ref"
+				return i
+			}(),
+			Err:             true,
+			testDescription: "Wrong reference in Trial parameters",
+		},


nit: How about make this test case as invalidReferenceParameterTemplate or unknownReferenceParameterTemplate rather than validTemplate5?
Also, how about more detailed description? something like Trial template contains Trial Parameters which weren't referenced from spec.parameters

I don't like the name validTemplate5 either and I agree that invalidReferenceParameterTemplate is better.. but I guess that some rules are applied to mock template names (maybe I misunderstood it?).

Mock templates with name validTemplate* have a valid job, while mock templates with specific names like missedParameterTemplate, invalidParameterTemplate have a invalid job.

So I named it as a validTemplate5 (since it has a valid job)... Please let me know if you still think it's better to name it as invalidReferenceParameterTemplate instead of validTemplate5?

I changed the description as you suggested, thanks :)

Since the validation logic was changed to check only it's empty, maybe we could change the test code :)
And in that case, yes the variable name validTemplate5 would be better.

anencore94 · 2021-11-01T13:34:20Z

/ok-to-test

henrysecond1 · 2021-11-01T16:40:04Z

@anencore94 Thanks for the review! PTAL

andreyvelich

Thank you for driving this @henrysecond1!
I left comment.

andreyvelich · 2021-11-01T17:31:57Z

pkg/webhook/v1beta1/experiment/validator/validator.go

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


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 ?

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

Yes that make sense ! Thanks

…ot empty

henrysecond1 · 2021-11-08T16:34:28Z

@anencore94 @andreyvelich I updated the test code (for the case when spec.parameters is empty). Could you re-check PR? Thanks for your time.

andreyvelich · 2021-11-08T18:44:44Z

Thank you for updating this @henrysecond1!
/lgtm
/assign @gaocegege @johnugeorge

johnugeorge · 2021-11-09T09:30:27Z

/lgtm

gaocegege

/lgtm

google-oss-prow · 2021-11-09T11:08:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege, henrysecond1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gaocegege]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fix: check if parameter references exist in experiment parameters

488a935

aws-kf-ci-bot added the needs-ok-to-test label Oct 29, 2021

google-oss-robot added the size/XS label Oct 29, 2021

google-oss-robot requested review from andreyvelich, gaocegege and sperlingxx October 29, 2021 03:38

Fix validator test

b1a3eac

google-oss-robot added size/S and removed size/XS labels Oct 29, 2021

anencore94 reviewed Nov 1, 2021

View reviewed changes

aws-kf-ci-bot added ok-to-test and removed needs-ok-to-test labels Nov 1, 2021

Update some comments and test descriptions

059cbbf

andreyvelich reviewed Nov 1, 2021

View reviewed changes

henrysecond1 added 2 commits November 3, 2021 02:21

Check trial parameter reference only when experiment parameters are n…

81cf7de

…ot empty

Add a test for the case 'spec.parameters' is mepty

38b1770

google-oss-prow bot added size/M and removed size/S labels Nov 8, 2021

google-oss-prow bot assigned gaocegege, johnugeorge and andreyvelich Nov 8, 2021

google-oss-prow bot added the lgtm label Nov 8, 2021

gaocegege approved these changes Nov 9, 2021

View reviewed changes

google-oss-prow bot added the approved label Nov 9, 2021

google-oss-robot merged commit cffab07 into kubeflow:master Nov 9, 2021

henrysecond1 deleted the fix/wrong-parameter-reference branch November 9, 2021 15:17

alexeygorobets mentioned this pull request Apr 7, 2022

Trial name could not be passed to env #1844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

henrysecond1 commented Oct 29, 2021 •

edited

Loading

aws-kf-ci-bot commented Oct 29, 2021

google-oss-robot commented Oct 29, 2021

coveralls commented Oct 29, 2021 •

edited

Loading

anencore94 left a comment

anencore94 Nov 1, 2021

henrysecond1 Nov 1, 2021

anencore94 Nov 1, 2021

henrysecond1 Nov 1, 2021

anencore94 Nov 1, 2021

henrysecond1 Nov 1, 2021

anencore94 Nov 1, 2021

henrysecond1 Nov 1, 2021 •

edited

Loading

anencore94 Nov 3, 2021

anencore94 commented Nov 1, 2021

henrysecond1 commented Nov 1, 2021

andreyvelich left a comment

andreyvelich Nov 1, 2021

gaocegege Nov 2, 2021

henrysecond1 Nov 2, 2021

anencore94 Nov 3, 2021

henrysecond1 commented Nov 8, 2021

andreyvelich commented Nov 8, 2021

johnugeorge commented Nov 9, 2021

gaocegege left a comment

google-oss-prow bot commented Nov 9, 2021

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

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

Conversation

henrysecond1 commented Oct 29, 2021 • edited Loading

aws-kf-ci-bot commented Oct 29, 2021

google-oss-robot commented Oct 29, 2021

coveralls commented Oct 29, 2021 • edited Loading

anencore94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrysecond1 Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anencore94 commented Nov 1, 2021

henrysecond1 commented Nov 1, 2021

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrysecond1 commented Nov 8, 2021

andreyvelich commented Nov 8, 2021

johnugeorge commented Nov 9, 2021

gaocegege left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Nov 9, 2021

henrysecond1 commented Oct 29, 2021 •

edited

Loading

coveralls commented Oct 29, 2021 •

edited

Loading

henrysecond1 Nov 1, 2021 •

edited

Loading