DB: add studyjob table and extend worker table #362

YujiOshima · 2019-02-07T08:14:16Z

Related to #352 (comment)
I split changes related to DB scheme from #352 .
In this PR, information of studyjob and worker are stored in DB and users can refer them after the studyjob CR is deleted.
It makes easier to rerun the workers since the manifests of them are stored in DB.

@hougangliu @gaocegege @richardsliu @johnugeorge

Signed-off-by: YujiOshima [email protected]

This change is

k8s-ci-robot · 2019-02-07T08:14:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: yujioshima

If they are not already assigned, you can assign the PR to them by writing /assign @yujioshima in a comment when ready.

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

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

YujiOshima · 2019-02-12T01:12:10Z

/retest

googlebot · 2019-02-13T08:09:57Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2019-02-13T08:12:20Z

CLAs look good, thanks!

johnugeorge · 2019-02-13T10:47:53Z

Needs rebase to get gcloud fix

YujiOshima · 2019-02-14T07:10:13Z

CI passed!
@johnugeorge @hougangliu @richardsliu Could you take a look?

YujiOshima · 2019-02-21T05:52:13Z

/retest

hougangliu · 2019-02-21T06:47:26Z

pkg/api/api.proto

    State status = 5; /// Status of Worker.
-    string TemplatePath = 6; /// Path for the manufest template of Worker.
-    repeated Tag tags = 7; /// Tags of Worker.
+    string manufest = 6; /// A json formatted manufest for Worker.


hougangliu · 2019-02-21T06:48:04Z

pkg/api/api.proto

-    string TemplatePath = 6; /// Path for the manufest template of Worker.
-    repeated Tag tags = 7; /// Tags of Worker.
+    string manufest = 6; /// A json formatted manufest for Worker.
+    string metrics_collector_manufest = 7; /// A json formatted manufest for MetricsCollector.


hougangliu · 2019-02-21T06:53:10Z

pkg/controller/studyjob/studyjob_controller.go

