Add validation for NAS job in Katib controller #398

andreyvelich · 2019-02-20T23:55:01Z

Fixes: #372.
As well, I added validation for HP job.
/assign @hougangliu @YujiOshima

This change is

hougangliu · 2019-02-21T00:04:49Z

pkg/controller/studyjob/utils.go

+		return fmt.Errorf("Missing OutputSize in NasConfig.GraphConfig: %v", instance.Spec.NasConfig.GraphConfig)
+	}
+
+	if instance.Spec.NasConfig.GraphConfig.NumLayers == 0 {


NumLayers is not always in need for all NAS (ENAS need it). I think we need validate NAS config considering suggestion algorithm.
Please also verify if other parameters are general or only ENAS-specified

@hougangliu I think we can check these parameters exactly for Reinforcement Learning algorithm for now. For NumLayers and Step here:

katib/pkg/controller/studyjob/utils.go

Line 287 in d407be8

if jobType == jobTypeNAS && pc.Feasible.Step == "" && pc.ParameterType == katibv1alpha1.ParameterTypeDouble {

, we should validate only for RL algorithm.
What do you think about it?

…e-nasconfig

andreyvelich · 2019-02-21T03:05:14Z

/retest

YujiOshima · 2019-02-21T03:39:38Z

pkg/controller/studyjob/utils.go

+		if reflect.DeepEqual(pc.Feasible, katibv1alpha1.FeasibleSpace{}) {
+			return fmt.Errorf("Missing Feasible in ParameterConfig: %v", pc)
+		}
+		if pc.ParameterType == katibv1alpha1.ParameterTypeCategorical {


Not only categorical but discrete.

In discrete we also use only List structure or something else?

Currently discrete only need List. But we may need more type for discretes, int or double.
Please treated in the same way as Categorical for now.

hougangliu · 2019-02-21T05:09:06Z

pkg/controller/studyjob/katib_api_util.go

+			return validJobErr
+		}
+	}
+


move validateHPJob and validateNASJob into validateStudy, and move validateStudy away from initializeStudy

You want to put validateStudy function before initializeStudy?
Right here: https://github.com/kubeflow/katib/blob/master/pkg/controller/studyjob/studyjob_controller.go#L250 ?

no, validateStudy has been moved to https://github.com/kubeflow/katib/blob/master/pkg/controller/studyjob/validating_webhook.go#L44

hougangliu · 2019-02-21T05:11:02Z

pkg/controller/studyjob/utils.go

+		return err
+	}
+	return nil
+}


return validateParameterConfigs to replace line 215-219

hougangliu · 2019-02-21T05:18:28Z

pkg/controller/studyjob/katib_api_util.go

