Add Model Management API #48

YujiOshima · 2018-04-16T15:11:31Z

Close #43
Add Model Management API. Katib will provide store, update Model Info in ModelDB.

Define Model Management API in api.proto
Add ModelDB go client library ( This will solve ModelDB integration issue pointed draft suggestions for better user guide #47 )
Add Model Management CLI

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

gaocegege · 2018-04-17T02:14:11Z

YujiOshima · 2018-04-17T11:47:20Z

@gaocegege
I tried to add Cli for Model management, but the katib-cli code is unrefined.
So I want to merge this PR without cli. Then we will introduce cobra to cli and add Model management command.
WDYT?

I worry about this PR is too big to review.
You can ignore 1st commit (only for vendor pkg) and manager/modelstore_interface/modeldb/ dir.
The files in this dir are generated by Thrift.
Please look

api.proto update
manager/modelstore_interface/modelstore_interface.go
manager/modelstore_interface/modeldb.go.

gaocegege

Generally LGTM. I agree that we could change the code iteratively and no need to implement cli in this PR.

gaocegege · 2018-04-17T12:10:41Z

manager/main.go

 	"google.golang.org/grpc"
 	"google.golang.org/grpc/reflection"

+	pb "github.com/kubeflow/hp-tuning/api"


Could we group the deps in order?

e.g.

built-in dep third party github.com/kubeflow/hp-tuning

gaocegege · 2018-04-17T12:11:42Z

manager/main.go

@@ -109,6 +68,7 @@ func (s *server) trialIteration(conf *pb.StudyConfig, study_id string, sCh study
 		ei = defaultEarlyStopInterval
 	}
 	estm := time.NewTimer(time.Duration(ei) * time.Second)
+	strtm := time.NewTimer(time.Duration(30) * time.Second)


Could we define 30 as a constant.

gaocegege · 2018-04-17T12:14:37Z

manager/modelstore_interface/modeldb.go

+}
+
+func (m *ModelDBInterface) GetStoredModel(in *api.GetStoredModelRequest) (*api.ModelInfo, error) {
+


Please remove the new line here.

gaocegege · 2018-04-17T12:15:08Z

manager/modelstore_interface/modeldb.go

+	return nil
+}
+
+//func main() {


Could we remove the function if it is not used?

gaocegege · 2018-04-17T12:16:16Z

manager/modelstore_interface/modelstore_interface.go

@@ -0,0 +1,13 @@
+package modelstore_interface


I think we could name it as interface and name the package modelstore_interface to modelstore, WDYT

+1

manager/modelstore_interface/modelstore_interface.go -> manager/modelstore/interface.go

gaocegege · 2018-04-17T12:16:44Z

manager/worker_interface/nvdocker/nvidia-docker.go

@@ -10,7 +10,7 @@ import (
 	dclient "github.com/docker/docker/client"
 	"github.com/kubeflow/hp-tuning/api"
 	"github.com/kubeflow/hp-tuning/db"
-	"github.com/kubeflow/hp-tuning/manager/modeldb"
+	//	"github.com/kubeflow/hp-tuning/manager/modeldb"


Could we remove the dep?

gaocegege · 2018-04-17T12:17:05Z

manager/worker_interface/nvdocker/nvidia-docker.go

-				et, _ := time.Parse(time.RFC3339, t.EvalLogs[len(t.EvalLogs)-1].Time)
-				mr.Metrics["time-cost-Min"] = et.Sub(st).Minutes()
-				mif.SendReq(mr)
+				//				mif := modeldb.ModelDbIF{}


Could we remove the code directly?

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

YujiOshima · 2018-04-18T02:24:19Z

@gaocegege Thank you for the review!
I updated it.

gaocegege

lgtm, only nits.

gaocegege · 2018-04-18T02:28:00Z

manager/main.go

@@ -154,7 +153,7 @@ func (s *server) trialIteration(conf *pb.StudyConfig, study_id string, sCh study
 					}
 				}
 			}
-			strtm.Reset(30 * time.Second)
+			strtm.Reset(time.Duration(defaultStoreInterval) * time.Second)


