add v1alpha2 grpc api #427

YujiOshima · 2019-03-12T17:14:06Z

#423
I added v1alpha2 gRPC API complies with the v1alpha2 CRD.
https://github.com/kubeflow/katib/pull/427/files#diff-c0f6760a929f1056527071edf1f57db8

I will add a new DB schema soon.

@richardsliu @johnugeorge @hougangliu @gaocegege

Signed-off-by: YujiOshima [email protected]

This change is

Signed-off-by: YujiOshima <[email protected]>

pkg/api/v1alpha2/api.proto

Signed-off-by: YujiOshima <[email protected]>

YujiOshima · 2019-03-14T05:17:51Z

I added a new version of DB IF
1a6d4ca

pkg/api/v1alpha2/api.proto

richardsliu · 2019-03-14T20:42:51Z

pkg/api/v1alpha2/api.proto

+    ObjectiveSpec objective = 2; 
+    AlgorithmSpec algorithm = 3;
+    string trial_template = 4;
+    int32 pallarel_trial_count = 5;


Typo - parallel

richardsliu · 2019-03-14T20:46:24Z

pkg/api/v1alpha2/api.proto

+    string value = 2;
+}
+
+message MetricLog {


What is this used for?

richardsliu · 2019-03-14T20:49:05Z

pkg/api/v1alpha2/api.proto

+    repeated Metric metrics = 1;
+}
+
+message ObservationLog {


Document what this is used for.

I added doc here 2e3444d#diff-c0f6760a929f1056527071edf1f57db8R117
Katib will store every log of metrics. Users can look at the history of training. And the logs are used by suggestion algorithm and early stopping.

richardsliu · 2019-03-14T20:51:06Z

pkg/api/v1alpha2/api.proto

+}
+
+message GetExperimentListReply {
+    repeated ExperimentSummary experiment_summarys = 1;


richardsliu · 2019-03-14T20:52:33Z

pkg/db/v1alpha2/db_init.go

+		objective TEXT,
+		algorithm TEXT,
+		trial_template TEXT,
+		pallarel_trial_count INT,


andreyvelich · 2019-03-15T00:13:51Z

pkg/api/v1alpha2/api.proto

+ */
+ message GraphConfig {
+    int32 num_layers = 1; /// Number of layers
+    repeated int32 input_size = 2; ///  Dimenstions of input size


Dimenstions -> Dimensions

andreyvelich · 2019-03-15T00:16:18Z

pkg/api/v1alpha2/api.proto

+
+message ExperimentSummary {
+    string experiment_name = 1;
+    ExperimentStatus.ExperimentConditionType condition = 4;


Need to change 4 to 2 ?

andreyvelich · 2019-03-15T00:20:21Z

pkg/db/v1alpha2/db_init.go

+		start_time DATETIME(6),
+		metrics_collector_type TEXT,
+		completion_time DATETIME(6),
+		last_reconcile_time DATETIME(6))`)


Will we add NasConfig in experiments table in the future?

YujiOshima · 2019-03-15T03:52:51Z

@richardsliu @andreyvelich Thanks! I applied your comments.

Signed-off-by: YujiOshima <[email protected]>

johnugeorge · 2019-03-15T04:05:57Z

pkg/api/v1alpha2/api.proto

+    ObjectiveType type = 1;
+    float goal = 2;
+    string objective_metric_name = 3;
+    repeated string additional_metrics_name = 4; 


additional_metrics_names?

johnugeorge · 2019-03-15T05:12:07Z

pkg/api/v1alpha2/api.proto

+        string value = 2;
+    }
+    string name = 1;
+    repeated ValueLog valuelog = 2;


why would it be a list of [timestamp-value] pair for a particular key? Isn't it, a [key-value] pair for a particular timestamp?

Is this the metric name?

s/valuelog/value_log

johnugeorge · 2019-03-15T05:47:46Z

pkg/api/v1alpha2/api.proto

+    Experiment experiment = 1;
+}
+
+message GetStudyListRequest {


what is this used for?

johnugeorge · 2019-03-15T06:01:38Z

pkg/api/v1alpha2/api.proto

+
+message GetTrialListRequest {
+    string experiment_name = 1;
+    string start_time = 2; ///The start of the time range. RFC3339 format


are these filter options?

johnugeorge · 2019-03-15T06:30:42Z

pkg/api/v1alpha2/api.proto

+message GetSuggestionsRequest {
+    string experiment_name = 1;
+    string algorithm_name = 2;
+    int32 request_number = 3;


what is this field used for?

When you set 3 to request_number, you can get three Suggestions at one time.

Isn't this same as the parallel_trial_count which is already in the experiment Spec ?

a little bit different.
When parallel_trial_count is 4 and 2 trials are completed at the same time, the operator needs to set 2 to request_number in GetSuggesitonRequest.

Makes sense. Thanks

johnugeorge · 2019-03-15T16:57:33Z

pkg/api/v1alpha2/api.proto

+
+message ExperimentSummary {
+    string experiment_name = 1;
+    ExperimentStatus.ExperimentConditionType condition = 2;


Should we add more fields as in ExperimentStatus https://github.com/kubeflow/katib/blob/master/pkg/api/operators/apis/experiment/v1alpha2/types.go#L75
OptimalTrial ,TrialsCompleted,TrialsFailed ,TrialsKilled ,TrialsPending

johnugeorge · 2019-03-15T16:58:27Z

pkg/api/v1alpha2/api.proto

+    }
+    string start_time = 1; /// RFC3339 format
+    string completion_time = 2; /// RFC3339 format
+    string last_reconcile_time = 3; /// RFC3339 format


Do we need last_reconcile_time in DB?

johnugeorge · 2019-03-15T16:58:41Z

pkg/api/v1alpha2/api.proto

+    }
+    string start_time = 1; /// RFC3339 format
+    string completion_time = 2; /// RFC3339 format
+    string last_reconcile_time = 3; /// RFC3339 format


Do we need last_reconcile_time in DB?

johnugeorge · 2019-03-15T17:06:06Z

pkg/api/v1alpha2/api.proto

+}
+
+message Trial {
+    string id = 1;


Is this ID or name(like in experiment)?

Since the trials are generated by the controller, users won't name them.
I think ID is more suitable here.

It is true. But, I feel that it might be better to keep the same terminology as in the Kubernetes config(matadata.name)

make sense. "name" would be more user friendly.

johnugeorge · 2019-03-15T17:08:45Z

pkg/api/v1alpha2/api.proto

+}
+
+message RegisterTrialReply {
+    string trial_id = 1;


DB interface can have multiple storage backend(Mysql, ModelDb etc). Is the controller creating Id? What is the return trial_id

My assumption, it is created by DB backend.
But it is OK the controller create the ID. It will probably be more consistent behavior.

I am not sure if every storage backend supports id creation.

OK. let's make the name of trial is generated by controller.

johnugeorge · 2019-03-15T17:16:40Z

pkg/api/v1alpha2/api.proto

+    /** 
+     * Update Status of a experiment.
+     */
+    rpc UpdateExperimentStatus(UpdateExperimentStatusRequest) returns (UpdateExperimentStatusReply){


Currently, Suggestion algorithms can modify the algorithm parameters. https://github.com/kubeflow/katib/blob/master/pkg/suggestion/grid_service.go#L195

How is this handled?

I will create UpdateAlgorithmParameter API.

hougangliu · 2019-03-18T23:40:39Z

pkg/api/v1alpha2/api.proto

+ * Config for operations in DAG
+ */
+ message Operation {
+     string operationType = 1; /// Type of operation in DAG


s/operationType/operation_type to keep same name style as others

hougangliu · 2019-03-18T23:42:15Z

pkg/api/v1alpha2/api.proto

+        string value = 2;
+    }
+    string name = 1;
+    repeated ValueLog valuelog = 2;


s/valuelog/value_log

hougangliu · 2019-03-18T23:42:49Z

pkg/api/v1alpha2/api.proto

+}
+
+message ObservationLog {
+    repeated MetricLog metriclogs = 1;


s/metriclogs/metric_logs

Signed-off-by: YujiOshima <[email protected]>

YujiOshima · 2019-03-20T05:18:48Z

@johnugeorge @hougangliu Thank you for your comments. I updated.
PTAL.

johnugeorge · 2019-03-20T08:43:26Z

pkg/db/v1alpha2/db_init.go

 		id INT AUTO_INCREMENT PRIMARY KEY,
 		time DATETIME(6),
-		name VARCHAR(255),
+		metric_name VARCHAR(255),
 		value TEXT,


is this metric_value field?

johnugeorge · 2019-03-20T08:59:22Z

@YujiOshima I see that new API UpdateAlgorithmSpec is added. How is experiment Spec update handled in general? Say If user updates fields like MaxTrials, ParallelTrials etc in K8s config , how is DB updated?

YujiOshima · 2019-03-22T02:11:13Z

@johnugeorge You have a point.
The problem is how to sync the AlgorithmSpec between k8s CR and DB.
One idea, the AlgorithmSpec would be an initial config of an algorithm for the experiment and it will not change.
And adding a new table AlgorithmStatus to DB. SuggestionService will save the status of the algorithm with UpdateAlgorithmStatus to DB.
WDYT?

johnugeorge · 2019-03-22T05:56:17Z

@YujiOshima How would AlgorithmStatus structure look ? Is it a key-value pair(similar to AlgorithmSetting) that would be only configured by Algorithms internally?

YujiOshima · 2019-03-22T09:32:41Z

@johnugeorge Yes. Since the parameter that each algorithm need is different, the AlgorithmStatus should have a general struct like key-value pair.

that would be only configured by Algorithms internally?

Yes.

johnugeorge · 2019-03-25T04:10:17Z

@YujiOshima
Few thoughts

What does status mean in `AlgorithmStatus' call? I think, term is slightly confusing.
From what i understood, this rpc call can be used to set extra key-value pairs (Algorithm specific parameters, in addition to the ones set by user in ExperimentSpec). Is it right?

3 . Do we need UpdateAlgorithmSpec call in such a case?

YujiOshima · 2019-03-25T09:06:26Z

@johnugeorge Sorry for confusing you.

From what i understood, this rpc call can be used to set extra key-value pairs (Algorithm specific parameters, in addition to the ones set by user in ExperimentSpec). Is it right?

Right.
We need to store the extra key-value pairs to DB.
As you pointed, we should not use UpdateAlgorithmSpec in this case (I will delete this API since I think we don't need it now).
I called The extra parameters AlgorithmStatus in the previous comment.
My idea is the extra parameters are handled independently from AlgorithmSpec of ExperimentSpec.
When a Suggestion service calls UpdateAlgorithmStatus gRpc API to save the parameters, the Katib-manager save the parameter to DB and it doesn't any effect on Experiments CR.

make sense?

johnugeorge · 2019-03-25T09:51:25Z

@johnugeorge Sorry for confusing you.

From what i understood, this rpc call can be used to set extra key-value pairs (Algorithm specific parameters, in addition to the ones set by user in ExperimentSpec). Is it right?

Right.
We need to store the extra key-value pairs to DB.
As you pointed, we should not use UpdateAlgorithmSpec in this case (I will delete this API since I think we don't need it now).
I called The extra parameters AlgorithmStatus in the previous comment.
My idea is the extra parameters are handled independently from AlgorithmSpec of ExperimentSpec.
When a Suggestion service calls UpdateAlgorithmStatus gRpc API to save the parameters, the Katib-manager save the parameter to DB and it doesn't any effect on Experiments CR.

make sense?

Yes. Should we rename the api UpdateAlgorithmStatus to a better one? Status looks misleading since we are just updating algorithm specific parameters

andreyvelich · 2019-03-25T20:27:49Z

pkg/api/v1alpha2/api.proto

+      /** 
+     * List of ParameterAssignment
+     */
+     message ParameterAssignments {


@YujiOshima @richardsliu I think we need ParameterSpec instead of ParameterAssignments, that I told you. We need Feasible space inside ParameterConfigs in Operations.

OK, I will update.

YujiOshima · 2019-03-26T03:54:04Z

@johnugeorge How about

AlgorithmVariables
AdditionalAlgorithmSetting

Signed-off-by: YujiOshima <[email protected]>

YujiOshima · 2019-03-26T07:11:59Z

@johnugeorge @andreyvelich @richardsliu @hougangliu I think I applied all comments. PTAL.

hougangliu · 2019-03-26T07:15:14Z

/lgtm

johnugeorge · 2019-03-26T08:55:22Z

/retest

YujiOshima · 2019-03-26T09:22:29Z

/retest

johnugeorge · 2019-03-26T13:28:40Z

/retest

johnugeorge · 2019-03-26T14:28:59Z

/retest

johnugeorge · 2019-03-26T16:59:10Z

/retest

johnugeorge · 2019-03-26T17:06:09Z

/lgtm
/approve

k8s-ci-robot · 2019-03-26T17:06:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge

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

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

add v1alpha2 grpc api

2ed4f1f

Signed-off-by: YujiOshima <[email protected]>

k8s-ci-robot added the do-not-merge/work-in-progress label Mar 12, 2019

k8s-ci-robot requested review from garganubhav and libbyandhelen March 12, 2019 17:14

k8s-ci-robot added the size/XXL label Mar 12, 2019

andreyvelich reviewed Mar 12, 2019

View reviewed changes

pkg/api/v1alpha2/api.proto Outdated Show resolved Hide resolved

andreyvelich reviewed Mar 13, 2019

View reviewed changes

pkg/api/v1alpha2/api.proto Show resolved Hide resolved

YujiOshima added 2 commits March 14, 2019 14:04

update gRPC API

8daddec

Signed-off-by: YujiOshima <[email protected]>

add v1alpha2 DB IF

1a6d4ca

Signed-off-by: YujiOshima <[email protected]>

johnugeorge mentioned this pull request Mar 14, 2019

[WIP] Decoupling DB interface from vizier manager #431

Closed

richardsliu reviewed Mar 14, 2019

View reviewed changes

andreyvelich reviewed Mar 15, 2019

View reviewed changes

fix typo, add doc and add todo for nasconfig

2e3444d

Signed-off-by: YujiOshima <[email protected]>

YujiOshima force-pushed the v1-alpha-grpc branch from d939e6d to 2e3444d Compare March 15, 2019 03:56

johnugeorge reviewed Mar 15, 2019

View reviewed changes

pkg/api/v1alpha2/api.proto Outdated

Experiment experiment = 1;

}

message GetStudyListRequest {

Copy link

Member

johnugeorge Mar 15, 2019

Choose a reason for hiding this comment

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

what is this used for?

johnugeorge reviewed Mar 15, 2019

View reviewed changes

hougangliu reviewed Mar 18, 2019

View reviewed changes

apply comments

0fa5bd6

Signed-off-by: YujiOshima <[email protected]>

johnugeorge reviewed Mar 20, 2019

View reviewed changes

YujiOshima mentioned this pull request Mar 22, 2019

Python migration #435

Closed

andreyvelich reviewed Mar 25, 2019

View reviewed changes

YujiOshima added 2 commits March 26, 2019 14:37

update proto

09704ad

Signed-off-by: YujiOshima <[email protected]>

update

a726b67

Signed-off-by: YujiOshima <[email protected]>

YujiOshima changed the title ~~[WIP] add v1alpha2 grpc api~~ add v1alpha2 grpc api Mar 26, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress label Mar 26, 2019

k8s-ci-robot assigned hougangliu Mar 26, 2019

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

k8s-ci-robot assigned johnugeorge Mar 26, 2019

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

k8s-ci-robot merged commit 1316bad into kubeflow:master Mar 26, 2019

add v1alpha2 grpc api #427

add v1alpha2 grpc api #427

Conversation

YujiOshima commented Mar 12, 2019 • edited Loading

YujiOshima commented Mar 14, 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

YujiOshima commented Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

johnugeorge Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

johnugeorge Mar 15, 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

johnugeorge Mar 25, 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

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

YujiOshima commented Mar 20, 2019

Choose a reason for hiding this comment

johnugeorge commented Mar 20, 2019

YujiOshima commented Mar 22, 2019

johnugeorge commented Mar 22, 2019

YujiOshima commented Mar 22, 2019

johnugeorge commented Mar 25, 2019

YujiOshima commented Mar 25, 2019

johnugeorge commented Mar 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YujiOshima commented Mar 26, 2019

YujiOshima commented Mar 26, 2019

hougangliu commented Mar 26, 2019

johnugeorge commented Mar 26, 2019

YujiOshima commented Mar 26, 2019

johnugeorge commented Mar 26, 2019

johnugeorge commented Mar 26, 2019

johnugeorge commented Mar 26, 2019

johnugeorge commented Mar 26, 2019

k8s-ci-robot commented Mar 26, 2019

YujiOshima commented Mar 12, 2019 •

edited

Loading

YujiOshima commented Mar 15, 2019 •

edited

Loading

johnugeorge Mar 15, 2019 •

edited

Loading

johnugeorge Mar 15, 2019 •

edited

Loading

johnugeorge Mar 25, 2019 •

edited

Loading