+		return validErr
+	}
+
+	if getJobType(instance) != jobTypeNAS {


please use suggestionSpec.suggestionAlgorithm to return jobTypeNAS or jobTypeHP in getJobType to handle the invalid studyjob case: .Spec.NasConfig is not nil and suggestionSpec.suggestionAlgorithm is not nas-like

hougangliu · 2019-02-21T05:22:00Z

pkg/controller/studyjob/const.go

+	PyTorchJobWorker      = "PyTorchJob"
+	jobTypeNAS            = "NAS"
+	jobTypeHP             = "HP"
+	suggestionAlgorithmRL = "nasrl"


s/suggestionAlgorithmRL/suggestionNASRL

hougangliu · 2019-02-21T05:25:11Z

pkg/controller/studyjob/utils.go

+				return fmt.Errorf("Missing Max in ParameterConfig.Feasible: %v", pc.Feasible)
+			}
+			if jobType == jobTypeNAS && pc.Feasible.Step == "" && pc.ParameterType == katibv1alpha1.ParameterTypeDouble && suggestionAlgorithm == suggestionAlgorithmRL {
+				return fmt.Errorf("Missing Step in ParameterConfig.Feasible for NAS job: %v", pc.Feasible)


define a suggestionNASList (now only nasrl is in)
s/suggestionAlgorithm == suggestionAlgorithmRL/contains(suggestionNASList, suggestionAlgorithm)
so that when we add other nas suggestion, we only need update suggestionNASList

Same here. Better way to handle this?

Change GetJobType function Make a list with NAS algorithms

andreyvelich · 2019-02-21T22:16:44Z

@hougangliu Thank you! I changed it.

hougangliu · 2019-02-22T00:12:21Z

pkg/controller/studyjob/utils.go

@@ -237,7 +251,7 @@ func validateNASJob(instance *katibv1alpha1.StudyJob) error {
 		return fmt.Errorf("Missing OutputSize in NasConfig.GraphConfig: %v", instance.Spec.NasConfig.GraphConfig)
 	}

-	if instance.Spec.NasConfig.GraphConfig.NumLayers == 0 && instance.Spec.SuggestionSpec.SuggestionAlgorithm == suggestionAlgorithmRL {
+	if instance.Spec.NasConfig.GraphConfig.NumLayers == 0 && contains(suggestionNASList, instance.Spec.SuggestionSpec.SuggestionAlgorithm) {


I think line 298 can use contains(suggestionNASList,..), but here I don't think so.
If we add another nas suggestion, it may not need NumLayers.

@hougangliu You think we should specify this block only for suggestionNASRL ?

Is there a better place to put algorithm specific checks other than the controller? This list can grow over time with more algorithms and these checks are very specific to the algorithm implementation. @andreyvelich @hougangliu Thoughts?

From my point of view, it is important to check it in the controller. We don't need to run getSuggestion if something wrong with yaml file. We can't set failed condition for the StudyJob inside Suggestion. Controller do that.

We should do algorithm-specific check in suggestion service.

Add ValidateSuggestionParameter API to Suggestion service
or

GetSuggesiton returns a specific error code if the config is invalid then the controller handle the error code

@andreyvelich @johnugeorge @hougangliu Thoughts?

@YujiOshima In that case, should we send StudyConfig to ValidateSuggestionParameters, because we don't save StudyConfig in db, if it is not valid.
We can't GetStudy like here: https://github.com/kubeflow/katib/blob/master/pkg/suggestion/nasrl_service.py#L219 ?
As well, should we put this function after this line: https://github.com/kubeflow/katib/blob/master/pkg/controller/studyjob/katib_api_util.go#L43 ?
We generate StudyConfig and can send it to Suggestion to check, whether it is valid or not.

@andreyvelich You're right. StudyConf and SuggestionParams should be included in an args of ValidateSuggestionParameters.

@YujiOshima Ok. In this PR, should I make a validation only for NAS RL service, or we should include BO here, as well?

@andreyvelich Only for NAS RL is enough here. If you could add validation for BO in another PR, it would be fantastic.

@YujiOshima @hougangliu What do you think about name of this API: ValidateStudyParameters ?
It looks confused with ValidateSuggestionParameters since we validate StudyConfig and SuggestionParameters in this function

johnugeorge · 2019-02-22T03:40:08Z

pkg/controller/studyjob/const.go

 )

 var ValidWorkerKindList = [...]string{DefaultJobWorker, TFJobWorker, PyTorchJobWorker}
+
+var suggestionNASList = []string{suggestionNASRL}


Our goal is to keep controller independent of worker type and suggestion algos. Currently there are no worker specific code in the code(except for watch. Hope, it can be fixed soon and ValidWorkerKindList won't not needed any more). Similarly i feel that there should not be any suggestion algorithm specific changes in the controller code.

In that case, how we can make a validation for the NAS job ?

andreyvelich · 2019-02-27T03:49:48Z

Validation in NAS RL service is not ready
/hold

andreyvelich · 2019-02-28T04:34:54Z

I created a new API function called ValidateSuggestionParameters and added validation inside NAS RL service, as noticed here #398 (comment).
Right now, we can add this function inside Suggestion service and validate StudyJob.
Logic works like this:

StudyJobController -> vizier-core-manager -> Suggestion service

Case1: Suggestion can't reply (Suggestion doesn't have ValidateSuggestionParameters function)

StudyJobController (Request)-> vizier-core-manager  (Request)->x Failed Suggestion service
StudyJobController <-(Reply{}, error code Unknown) vizier-core-manager - Suggestion service
instance.Status.Condition = katibv1alpha1.ConditionRunning

Case2: StudyJob is Valid

StudyJobController (Request)-> vizier-core-manager  (Request)-> Suggestion service
StudyJobController <-(Reply{}, nil) vizier-core-manager <-(Reply{}, nil) Suggestion service
instance.Status.Condition = katibv1alpha1.ConditionRunning

Case3: StudyJob is Invalid

StudyJobController (Request)-> vizier-core-manager  (Request)-> Suggestion service
StudyJobController <-(Reply{}, error code InvalidArgument + error message from Suggestion) vizier-core-manager <-(Reply{}, error code InvalidArgument + error message from Suggestion) Suggestion service
instance.Status.Condition = katibv1alpha1.ConditionFailed

@YujiOshima @hougangliu @johnugeorge @richardsliu @DeeperMind Please, check it.

/hold cancel

…itten in GO

andreyvelich · 2019-02-28T05:10:14Z

/test kubeflow-katib-presubmit

hougangliu · 2019-02-28T05:16:02Z

pkg/controller/studyjob/katib_api_util.go

 		return err
 	}
+
+	log.Println("Validate Parameters inside Suggestion Start")


Start to validate Suggestion Parameters

hougangliu · 2019-02-28T05:19:40Z

pkg/controller/studyjob/katib_api_util.go

+	} else {
+		instance.Status.Condition = katibv1alpha1.ConditionFailed
+		return errors.New("Suggestion Parameters is not valid")
+	}