I think we could use defaultStoreInterval directly here.

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

gaocegege · 2018-04-18T02:44:40Z

/lgtm

k8s-ci-robot · 2018-04-18T02:44:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege

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

gaocegege · 2018-04-18T02:44:52Z

Thanks for your great work haha

ddysher · 2018-04-18T02:22:51Z

api/api.proto

@@ -12,6 +12,11 @@ service Manager {
 	rpc GetObjectValue(GetObjectValueRequest) returns (GetObjectValueReply);
 	rpc AddMeasurementToTrials(AddMeasurementToTrialsRequest) returns (AddMeasurementToTrialsReply);
    rpc InitializeSuggestService(InitializeSuggestServiceRequest) returns(InitializeSuggestServiceReply);
+    rpc StoreStudy(StoreStudyRequest) returns(StoreStudyReply);


nit: should we change Store to Save, i.e. SaveStudy, SaveModel, GetSavedStudies, GetSavedModels, etc.

IMO, word store is mostly used as a noun (e.g. StoreInterface),

ddysher · 2018-04-18T02:23:01Z

api/api.proto

@@ -214,6 +219,65 @@ message AddMeasurementToTrialsRequest {
 message AddMeasurementToTrialsReply {
 }

+message StudyOverView {


s/OverView/Overview?

ddysher · 2018-04-18T02:23:33Z

api/api.proto

@@ -214,6 +219,65 @@ message AddMeasurementToTrialsRequest {
 message AddMeasurementToTrialsReply {
 }

+message StudyOverView {
+	string name = 1;
+	string author  = 2;


author or owner?

I will unify owner.

ddysher · 2018-04-18T02:24:31Z

api/api.proto

+}
+
+message DataSetInfo {
+	string Name = 1;


why is this capitalized while others are not?

ddysher · 2018-04-18T02:34:14Z

api/api.proto

@@ -12,6 +12,11 @@ service Manager {
 	rpc GetObjectValue(GetObjectValueRequest) returns (GetObjectValueReply);
 	rpc AddMeasurementToTrials(AddMeasurementToTrialsRequest) returns (AddMeasurementToTrialsReply);
    rpc InitializeSuggestService(InitializeSuggestServiceRequest) returns(InitializeSuggestServiceReply);
+    rpc StoreStudy(StoreStudyRequest) returns(StoreStudyReply);


A little confused here, why do we need to add StoreStudy API here if the PR is about model management API?

ModelDB has two concepts, Project and Model.
You need to create a Project before to save Models.

A Study in Katib is mapped to a Project.
A Trial in Katib is mapped to a Model.

ddysher · 2018-04-18T03:07:22Z

manager/main.go

+					isin := false
+					for _, m := range ret.Models {
+						if m.TrialId == tid {
+							isin = true


can we break early here, since you already set isin to true, there is no need to loop through all models.

BTW, what is the mapping between model and trail, is it 1:1 mapping?

Yes, the model is 1:1 mapping to a trial.
The trial is a set of hyperparameters.

ddysher · 2018-04-18T03:08:39Z

manager/main.go

+						}
+					}
+					met := make([]*pb.Metrics, len(conf.Metrics))
+					for i, mn := range conf.Metrics {


why not put this into isin if block? if isin is false, then operation here is basically no-op.

ddysher · 2018-04-18T03:11:26Z

manager/modelstore_interface/modelstore_interface.go

@@ -0,0 +1,13 @@
+package modelstore_interface


+1

manager/modelstore_interface/modelstore_interface.go -> manager/modelstore/interface.go

ddysher · 2018-04-18T03:12:45Z

manager/modelstore_interface/modeldb.go

+	return &ModelDBInterface{host: host, port: port}
+}
+
+func (m *ModelDBInterface) createSocket() (thrift.TTransport, *modeldb.ModelDBServiceClient, error) {


does model db only supports thrift? Maintaining two messaging format sounds like a painful task ...

Unfortunately, ModelDB is only supporting Thrift..

ddysher · 2018-04-18T03:14:00Z

manager/modelstore_interface/modeldb.go

+
+}
+
+func (m *ModelDBInterface) StoreModel(in *api.StoreModelRequest) error {


can you add documentation for exported methods :)

ddysher · 2018-04-18T03:16:20Z

@YujiOshima @gaocegege looks like there's a race condition :)

Can you take a look at the comments even though it's merged？

YujiOshima · 2018-04-18T03:23:15Z

@ddysher Thank you for comment.
I will open a new PR for refactoring this PR.

YujiOshima added 2 commits April 16, 2018 15:05

update vendor

f85ea44

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

add ModelStore API

94e029f

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

k8s-ci-robot added the do-not-merge/work-in-progress label Apr 16, 2018

k8s-ci-robot requested review from jose5918 and mitake April 16, 2018 15:11

k8s-ci-robot added the size/XXL label Apr 16, 2018

YujiOshima mentioned this pull request Apr 16, 2018

draft suggestions for better user guide #47

Closed

k8s-ci-robot requested review from ddysher and gaocegege April 17, 2018 02:14

YujiOshima changed the title ~~[WIP] Add Model Management API~~ Add Model Management API Apr 17, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress label Apr 17, 2018

gaocegege reviewed Apr 17, 2018

View reviewed changes

refactor

27bd862

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

gaocegege approved these changes Apr 18, 2018

View reviewed changes

refactor

fa5ead3

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

k8s-ci-robot assigned gaocegege Apr 18, 2018

k8s-ci-robot added the lgtm label Apr 18, 2018

k8s-ci-robot added the approved label Apr 18, 2018

k8s-ci-robot merged commit 3ca2df0 into kubeflow:master Apr 18, 2018

ddysher reviewed Apr 18, 2018

View reviewed changes

YujiOshima deleted the modelstoreapi branch April 24, 2018 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Model Management API #48

Add Model Management API #48

YujiOshima commented Apr 16, 2018

gaocegege commented Apr 17, 2018

YujiOshima commented Apr 17, 2018

gaocegege left a comment

gaocegege Apr 17, 2018

gaocegege Apr 17, 2018

gaocegege Apr 17, 2018

gaocegege Apr 17, 2018

gaocegege Apr 17, 2018

ddysher Apr 18, 2018

gaocegege Apr 17, 2018

gaocegege Apr 17, 2018

YujiOshima commented Apr 18, 2018 •

edited

Loading

gaocegege left a comment

gaocegege Apr 18, 2018

gaocegege commented Apr 18, 2018

k8s-ci-robot commented Apr 18, 2018

gaocegege commented Apr 18, 2018

ddysher Apr 18, 2018

YujiOshima Apr 18, 2018

ddysher Apr 18, 2018

ddysher Apr 18, 2018

YujiOshima Apr 18, 2018

ddysher Apr 18, 2018

ddysher Apr 18, 2018

YujiOshima Apr 18, 2018

ddysher Apr 18, 2018

YujiOshima Apr 18, 2018

ddysher Apr 18, 2018

ddysher Apr 18, 2018

ddysher Apr 18, 2018

YujiOshima Apr 18, 2018

ddysher Apr 18, 2018

ddysher commented Apr 18, 2018

YujiOshima commented Apr 18, 2018

		}

		func (m ModelDBInterface) GetStoredModel(in api.GetStoredModelRequest) (*api.ModelInfo, error) {


		}

		func (m ModelDBInterface) StoreModel(in api.StoreModelRequest) error {

Add Model Management API #48

Add Model Management API #48

Conversation

YujiOshima commented Apr 16, 2018

gaocegege commented Apr 17, 2018

YujiOshima commented Apr 17, 2018

gaocegege 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

Choose a reason for hiding this comment

YujiOshima commented Apr 18, 2018 • edited Loading

gaocegege left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaocegege commented Apr 18, 2018

k8s-ci-robot commented Apr 18, 2018

gaocegege commented Apr 18, 2018

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

ddysher commented Apr 18, 2018

YujiOshima commented Apr 18, 2018

YujiOshima commented Apr 18, 2018 •

edited

Loading