@@ -248,7 +244,7 @@ func (r *ReconcileStudyJobController) Reconcile(request reconcile.Request) (reco
 	default:
 		now := metav1.Now()
 		instance.Status.StartTime = &now
-		err = initializeStudy(instance)
+		update, err = initializeStudy(instance, request.Namespace)


I think maybe it's not your change, just merge by mistake?

hougangliu · 2019-02-21T06:55:07Z

pkg/db/db_init.go

@@ -53,8 +62,11 @@ func (d *dbConn) DBInit() {
 		trial_id CHAR(16),
 		type VARCHAR(255),
 		status TINYINT,
-		template_path TEXT,
+		manufest TEXT,
+		metrics_collector_manufest TEXT,


hougangliu · 2019-02-21T06:58:24Z

pkg/db/db_init.go

+		study_id CHAR(16),
+		job_type TEXT,
+		worker_template TEXT,
+		metrics_collector_template TEXT,


I think we'd better choose one of manifest and template to name var/column related consistently

hougangliu · 2019-02-21T07:01:18Z

pkg/controller/studyjob/studyjob_controller.go

@@ -226,11 +227,6 @@ func (r *ReconcileStudyJobController) Reconcile(request reconcile.Request) (reco
 		if !contains(pendingFinalizers, cleanDataFinalizer) {
 			return reconcile.Result{}, nil
 		}
-		err = deleteStudy(instance)


I think maybe we can add a field like "Retain" for studyjob to decide if deleteStudy when delete studyjob

hougangliu · 2019-02-21T07:06:58Z

pkg/controller/studyjob/katib_api_util.go

 	}
 	defer conn.Close()
 	c := katibapi.NewManagerClient(conn)
+	_, err = c.GetStudyJob(context.Background(), &katibapi.GetStudyJobRequest{
+		StudyJobUid: string(instance.UID),
+	})


in which case instance.UID can be duplicated?

By CR operator implementation, the reconcile function will be called parallelly. So initializeStudy is often called two times. I don't know how to avoid it.

hougangliu · 2019-02-21T07:10:40Z

pkg/controller/studyjob/katib_api_util.go

+			if mt, ok := mtl[instance.Spec.MetricsCollectorSpec.GoTemplate.TemplatePath]; ok {
+				regStudyJobreq.StudyJob.MetricsCollectorTemplate = mt
+			}
+		}


please also handle GoTemplate.TemplatePath == "" && GoTemplate.RawTemplate == "" case both for worker and metrics.

In that case, the defaultWorkerTemplate.yaml and defaultMetricsCollectorTemplate.yaml will be used. It's not an error.

hougangliu · 2019-02-21T07:30:24Z

tables studyjobs stores workertemplate/metricscollectortemplate info.
table study stores other info except workertemplate/metricscollectortemplate.

@YujiOshima do you plan to support below configuration?

spec:
   reusestudyid: STUDYID
   reusestudyjobid: UUID

YujiOshima · 2019-02-21T07:55:43Z

@hougangliu Thank you for your review!
I will add reusestudyid. But can I reuse studyjobid? The studyjobid is a UID of a studyjob CR.
It is automatically filled by k8s.

johnugeorge · 2019-02-21T08:05:44Z

pkg/db/interface_test.go

@@ -341,12 +353,35 @@ func TestGetWorkerList(t *testing.T) {
 }

 func TestUpdateWorker(t *testing.T) {
-	mock.ExpectExec(`UPDATE workers SET status = \? WHERE id = \?`).WithArgs(api.State_COMPLETED, defaultWorkerID).WillReturnResult(sqlmock.NewResult(1, 1))
-	err := dbInterface.UpdateWorker(defaultWorkerID, api.State_COMPLETED)
+	wmanufest := "woker manufest"


typo: manufest -> manifest, woker ->worker

johnugeorge · 2019-02-21T08:06:28Z

pkg/db/interface_test.go

-	mock.ExpectExec(`UPDATE workers SET status = \? WHERE id = \?`).WithArgs(api.State_COMPLETED, defaultWorkerID).WillReturnResult(sqlmock.NewResult(1, 1))
-	err := dbInterface.UpdateWorker(defaultWorkerID, api.State_COMPLETED)
+	wmanufest := "woker manufest"
+	mcmanufest := "metricscollector manufest"


Same. Please correct the other lines too

hougangliu · 2019-02-21T08:13:24Z

@hougangliu Thank you for your review!
I will add reusestudyid. But can I reuse studyjobid? The studyjobid is a UID of a studyjob CR.
It is automatically filled by k8s.

Do you mean when a new studyjob uses reusestudyid, it will use configuration of row from studys tables and worker/metrics of row from study jobs tables whose studyid == reusestudyid?
is there one record or multiple records in studyjobs table which studyid == reusestudyid? if there is many, if worker/metrics template keeps same or not?

YujiOshima · 2019-02-21T08:39:42Z

@hougangliu Sorry for confusing.
Yes, I will add

spec:
   reusestudyid: STUDYID

But I don't have plan to add reusestudyjobid: UUID since studyjobid cannot be reused.

is there one record or multiple records in studyjobs table which studyid == reusestudyid? if there is many, if worker/metrics template keeps same or not?

Multiple studyjob for a studyid and worker/metrics template do not need to be the same for each studyjobs even they have the same studyid.

hougangliu · 2019-02-21T08:53:57Z

But I don't have plan to add reusestudyjobid: UUID since studyjobid cannot be reused.

studyjob has been stored in db, why we cannot reuse it? (we create a studyjob crd whose uid is xxx, a studyjob with studyjobid xxx also created in db, when we delete studyjob with uid xx, a studyjob with studyjobid xxx still stored in db.)

Multiple studyjob for a studyid and worker/metrics template do not need to be the same for each studyjobs even they have the same studyid.

I think maybe you plan to use it as below, which can lead to multiple studyjob for a studyid, right?
if yes, we can only reuse a studyjob spec without worker/metrics spec. That is, even we store studyjob in db, we cannot reuse it.

spec:
   reusestudyid: STUDYID
   workerSpec:
     ...
   metricsCollectorSpec:
     ...

YujiOshima · 2019-02-22T06:20:31Z

@hougangliu

studyjob has been stored in db, why we cannot reuse it?
Oh, I misunderstood. We can reuse a config for a studyjob but a UID will change in the new studyjob.

My assumption is below.

Reuse StudyID: Run another config(algorithm, worker template) for an existing study. Only reuse studyID and studyconfig.
Reuse StudyJobID: Rerun a studyjob with the same config (same algorithm, same worker template)

hougangliu · 2019-02-22T08:22:28Z

@YujiOshima I think the change is a little complex. To avoid misunderstanding, I think a detail example can make it clear.
saying: there are existing studyjob s1 and s2 (what it looks config and worker/metrics, simple one is ok). when create a new studyjob s3 with reusestudy/reusestudyjob, what s3 will look like.
Thanks!

YujiOshima · 2019-02-28T08:55:17Z

@hougangliu

You have a original studyjob yaml.
https://gist.github.com/YujiOshima/ac143f0d334759e8a812136d607d6cad#original-job-yaml
and you can create original CR kubectl create -f original-job.yaml
https://gist.github.com/YujiOshima/ac143f0d334759e8a812136d607d6cad#original-cr

When you reuse StudyID, you can omit StudyConf but need to write other info e.g. WorkerSpec.
https://gist.github.com/YujiOshima/ac143f0d334759e8a812136d607d6cad#reuse-studyid-yaml
You can reused StudyConf like this.
https://gist.github.com/YujiOshima/ac143f0d334759e8a812136d607d6cad#reuse-studyid-cr

When you reuse StudyJobID, you can omit almost all field.
https://gist.github.com/YujiOshima/ac143f0d334759e8a812136d607d6cad#reuse-studyjobid-yaml
You will get job copy of the original.
https://gist.github.com/YujiOshima/ac143f0d334759e8a812136d607d6cad#reuse-studyjobid-cr

hougangliu · 2019-03-01T00:19:48Z

@YujiOshima it makes sense.
Then I think there is only one studyjob for a studyid (one to one). Otherwise, for below case, it's hard to decide selecting one of studyjob to fill workerSpec automatically.

apiVersion: "kubeflow.org/v1alpha1"
kind: StudyJob
metadata:
  namespace: kubeflow
  labels:
    controller-tools.k8s.io: "1.0"
  name: random-example-reusejob
spec:
  reuseStudyJobId: f4a4b946-3b33-11e9-adcb-08002750630b

YujiOshima · 2019-03-04T02:32:08Z

@hougangliu

Then I think there is only one studyjob for a studyid (one to one).

No, multi studyjobs can use a studyID and the studyjobs share historical data of Trial and Worker belong to the studyID.
The use cases are

You can get initial values for bayesian opt, running random or manual studyjob. After that, run bayesian opt job reusing the StudyID.
You can retry or try another algorithm after a job is completed and see all results on the same graph in UI.
You can add a trial manually during job running since the algorithm doesn't search near the trial.

Otherwise, for below case, it's hard to decide selecting one of studyjob to fill workerSpec automatically.

A studyjob has only one WorkerSpec. So it can fill with the WorkerSpec.

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

gaocegege · 2019-09-29T03:52:35Z

/close

We have dropped v1alpha1. Thus I think we cannot merge it into master.

k8s-ci-robot · 2019-09-29T03:52:36Z

@gaocegege: Closed this PR.

In response to this:

/close

We have dropped v1alpha1. Thus I think we cannot merge it into master.

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.

k8s-ci-robot requested review from jose5918 and libbyandhelen February 7, 2019 08:14

k8s-ci-robot added the size/XXL label Feb 7, 2019

YujiOshima force-pushed the enrich_workerinfo branch from 4e13574 to 04dc869 Compare February 7, 2019 08:15

YujiOshima mentioned this pull request Feb 7, 2019

Manual suggest #352

Closed

YujiOshima force-pushed the enrich_workerinfo branch from 04dc869 to 258491a Compare February 12, 2019 02:01

johnugeorge mentioned this pull request Feb 12, 2019

StudyJob v1alpha2 API version #370

Closed

15 tasks

YujiOshima force-pushed the enrich_workerinfo branch from 258491a to b749fde Compare February 13, 2019 08:09

YujiOshima force-pushed the enrich_workerinfo branch from b749fde to 04dc869 Compare February 13, 2019 08:12

YujiOshima force-pushed the enrich_workerinfo branch 2 times, most recently from f8e5fa0 to 819b32f Compare February 14, 2019 06:01

YujiOshima force-pushed the enrich_workerinfo branch from 819b32f to e66498c Compare February 21, 2019 04:31

hougangliu reviewed Feb 21, 2019

View reviewed changes

johnugeorge reviewed Feb 21, 2019

View reviewed changes

YujiOshima added 2 commits March 4, 2019 13:52

DB: add studyjob table and extend worker table

1874dec

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

apply comments

be70b10

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

YujiOshima force-pushed the enrich_workerinfo branch from 4bf8e0d to be70b10 Compare March 4, 2019 09:20

k8s-ci-robot closed this Sep 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DB: add studyjob table and extend worker table #362

DB: add studyjob table and extend worker table #362

YujiOshima commented Feb 7, 2019 •

edited by jlewi

Loading

k8s-ci-robot commented Feb 7, 2019

YujiOshima commented Feb 12, 2019

googlebot commented Feb 13, 2019

googlebot commented Feb 13, 2019

johnugeorge commented Feb 13, 2019

YujiOshima commented Feb 14, 2019 •

edited

Loading

YujiOshima commented Feb 21, 2019

hougangliu Feb 21, 2019

hougangliu Feb 21, 2019

hougangliu Feb 21, 2019

hougangliu Feb 21, 2019

hougangliu Feb 21, 2019

hougangliu Feb 21, 2019

hougangliu Feb 21, 2019

YujiOshima Feb 22, 2019

hougangliu Feb 21, 2019

YujiOshima Feb 22, 2019

hougangliu commented Feb 21, 2019 •

edited

Loading

YujiOshima commented Feb 21, 2019

johnugeorge Feb 21, 2019

johnugeorge Feb 21, 2019

hougangliu commented Feb 21, 2019

YujiOshima commented Feb 21, 2019

hougangliu commented Feb 21, 2019

YujiOshima commented Feb 22, 2019

hougangliu commented Feb 22, 2019

YujiOshima commented Feb 28, 2019

hougangliu commented Mar 1, 2019

YujiOshima commented Mar 4, 2019 •

edited

Loading

gaocegege commented Sep 29, 2019

k8s-ci-robot commented Sep 29, 2019

DB: add studyjob table and extend worker table #362

DB: add studyjob table and extend worker table #362

Conversation

YujiOshima commented Feb 7, 2019 • edited by jlewi Loading

k8s-ci-robot commented Feb 7, 2019

YujiOshima commented Feb 12, 2019

googlebot commented Feb 13, 2019

googlebot commented Feb 13, 2019

johnugeorge commented Feb 13, 2019

YujiOshima commented Feb 14, 2019 • edited Loading

YujiOshima 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

hougangliu commented Feb 21, 2019 • edited Loading

YujiOshima commented Feb 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hougangliu commented Feb 21, 2019

YujiOshima commented Feb 21, 2019

hougangliu commented Feb 21, 2019

YujiOshima commented Feb 22, 2019

hougangliu commented Feb 22, 2019

YujiOshima commented Feb 28, 2019

hougangliu commented Mar 1, 2019

YujiOshima commented Mar 4, 2019 • edited Loading

gaocegege commented Sep 29, 2019

k8s-ci-robot commented Sep 29, 2019

YujiOshima commented Feb 7, 2019 •

edited by jlewi

Loading

YujiOshima commented Feb 14, 2019 •

edited

Loading

hougangliu commented Feb 21, 2019 •

edited

Loading

YujiOshima commented Mar 4, 2019 •

edited

Loading