Python migration #435

jdplatt · 2019-03-16T19:59:45Z

This change is

jdplatt · 2019-04-15T12:58:21Z

/retest

johnugeorge · 2019-04-15T17:12:07Z

Can you update the description with the details of restructuring done in this PR ?

johnugeorge · 2019-04-15T17:18:25Z

manifests/v1alpha2/studyjobcontroller/crd.yaml

@@ -0,0 +1,19 @@
+apiVersion: apiextensions.k8s.io/v1beta1


studyjobcontroller is not needed in v1alpha2. Why are these files added?

jdplatt · 2019-04-15T18:02:52Z

Sure! Along with my changes to the python code to generate suggestions I added v1alpha2 folders for several components. For the following folders I copied my version into a new folder called v1alpha2 and the copied the v1alpha1 folder over from master:

manifests
scripts
pkg/suggestion
test/e2e
test/scripts

For the cmd/suggestion and test/workflow folders I left the current folders in the same place to avoid creating merge conflicts for other people and added a v1alpha2 folder containing my version. I figured we can clean up the cmd/suggestion folder when we merge the suggestion service into a single image, and the test/workflow folder when we do the integrations tests for v1alpha2.

Because my v1alpha2 is just the current state of the manifests, there will be need to be more changes as we add the v1alpha2 versions of the different components. For example, if the studyjobcontroller is not needed in v1alpha2 you can delete it in the PR that implements the replacement. This may not be the ideal approach, but this PR was already getting huge and I wanted to get it merged and not have to roll changes for even more components into it.

johnugeorge · 2019-04-15T18:30:55Z

Do you really need to copy all the files to v1alpha2 folder in this PR? Is there any reason? I feel that it will be better to add v1alpha2 only for relevant components related to the changes in this PR . eg: manifests for suggestion

For other components, we can add whenever corresponding v1alpha2 code is added.

jdplatt · 2019-04-15T19:58:27Z

The code for v1alpha1 and v1alpha2 is very similar so it seemed natural to take a copy of v1alpha1 as the starting point of v1alpha2 and make the required alterations, rather than build up v1alpha2 from scratch. I guess I could have created the v1alpha2 folders and cherry-picked only the files I changed to move over, but it seemed easier to move everything at once to make sure nothing was missed.

richardsliu · 2019-04-16T00:18:11Z

@jdplatt We've changed the API implementation quite a bit from v1alpha1 to v1alpha2, so taking a copy of all the files would make it harder for others to determine which files were copied from v1alpha1 and which ones are under development. It also unfortunately makes this PR harder to review. Can we include only the relevant changed files? Or alternatively mark the copied files with a note/comment so we know which files should be removed later?

jlewi · 2019-04-22T12:16:31Z

@jdplatt @richardsliu How's this PR coming? Do we have a goal in mind for getting it merged?

richardsliu · 2019-04-26T17:01:53Z

@jdplatt We should try to get this PR merged soon, otherwise we will be dealing with more conflicts later on. Let me know if you need help with this.

richardsliu · 2019-05-06T18:57:13Z

/retest

k8s-ci-robot · 2019-05-06T19:49:30Z

@jdplatt: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
kubeflow-katib-presubmit	`bbde559`	link	`/test kubeflow-katib-presubmit`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

richardsliu · 2019-05-06T19:06:44Z

cmd/suggestion/v1alpha2/katib-suggestion/README.md

