Add Goptuna based suggestion service for CMA-ES. #1131

c-bata · 2020-04-09T09:25:52Z

What this PR does / why we need it:

Add CMA-ES suggestion service using Goptuna.

I tested this suggestion service with the following experiment:
https://github.com/c-bata/katib-goptuna-example

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1100

Special notes for your reviewer:

Goptuna suggestion service also supports TPE and Random. So if you modify katib-config, you can optimize your objective function with Goptuna TPE and Goptuna Random.
After merged this PR and the docker image is pushed, I'll create a pull request to update katib-config. See Add Goptuna based suggestion service for CMA-ES. #1131 (comment)

Release note:

Support [Goptuna](https://github.com/c-bata/goptuna) based suggestion service for CMA-ES.

kubeflow-bot · 2020-04-09T09:25:58Z

This change is

k8s-ci-robot · 2020-04-09T09:26:04Z

Hi @c-bata. 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.

c-bata · 2020-04-09T14:23:43Z

/ok-to-test

andreyvelich · 2020-04-09T15:04:46Z

Thank you, this is great!
I will take a look @c-bata.

andreyvelich

Overall looks good and easy to read! I left few comments

cmd/suggestion/goptuna/v1alpha3/main.go

manifests/v1alpha3/katib-controller/katib-config.yaml

andreyvelich · 2020-04-10T12:33:20Z

pkg/suggestion/v1alpha3/goptuna/service.go

@@ -0,0 +1,129 @@
+package suggestion_goptuna_v1alpha3


Do you want to add service to /goptuna folder or follow current design, when service is located under /suggestion/v1alpha3 folder?
I am not sure what is the best way to store Algorithms which is written in GO.
What do you think @gaocegege @johnugeorge ?

In Go, basically we can't put multiple packages in the same directory (except for *_test package). So I think we need to put this service into /goptuna folder.

pkg/suggestion/v1alpha3/goptuna/converter.go

pkg/suggestion/v1alpha3/goptuna/service.go

pkg/suggestion/v1alpha3/goptuna/converter.go

pkg/suggestion/v1alpha3/goptuna/service.go

andreyvelich · 2020-04-13T15:26:12Z

Thank you for the changes, overall /lgtm.

I think, we can add yaml example, CI Tests, update katib-config in the future PRs.

/assign @johnugeorge @gaocegege

Thank you for implementing this!

andreyvelich · 2020-04-13T15:30:12Z

/lgtm

c-bata · 2020-04-14T04:39:19Z

pkg/suggestion/v1alpha3/goptuna/sample.go

+func isSameTrialParam(ktrial, gtrial goptuna.FrozenTrial) bool {
+	// Compare trial parameters by "internal representation".
+	// In the internal representation, all parameters are represented by `float64` to store the storage
+	// (because Goptuna supports not only in-memory but also RDB storage backend).
+	// To represent categorical parameters, Goptuna holds an index of the list in the database.
+	//
+	// SearchSpace: map[string]interface{}{"x1": Uniform{Min: -10, Max: 10}, "x2": Categorical{Choices: []string{"param-1", "param-2"}}}
+	// External representation: map[string]interface{}{"x1": 5.5, "x2": "param-2"}
+	// Internal representation: map[string]float64{"x1": 5.5, "x2": 1.0}
+	for name := range gtrial.InternalParams {
+		gtrialParamValue := gtrial.InternalParams[name]
+		ktrialParamValue, ok := ktrial.InternalParams[name]
+		if !ok {
+			// must not reach here
+			klog.Errorf("Detect inconsistent internal parameters: %v and %v",
+				ktrial.InternalParams, gtrial.InternalParams)
+			return false
+		}
+		if gtrialParamValue != ktrialParamValue {
+			return false
+		}
+	}
+	return true
+}


Sorry, this logic seems to be broken.

In Goptuna, the internal representation is not necessarily reproducible from external representation because of the following reason:
optuna/optuna#925

So we need to compare external representations because ktrial is created from Katib parameter assignments which store external representation. I'll fix this soon.

I think it is enough to use reflect.DeepEqual(gtrial.Params, ktrial.Params.

I fixed this at 8c21693 and tested on the following parameter settings:

# https://github.com/c-bata/katib-goptuna-example/blob/master/experiment-step.yaml parameters: - name: x1 parameterType: int feasibleSpace: min: "-15" max: "15" step: "2" - name: x2 parameterType: double feasibleSpace: min: "-10" max: "10" step: 0.5

It works fine!

>>> logs -f example-step3-cmaes-54b4886448-cgkdm I0414 06:22:33.668875 1 main.go:36] Start Goptuna suggestion service: 0.0.0.0:6789 I0414 06:22:47.179106 1 service.go:67] Success to sample new trial: trialID=0, assignments=[name:"x1" value:"3" name:"x2" value:"1.5" ] I0414 06:22:47.182047 1 service.go:67] Success to sample new trial: trialID=1, assignments=[name:"x1" value:"5" name:"x2" value:"-1" ] I0414 06:22:53.962858 1 service.go:100] Update trial mapping : trialName=example-step3-lvmdgldl -> trialID=1 I0414 06:22:53.962882 1 service.go:100] Update trial mapping : trialName=example-step3-458v8s2c -> trialID=0 I0414 06:22:53.962888 1 service.go:130] Detect changes of Trial (trialName=example-step3-458v8s2c, trialID=0) : State Complete, Evaluation 46.250000 I0414 06:22:53.962988 1 service.go:67] Success to sample new trial: trialID=2, assignments=[name:"x2" value:"0.5" name:"x1" value:"-3" ] I0414 06:22:56.158324 1 service.go:100] Update trial mapping : trialName=example-step3-mrxdqpq9 -> trialID=2 I0414 06:22:56.158345 1 service.go:130] Detect changes of Trial (trialName=example-step3-lvmdgldl, trialID=1) : State Complete, Evaluation 16.000000 I0414 06:22:56.158473 1 service.go:67] Success to sample new trial: trialID=3, assignments=[name:"x1" value:"-7" name:"x2" value:"1" ] I0414 06:22:59.670946 1 service.go:130] Detect changes of Trial (trialName=example-step3-mrxdqpq9, trialID=2) : State Complete, Evaluation 94.250000 I0414 06:22:59.670984 1 service.go:100] Update trial mapping : trialName=example-step3-z9p7rklk -> trialID=3 I0414 06:22:59.671069 1 service.go:67] Success to sample new trial: trialID=4, assignments=[name:"x1" value:"-1" name:"x2" value:"-1" ]