/s/is/are
or Valid suggestion Parameters and Invalid suggestion Parameters

hougangliu · 2019-02-28T05:29:50Z

pkg/api/api.proto

+ * Return 1 if Suggestion Parameters is Valid, 0 if not
+ */
+message ValidateSuggestionParametersReply {
+}


bool is_valid 1;
string msg 2;

andreyvelich · 2019-02-28T07:08:00Z

/test kubeflow-katib-presubmit

andreyvelich · 2019-02-28T07:45:27Z

/retest

andreyvelich · 2019-02-28T08:34:27Z

/retest

andreyvelich · 2019-02-28T09:28:11Z

/retest

andreyvelich · 2019-02-28T18:39:45Z

/retest

andreyvelich · 2019-02-28T23:29:52Z

@jlewi @YujiOshima @hougangliu @richardsliu
Should we switch test server to us-east1-c since us-east1-d has VM shortage?
Like here: kubeflow/kubeflow#2583

andreyvelich · 2019-03-01T02:13:50Z

/retest

andreyvelich · 2019-03-01T03:44:29Z

/retest

YujiOshima · 2019-03-04T04:25:38Z

pkg/api/api.proto

@@ -212,10 +212,20 @@ service Manager {
            get: "/api/Manager/GetSavedModels"
        };
    };