+- start the service
+
+```
+python suggestion/bayesian/main.py


Is this the correct path?

richardsliu · 2019-05-06T23:49:32Z

pkg/suggestion/v1alpha2/random_service.go

@@ -0,0 +1,88 @@
+package suggestion


Is this still needed?

richardsliu · 2019-05-07T00:00:17Z

manifests/v1alpha2/vizier/suggestion/katib_suggestion/deployment.yaml

@@ -0,0 +1,28 @@
+apiVersion: extensions/v1beta1


Name of the directory should be bayestian-optimization?

richardsliu · 2019-05-07T00:07:52Z

pkg/suggestion/v1alpha2/katib_suggestion/algorithm_register.py

+from .bayesian_optimization_algorithm import BOAlgorithm
+
+
+ALGORITHM_REGISTER = {"random_search": RandomSearch,


How would someone distinguish between an algorithm that is still in development and one that is stable? What if users want to enable or disable a specific algorithm?

Also if a user wants to add a new algorithm, how would they achieve this without changing the shared Docker image?

can we make it configurable instead of hard-code

richardsliu · 2019-05-07T00:09:05Z

test/scripts/v1alpha2/run-tests.sh

@@ -66,6 +66,11 @@ echo "REPO_NAME ${REPO_NAME}"
 echo "VERSION ${VERSION}"

 sed -i -e "s@image: katib\/katib-controller@image: ${REGISTRY}\/${REPO_NAME}\/katib-controller:${VERSION}@" manifests/v1alpha2/katib-controller/katib-controller.yaml
+sed -i -e "s@image: katib\/suggestion@image: ${REGISTRY}\/${REPO_NAME}\/suggestion:${VERSION}@" manifests/vizier/suggestion/random/deployment.yaml


These seem to be missing the v1alpha2 path.

richardsliu · 2019-05-07T00:10:47Z

manifests/v1alpha2/vizier/suggestion/grid/deployment.yaml

@@ -0,0 +1,28 @@
+apiVersion: extensions/v1beta1


Is "vizier" necessary here? I think we can just use "suggestion" here. Also going forward we should avoid using the name vizier since it can confuse users.

hougangliu · 2019-05-07T07:40:47Z

cmd/suggestion/v1alpha2/hyperband/Dockerfile

@@ -0,0 +1,10 @@
+FROM golang:alpine AS build-env
+# The GOPATH in the image is /go.


we had cmd/suggestion/hyperband/v1alpha1 already. can you move it in cmd/suggestion/hyperband/v1alpha2? do dose others

hougangliu · 2019-05-07T07:43:15Z

cmd/suggestion/v1alpha2/hyperband/Dockerfile

+# The GOPATH in the image is /go.
+ADD . /go/src/github.com/kubeflow/katib
+WORKDIR /go/src/github.com/kubeflow/katib/cmd/suggestion/hyperband
+RUN go build -o hyperband


can go build -o hyperband work well?

hougangliu · 2019-05-07T07:45:54Z

pkg/suggestion/v1alpha2/types.go

@@ -0,0 +1,17 @@
+package suggestion


we'd better rename this file's name

hougangliu · 2019-05-07T08:32:08Z

cmd/suggestion/v1alpha2/katib-suggestion/main.py

+import grpc
+
+from pkg.api.python import api_pb2_grpc
+from pkg.suggestion.katib_suggestion.rpc_service import SuggestionService


wrong python path

hougangliu · 2019-05-07T08:45:13Z

cmd/suggestion/v1alpha2/hyperband/main.go

@@ -0,0 +1,27 @@
+package main


do you plan to migrate hyperband to python one in another PR? if yes, please move it in this PR

hougangliu · 2019-05-07T09:02:15Z

test/e2e/v1alpha2/invalid-studyjob.yaml

@@ -0,0 +1,26 @@
+apiVersion: "kubeflow.org/v1alpha1"
+kind: StudyJob


I think you need sync up most part in test/ for v1alpha2

hougangliu · 2019-05-07T09:07:15Z

pkg/suggestion/v1alpha2/katib_suggestion/algorithm_register.py

+from .bayesian_optimization_algorithm import BOAlgorithm
+
+
+ALGORITHM_REGISTER = {"random_search": RandomSearch,


can we make it configurable instead of hard-code

hougangliu · 2019-05-07T09:20:49Z

pkg/suggestion/v1alpha2/katib_suggestion/rpc_service.py

+        self.manager_addr = MANAGER_ADDRESS
+        self.manager_port = MANAGER_PORT
+        self.logger = logger if (logger is not None) else get_logger()
+        self.search_algorithm = search_algorithm


if search_algorithm is not in ALGORITHM_REGISTER, we should exit with right error message in init instead of exception occurring in line 30 when GetSuggestions.
That is, the corresponding deployment should not be Running if search_algorithm is not supported

hougangliu · 2019-05-07T09:24:13Z

pkg/suggestion/v1alpha2/katib_suggestion/random_search.py

+    def __init__(self, parameter_config, suggestion_config, logger=None):
+        self.parameter_config = parameter_config
+        self.suggestion_config = suggestion_config
+        self.logger = logger


self.logger unused

hougangliu · 2019-05-07T09:28:08Z

pkg/suggestion/v1alpha2/katib_suggestion/rpc_service.py

+    def GetSuggestions(self, request, context):
+        suggestion_config = self._parse_suggestion_parameters(request.param_id)
+        suggestion_config = {param.name: param.value for param in suggestion_config}
+        study_conf = self._get_study_config(request.study_id)


study_id field is dropped in v1alpha2

hougangliu · 2019-05-07T09:33:17Z

@jdplatt GetSuggestionsRequest of v1alpha2 changes a lot, I think you need sync up suggestion services of v1alpha2 with it.
Thanks!

richardsliu · 2019-05-09T00:09:04Z

Discussed offline with @jdplatt and @johnugeorge - we (myself and Johnu) will patch this PR and make the necessary fixes based on recent changes to the v1alpha2 API. Thanks John for your contributions.

epa095 · 2019-05-14T07:51:08Z

cmd/suggestion/v1alpha2/katib-suggestion/Dockerfile

@@ -0,0 +1,8 @@
+FROM python:3


In nasrl/Dockerfile it is pinned it to python:3.6 instead of having it floating at major version 3. Opinions differ, but I definitely think it is a good idea to have transitions between minor versions of python (like 3.6 -> 3.7) be a concious choice, it is not rare that stuff breaks (even though they should not), and then you need to pin the minor version.

epa095 · 2019-05-14T07:59:58Z

pkg/suggestion/v1alpha2/nasrl_service.py

+
+        self.registered_studies = dict()
+
+        if logger == None:


Why pass the logger object around? It is a bit confusing since you can then have a logger named package1.module1.class1 write logs for package1.module1.class2. If you instead just always do self.logger = logging.getLogger(__name__) then the loggers form a hierarchy reflecting the python module hierarchy, and you can configure the top logger and all the others will inherit that configuration

richardsliu · 2019-06-10T18:33:57Z

/close

k8s-ci-robot · 2019-06-10T18:33:59Z

@richardsliu: Closed this PR.

In response to this:

/close

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.

jdplatt and others added 30 commits February 26, 2019 15:13

added tests for acquisition function and models

75e9886

added tests for global_optimizer

dd3d563

added tests for boa

eeb76e5

minor linting

3e745ab

tests for algorithm manager

26f9106

added discrete parameter to study config

9dd5e6a

covered all parameter types

afe1874

moved python script to testing folder

f5a3860

Merge remote-tracking branch 'upstream/master'

76eb49f

linted bayesian_service.py

8620509

converted methods called in init for algorithm manager into functions

abe8e6e

eliminated algorithm_manager.py

d825a46

broke out parsing tests to cover individual functions

429c0c4

pushed parameter validation and defaults inside algorithm

dbc8b30

started testing service

1c3401b

added random search

89fcf56

added python tests to unit tests

41d19db

remembered to uncomment existing tests

f4ae52d

Merge branch 'master' into python_migration

b62c941

expanded parameter object and streamlined tests

a338fb2

updated manifests

a393d4b

made fetching observations optional

936708c

fixed path to test script

0217ace

moved python tests to separate job in workflow

9f75f05

added run command to test script

57dd5c5

fixed minor issues

acc3376

added grid search code

a95a6cb

updated manifests and dockerfiles

a417908

moved parsing inside algorithms

ddd0af3

Merge branch 'master' of https://github.com/kubeflow/katib

44e804f

added missing files

97720f6

johnugeorge reviewed Apr 15, 2019

View reviewed changes

John added 3 commits May 6, 2019 09:16

removed extra files from scripts folder

759bf35

removed extra files from test/scripts folder

120c63e

Merge branch 'master' into python_migration

bbde559

richardsliu reviewed May 7, 2019

View reviewed changes

hougangliu reviewed May 7, 2019

View reviewed changes

epa095 reviewed May 14, 2019

View reviewed changes

johnugeorge mentioned this pull request May 17, 2019

Adding random suggestion algorithm implementation and manifests #540

Merged

k8s-ci-robot closed this Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python migration #435

Python migration #435

jdplatt commented Mar 16, 2019 •

edited by jlewi

Loading

jdplatt commented Apr 15, 2019

johnugeorge commented Apr 15, 2019

johnugeorge Apr 15, 2019

jdplatt commented Apr 15, 2019

johnugeorge commented Apr 15, 2019

jdplatt commented Apr 15, 2019

richardsliu commented Apr 16, 2019

jlewi commented Apr 22, 2019

richardsliu commented Apr 26, 2019

richardsliu commented May 6, 2019

k8s-ci-robot commented May 6, 2019

richardsliu May 6, 2019

richardsliu May 6, 2019

richardsliu May 7, 2019

richardsliu May 7, 2019

hougangliu May 7, 2019

richardsliu May 7, 2019

richardsliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu May 7, 2019

hougangliu commented May 7, 2019

richardsliu commented May 9, 2019

epa095 May 14, 2019

epa095 May 14, 2019

richardsliu commented Jun 10, 2019

k8s-ci-robot commented Jun 10, 2019

		from .bayesian_optimization_algorithm import BOAlgorithm


		ALGORITHM_REGISTER = {"random_search": RandomSearch,

		@@ -0,0 +1,10 @@
		FROM golang:alpine AS build-env
		# The GOPATH in the image is /go.

		@@ -0,0 +1,26 @@
		apiVersion: "kubeflow.org/v1alpha1"
		kind: StudyJob

Python migration #435

Python migration #435

Conversation

jdplatt commented Mar 16, 2019 • edited by jlewi Loading

jdplatt commented Apr 15, 2019

johnugeorge commented Apr 15, 2019

Choose a reason for hiding this comment

jdplatt commented Apr 15, 2019

johnugeorge commented Apr 15, 2019

jdplatt commented Apr 15, 2019

richardsliu commented Apr 16, 2019

jlewi commented Apr 22, 2019

richardsliu commented Apr 26, 2019

richardsliu commented May 6, 2019

k8s-ci-robot commented May 6, 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

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 May 7, 2019

richardsliu commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardsliu commented Jun 10, 2019

k8s-ci-robot commented Jun 10, 2019

jdplatt commented Mar 16, 2019 •

edited by jlewi

Loading