PTAL.

gaocegege · 2020-04-14T08:26:13Z

pkg/suggestion/v1alpha3/goptuna/converter.go

+			}
+		}
+		return nil, cmaes.NewSampler(opts...), nil
+	} else if name == AlgorithmTPE {


I think we will have two suggestion providers which support TPE, do you think we should keep a prefix for it? Maybe gotuna-tpe?

/cc @andreyvelich @johnugeorge

Agree, we can name it goptuna-tpe and goptuna-random.

I'm a little bit concerned about the consistency of naming. It might be enough just updating katib-config if we want to use Goptuna suggestion service.

: suggestion: |- { "random": { "image": "gcr.io/kubeflow-images-public/katib/v1alpha3/suggestion-goptuna" }, "tpe": { "imagePullPolicy": "Always", "image": "gcr.io/kubeflow-images-public/katib/v1alpha3/suggestion-goptuna" }, "cmaes": { "imagePullPolicy": "Always", "image": "gcr.io/kubeflow-images-public/katib/v1alpha3/suggestion-goptuna" }, :

Agree with @c-bata .If we add prefix, user will be confused. User is not concerned about the internal implementation. What do you think?

Then we should update doc which implementation we currently support for which algorithm.
E.g random: hyperopt, chocolate, goptuna.

gaocegege

Generally LGTM

Thanks for your contribution! 🎉 👍

It is huge contribution.

pkg/suggestion/v1alpha3/goptuna/service.go

andreyvelich · 2020-04-15T17:11:19Z

I think it is ready to merge, right @c-bata ?

c-bata · 2020-04-15T18:23:47Z

I think it is ready to merge, right @c-bata ?

Yes!

FYI, I created issue #1147.

andreyvelich · 2020-04-15T18:54:29Z

I think it is ready to merge, right @c-bata ?

Yes!

FYI, I created issue #1147.

Thanks!
/lgtm
/cc @gaocegege @johnugeorge

andreyvelich

Let's merge it, I will create an issue for remaining work.
Thanks again @c-bata!
/approve

k8s-ci-robot · 2020-04-16T16:35:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich

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

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

* Add goptuna dependencies * Implement Goptuna based suggestion service * Add test cases * Validate duplicated parameters * Add test cases * Support step argument * Update vendor files * Support step argument for INT * Use new suggest APIs * Add refactor changes * Apply review feedbacks * Update vendor files * Apply review feedbacks * Fix tests * Add some refactor changes and add code comments * Fix tests * Refactor blank lines of import statements * Provide more algorithm settings * Create goptuna study and search space at first get suggestoins request * Compare trial parameters exactly * Fix isSameTrialParam() and refactor logging

k8s-ci-robot added needs-ok-to-test size/XXL labels Apr 9, 2020

k8s-ci-robot requested review from jinan-zhou and garganubhav April 9, 2020 09:26

This comment has been minimized.

Sign in to view

c-bata mentioned this pull request Apr 9, 2020

Bump up the Go version to 1.14.2 at Travis CI #1132

Merged

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Apr 9, 2020

c-bata changed the title ~~Add CMA-ES based suggestion service using Goptuna.~~ Add Goptuna based suggestion service for CMA-ES. Apr 9, 2020

c-bata force-pushed the cmaes-suggestion branch from 44ff60c to 0169033 Compare April 9, 2020 17:47

andreyvelich reviewed Apr 10, 2020

View reviewed changes

c-bata mentioned this pull request Apr 10, 2020

Add study option to define search space c-bata/goptuna#99

Merged

c-bata added 14 commits April 11, 2020 14:41

Add goptuna dependencies

ff5e0c2

Implement Goptuna based suggestion service

5533700

Add test cases

3929094

Validate duplicated parameters

4576221

Add test cases

10e58de

Support step argument

a00ace8

Update vendor files

6f37028

Support step argument for INT

3e89e4f

Use new suggest APIs

dfc7394

Add refactor changes

a4a8ebe

Apply review feedbacks

f261038

Update vendor files

77f6df3

Apply review feedbacks

dc542b8

Fix tests

472e588

Compare trial parameters exactly

be952f3

k8s-ci-robot assigned gaocegege Apr 13, 2020

k8s-ci-robot assigned andreyvelich Apr 13, 2020

k8s-ci-robot added the lgtm label Apr 13, 2020

andreyvelich mentioned this pull request Apr 13, 2020

Suggestion services folder structure #1144

Closed

c-bata commented Apr 14, 2020

View reviewed changes

Fix isSameTrialParam() and refactor logging

8c21693

k8s-ci-robot removed the lgtm label Apr 14, 2020

gaocegege reviewed Apr 14, 2020

View reviewed changes

k8s-ci-robot requested review from andreyvelich and johnugeorge April 14, 2020 08:26

gaocegege reviewed Apr 14, 2020

View reviewed changes

pkg/suggestion/v1alpha3/goptuna/service.go Show resolved Hide resolved

c-bata mentioned this pull request Apr 15, 2020

Update docs to tell which suggestion service supports for which algorithm. #1147

Closed

2 tasks

k8s-ci-robot requested a review from gaocegege April 15, 2020 18:54

k8s-ci-robot added the lgtm label Apr 15, 2020

andreyvelich approved these changes Apr 16, 2020

View reviewed changes

k8s-ci-robot added the approved label Apr 16, 2020

k8s-ci-robot merged commit 2238eee into kubeflow:master Apr 16, 2020

This was referenced Apr 16, 2020

add CMA-ES algorithm #67

Closed

Update katib-config document to describe suggestion service images kubeflow/website#1907

Merged

andreyvelich mentioned this pull request Apr 23, 2020

Rename Chocolate algorithm names #1163

Closed

c-bata mentioned this pull request Apr 23, 2020

Rename chocolate algorithm names for consistency #1164

Merged

andreyvelich mentioned this pull request Jun 9, 2020

Katib cut release for Kubeflow 1.1 #1211

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Goptuna based suggestion service for CMA-ES. #1131

Add Goptuna based suggestion service for CMA-ES. #1131

c-bata commented Apr 9, 2020 •

edited

Loading

kubeflow-bot commented Apr 9, 2020

k8s-ci-robot commented Apr 9, 2020

This comment has been minimized.

c-bata commented Apr 9, 2020

andreyvelich commented Apr 9, 2020

andreyvelich left a comment

andreyvelich Apr 10, 2020

c-bata Apr 11, 2020 •

edited

Loading

andreyvelich Apr 13, 2020

andreyvelich commented Apr 13, 2020

andreyvelich commented Apr 13, 2020

c-bata Apr 14, 2020

c-bata Apr 14, 2020

c-bata Apr 14, 2020

gaocegege Apr 14, 2020

andreyvelich Apr 14, 2020

c-bata Apr 14, 2020

johnugeorge Apr 14, 2020 •

edited

Loading

andreyvelich Apr 14, 2020

gaocegege left a comment

andreyvelich commented Apr 15, 2020

c-bata commented Apr 15, 2020

andreyvelich commented Apr 15, 2020

andreyvelich left a comment

k8s-ci-robot commented Apr 16, 2020

Add Goptuna based suggestion service for CMA-ES. #1131

Add Goptuna based suggestion service for CMA-ES. #1131

Conversation

c-bata commented Apr 9, 2020 • edited Loading

kubeflow-bot commented Apr 9, 2020

k8s-ci-robot commented Apr 9, 2020

This comment has been minimized.

c-bata commented Apr 9, 2020

andreyvelich commented Apr 9, 2020

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-bata Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreyvelich commented Apr 13, 2020

andreyvelich commented Apr 13, 2020

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 Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaocegege left a comment

Choose a reason for hiding this comment

andreyvelich commented Apr 15, 2020

c-bata commented Apr 15, 2020

andreyvelich commented Apr 15, 2020

andreyvelich left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 16, 2020

c-bata commented Apr 9, 2020 •

edited

Loading

c-bata Apr 11, 2020 •

edited

Loading

johnugeorge Apr 14, 2020 •

edited

Loading