+    /** 
+     * Validate Suggestion Parameters from Study Job.


Please explain a suggestion service should return INVALID_ARGUMENT error when the parameter is invalid.

YujiOshima · 2019-03-04T04:29:45Z

cmd/manager/main.go

+	}
+
+	conn, err := grpc.Dial("vizier-suggestion-"+in.SuggestionAlgorithm+":6789", grpc.WithInsecure())
+	if err != nil {


It should return specific error code here (e.g. Unavailable ) and set message.

@YujiOshima I think we should delete this part:
https://github.com/kubeflow/katib/pull/398/files/ea896ae1a4c4690ef8873ce416a019e782710f9e#diff-9e63f7b0791edcb4a53c944552b88970R119.
Since you add "random" algorithm, if it is not specified:
https://github.com/kubeflow/katib/blob/master/pkg/controller/studyjob/katib_api_util.go#L28

@YujiOshima By default this Error https://github.com/kubeflow/katib/pull/398/files#diff-9e63f7b0791edcb4a53c944552b88970R120 will return Unavailable code. I think, we don't need to specify this error code there.
Right now, I handle this error here, as you said: b88030b#diff-7a381fdaeae1295724b74c0a04f7644cR386.
Please check, if it is ok.

YujiOshima · 2019-03-04T04:35:07Z

pkg/controller/studyjob/katib_api_util.go

+	if statusCode.Code() == codes.Unknown {
+		log.Printf("Method ValidateSuggestionParameters not found inside Suggestion service: %s", suggestionAlgorithm)
+		return true
+	}


Please handle InvalidArgument and Unavailable error and print the reason for the error (the parameter is invalid or service is unavailable).

hougangliu · 2019-03-04T23:32:14Z

pkg/api/api.proto

+     * Suggestion service should return INVALID_ARGUMENT Error when the parameter is invalid
+     */
+     rpc ValidateSuggestionParameters(ValidateSuggestionParametersRequest) returns (ValidateSuggestionParametersReply){
+        option (google.api.http) = {


I find that ValidateSuggestionParametersReply is used as return &api.ValidateSuggestionParametersReply{} all the time. can we drop ValidateSuggestionParametersReply, alway return nothing?
or you can use below struct to replace current one

message ValidateSuggestionParametersReply { bool is_valid 1; string msg 2; }

@hougangliu We don't need to return anything, like here:
https://github.com/kubeflow/katib/blob/master/cmd/manager/main.go#L213.
If parameter is not valid, we return INVALID_ARGUMENT Error message.

andreyvelich · 2019-03-05T22:37:19Z

@YujiOshima @hougangliu Do you have any other suggestions on this PR?

hougangliu · 2019-03-05T23:47:18Z

/lgtm
/approve
Thanks!

k8s-ci-robot · 2019-03-05T23:47:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hougangliu

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~~ [hougangliu]

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

andreyvelich added 3 commits February 19, 2019 20:57

Initial commit

9f9a99a

Add validation for NAS config

0b171ab

Fix validation

46221b2

k8s-ci-robot assigned hougangliu and YujiOshima Feb 20, 2019

k8s-ci-robot requested review from jose5918 and texasmichelle February 20, 2019 23:55

k8s-ci-robot added the size/L label Feb 20, 2019

Fix conflicts

d407be8

hougangliu reviewed Feb 21, 2019

View reviewed changes

andreyvelich added 2 commits February 20, 2019 18:17

Add algorithmType in NasConfig validation

c5c284f

Merge remote-tracking branch 'upstream/master' into issue-372-validat…

14cc8be

…e-nasconfig

YujiOshima reviewed Feb 21, 2019

View reviewed changes

Add Discrete ParameterType to validation

36958e0

hougangliu reviewed Feb 21, 2019

View reviewed changes

Move validation to webhook

76f8397

Change GetJobType function Make a list with NAS algorithms

hougangliu reviewed Feb 22, 2019

View reviewed changes

johnugeorge reviewed Feb 22, 2019

View reviewed changes

Add ValidateSuggestionParameters function in Katib API

c3e54cb

k8s-ci-robot added size/XXL and removed size/L labels Feb 27, 2019

k8s-ci-robot added the do-not-merge/hold label Feb 27, 2019

Fetch from upstream

345d66b

k8s-ci-robot added size/XL and removed size/XXL labels Feb 27, 2019

Fix api

e45216e

k8s-ci-robot removed the size/XL label Feb 27, 2019

andreyvelich added 5 commits February 26, 2019 21:22

Add ValidateSuggestionParameters to Suggestion service

3dc4f3e

Change isValid to int32

4b47650

Create Validation function in NAS RL Suggestion service

3a7e0b0

Fix small problems

c16690e

Reduce code inside Validation function

30e876e

k8s-ci-robot removed the do-not-merge/hold label Feb 28, 2019

Add empty ValidateSuggestionParameters function in each HP service wr…

38febad

…itten in GO

hougangliu reviewed Feb 28, 2019

View reviewed changes

Fix logging

8a01094

Add ValidateSuggestionParameters to mock

ea896ae

YujiOshima reviewed Mar 4, 2019

View reviewed changes

Handle Unvailable error

b88030b

hougangliu reviewed Mar 4, 2019

View reviewed changes

k8s-ci-robot added the lgtm label Mar 5, 2019

k8s-ci-robot added the approved label Mar 5, 2019

k8s-ci-robot merged commit 8f89ad4 into kubeflow:master Mar 5, 2019

andreyvelich deleted the issue-372-validate-nasconfig branch September 27, 2021 20:39

Add validation for NAS job in Katib controller #398

Add validation for NAS job in Katib controller #398

Conversation

andreyvelich commented Feb 20, 2019 • edited by jlewi Loading

hougangliu Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

andreyvelich Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

andreyvelich commented Feb 21, 2019

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

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

andreyvelich commented Feb 21, 2019

Choose a reason for hiding this comment

andreyvelich Feb 22, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Feb 27, 2019

andreyvelich commented Feb 28, 2019 • edited Loading

andreyvelich commented Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Feb 28, 2019

andreyvelich commented Feb 28, 2019

andreyvelich commented Feb 28, 2019

andreyvelich commented Feb 28, 2019

andreyvelich commented Feb 28, 2019

andreyvelich commented Feb 28, 2019

andreyvelich commented Mar 1, 2019

andreyvelich commented Mar 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich Mar 4, 2019 • edited Loading

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

andreyvelich Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

andreyvelich commented Mar 5, 2019

hougangliu commented Mar 5, 2019

k8s-ci-robot commented Mar 5, 2019

andreyvelich commented Feb 20, 2019 •

edited by jlewi

Loading

hougangliu Feb 21, 2019 •

edited

Loading

andreyvelich Feb 21, 2019 •

edited

Loading

andreyvelich Feb 22, 2019 •

edited

Loading

andreyvelich commented Feb 28, 2019 •

edited

Loading

andreyvelich commented Feb 28, 2019 •

edited

Loading

andreyvelich Mar 4, 2019 •

edited

Loading

andreyvelich Mar 4, 2019 •

edited

Loading