Compile samples instead of hard code them in API server #76

IronPan · 2018-11-06T02:10:16Z

Please ignore the sample names in the sample configs since they are still TBD.

This change is

IronPan · 2018-11-06T02:11:41Z

/assign @qimingj @vicaire

…into yangpa/samples

…angpa/samples

qimingj · 2018-11-06T18:59:06Z

backend/Dockerfile

@@ -14,6 +14,15 @@ COPY . .
 RUN apk add --update gcc musl-dev
 RUN go build -o /bin/apiserver backend/src/apiserver/*.go

+FROM python:3.5.0-slim as compiler
+
+# This is hard coded to 0.0.26. Once kfp DSK release process is automated,


This is already in (#70). Give it a try?

the test need to update to adopt that. it's kind of late for the last minute change. I'll do it post beta.

qimingj · 2018-11-06T19:00:03Z

backend/src/apiserver/config/sample_config.json

+    "file":"/samples/tfx/taxi-cab-classification-pipeline.py.tar.gz"
+  },
+  {
+    "name":"[Sample] ML - CMLE - Resnet Training",


We have to remove this one --- CMLE is not ready for TPU yet.

qimingj · 2018-11-06T19:01:20Z

backend/Dockerfile

+RUN pip install https://storage.googleapis.com/ml-pipeline/release/0.0.26/kfp-0.0.26.tar.gz --upgrade
+WORKDIR /samples
+COPY ./samples .
+RUN  find . -maxdepth 2 -name "*.py" -exec python3  {} \;


I think it is more robust to call dsl-compile on these py files --- Not all py files come with self-compilation code. It is not required.

vicaire · 2018-11-06T20:10:41Z

backend/src/apiserver/main.go

@@ -132,16 +132,26 @@ func loadSamples(resourceManager *resource.ResourceManager) {
 	}
 	var configs []config
 	if json.Unmarshal(configBytes, &configs) != nil {
-		glog.Warningf("Failed to read sample configurations. Err: %v", err.Error())
-		return
+		glog.Fatalf("Failed to read sample configurations. Err: %v", err.Error())


Could we please return errors in this method and have the top level decide whether to abort? (i.e. let's avoid functions with side effects as it makes code hard to reuse).

vicaire · 2018-11-06T20:12:57Z

backend/src/apiserver/main.go

 		}
-		resourceManager.CreatePipeline(config.Name, config.Description, sampleBytes)
+		// Decompress if file is tarball


Do you need to test whether it is a tarball or not?

Why not reusing the logic in the API that already turns a file (yaml or tarball) into a pipeline object?

…angpa/samples

qimingj · 2018-11-06T21:41:37Z

backend/Dockerfile

@@ -14,6 +14,15 @@ COPY . .
 RUN apk add --update gcc musl-dev
 RUN go build -o /bin/apiserver backend/src/apiserver/*.go

+FROM python:3.5.0-slim as compiler
+
+# This is hard coded to 0.0.26. Once kfp DSK release process is automated,


qimingj · 2018-11-06T21:42:50Z

/lgtm

vicaire · 2018-11-06T21:40:19Z

backend/src/apiserver/main.go

@@ -132,16 +138,26 @@ func loadSamples(resourceManager *resource.ResourceManager) {
 	}
 	var configs []config
 	if json.Unmarshal(configBytes, &configs) != nil {
-		glog.Warningf("Failed to read sample configurations. Err: %v", err.Error())
-		return
+		return errors.New(fmt.Sprintf("Failed to parse sample configurations."))


why not returning the original error message as part of the result?

vicaire · 2018-11-06T21:45:38Z

test/presubmit-tests.sh

@@ -72,7 +72,7 @@ if [ "$CLUSTER_TYPE" == "create-gke" ]; then
    echo "Delete cluster..."
    gcloud container clusters delete ${TEST_CLUSTER} --async
  }
-  trap delete_cluster EXIT
+  # trap delete_cluster EXIT


why are we removing this?

this is for debugging. revert

vicaire · 2018-11-06T21:46:44Z

backend/src/apiserver/main.go

-	loadSamples(resourceManager)
+	err:= loadSamples(resourceManager)
+	if err!=nil{
+		glog.Fatalf("Failed to load samples. Err: %v", err.Error())


What if the server restarts?

Does this prevent the server from ever restarting unless the storage is deleted?

the samples are baked into the image so it won't be a problem

vicaire · 2018-11-06T21:52:50Z

/lgtm

IronPan · 2018-11-06T22:29:15Z

/approve

k8s-ci-robot · 2018-11-06T22:29:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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:

~~backend/OWNERS~~ [IronPan]
~~samples/OWNERS~~ [IronPan]

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

IronPan · 2018-11-06T22:29:24Z

/lgtm

k8s-ci-robot · 2018-11-06T22:29:24Z

@IronPan: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

google-prow-robot · 2018-11-06T22:29:24Z

@IronPan: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

IronPan · 2018-11-06T23:05:31Z

/lgtm

k8s-ci-robot · 2018-11-06T23:05:32Z

@IronPan: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

google-prow-robot · 2018-11-06T23:05:32Z

@IronPan: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

vicaire · 2018-11-06T23:06:21Z

/lgtm

* Moves docs from the /samples README to the wiki I've copied all the info from the googleprivate/samples README.md to the wiki. This PR removes most of the content from the README, adds a simple introduction, and points people to the wiki docs for further information. Relevant wiki files: https://github.com/kubeflow/pipelines/wiki/Deploy-the-Kubeflow-Pipelines-Service https://github.com/kubeflow/pipelines/wiki/Build-a-Pipeline https://github.com/kubeflow/pipelines/wiki/Build-Your-Own-Component * SDK/Tests/Components - Improve temporary file handling (#37) * Improve temporary file handling in python op tests * More small temp path fixes * Fix tfx name bug in the tfma sample test (#67) * fix tfx name bug * update release build for the data publish * SDK/DSL/Compiler - Fixed compilation of dsl.Condition (#28) * Fixed compilation of dsl.Conditional The compiler no longer produced intermediate steps. * Got rid of _create_new_groups * Changed the sub_group.type check * Fix tfx name bug in the tfma sample test (#67) * fix tfx name bug * update release build for the data publish * Build Python SDK in the releasing (#70) * publish python sdk in the cloud build * update cloudbuild * adjust output path * add dependency for buildapiserver * mlp -> kfp.dsl (#88) * Removed instruction to clone the repo. * Changed link to samples doc on wiki. * Add %%docker magic to jupyter kernel. It helps submitting a docker build job more easily with one cell. (#72) * fix miscellaneous List API issue (#90) * fix * Update job_store.go * Update run_store.go * Update presubmit-tests.sh * Update job_store.go * Moves docs from pipelines main README to wiki (#83) I've copied all the info from the kubeflow/pipelines README.md to the wiki. This PR removes most of the content from the README, adds a simple introduction, and points people to the wiki docs for further information. Relevant wiki files: https://github.com/kubeflow/pipelines/wiki https://github.com/kubeflow/pipelines/wiki/Deploy-the-Kubeflow-Pipelines-Service https://github.com/kubeflow/pipelines/wiki/Build-a-Pipeline * SDK/DSL/Compiler - Reverted fix of dsl.Condition until the UI is ready. (#94) * sort by run display name by default (#96) * CSS changes for nav menu and tables (#99) * debug tfma failure (#91) * debug tfma failure * tft version bug * minor fix * comment the test validation * Fix validation check for maximum size limit (#104) * Fixed the Minikube tests after moving to the new repo (#98) * Don't barf when experiment name is already used (#101) * don't barf when experiment name is already used * make mock backend use case insensitive names * ExperimentList tests, use immer.js (#86) * experiment list tests * use produce in more places * remove extra test * remove extra import * Use the experiment's resource reference in the listJobs request (#105) * use the experiment's resource reference in the listJobs request * fix test import * Add Ning and Alexey to OWNERS for components, samples and sample-test (#102) * Compile samples instead of hard code them in API server (#76) * compile samples * update logging * update description * update sample * add immediate value sample * revert * fail fast if the samples are failed to load * comment * address comments * comment out * update command * comments * Account for padding in metric progress fill (#107) * Account for padding in metric progress fill * small mock backend fix * move to css classes, add color * changes to breadcrumb style * increase width of summary card * tests * merge tests * First integration test for the ML Pipeline CLI (Pipeline List). (#81) * First integration test for the ML Pipeline CLI (Pipeline List). * Fixing an issue with an undefined variable * Adding the --debug flag to help with debugging. * Changing the namespace to Kubeflow. * Add tests for the NewExperiment page (#109) * Add tests for the NewExperiment page * Fix test name * Remove obsolete tests * Clean up * add xgboost: migrate from the old repo (#46) * migrate from the old repo * fix bug: accidentally override tfma test * add tfma test back * add tfma back * typo fix * fix small typo * if job fails, exit after logs are output * Remove CMLE sample for now since we are waiting for a service fix to support TPU. (#113) * image tag update for release (#114) * update image tag for new releases * add more * delete the accidentaly added sample * fix typo (#116) * Fix an issue that %%docker doesn't work. (#119) * updated favicon to monochrome color (#118) * Expanded row changes (#120) * updated favicon to monochrome color * simple CSS changes to expanded row * Removed mentions of ark7 in tests (#111) * Add basic sample tests (#79) * add sequential sample test * add condition basic sample * reuse script * add all the other basic tests * update sample test dockerfile to add run_basic_test file * write test output * typo bug

* Add ssh for minikube testing. * We use ssh to execute code on the VM used for minikube. * Log the file used to configure kubectl.

Include permission for get services

compile samples

8df55d3

k8s-ci-robot requested review from qimingj and vicaire November 6, 2018 02:10

k8s-ci-robot added the size/XXL label Nov 6, 2018

k8s-ci-robot assigned qimingj and vicaire Nov 6, 2018

IronPan and others added 8 commits November 6, 2018 00:08

Merge branch 'yangpa/samples' of https://github.com/kubeflow/pipelines …

6b38293

…into yangpa/samples

update logging

0ba9eee

update description

3cd0d30

update sample

4c599ba

add immediate value sample

204d7bf

revert

8a1ca01

Merge branch 'master' of https://github.com/kubeflow/pipelines into y…

2569f64

…angpa/samples

fail fast if the samples are failed to load

1b30574

qimingj reviewed Nov 6, 2018

View reviewed changes

comment

1998970

vicaire suggested changes Nov 6, 2018

View reviewed changes

IronPan added 4 commits November 6, 2018 12:40

address comments

5060b25

comment out

0389e64

update command

05b5b21

Merge branch 'master' of https://github.com/kubeflow/pipelines into y…

de258a7

…angpa/samples

qimingj approved these changes Nov 6, 2018

View reviewed changes

k8s-ci-robot added the lgtm label Nov 6, 2018

vicaire suggested changes Nov 6, 2018

View reviewed changes

vicaire approved these changes Nov 6, 2018

View reviewed changes

comments

ee8911c

k8s-ci-robot removed the lgtm label Nov 6, 2018

k8s-ci-robot added the approved label Nov 6, 2018

k8s-ci-robot added the lgtm label Nov 6, 2018

k8s-ci-robot merged commit 7847b74 into master Nov 6, 2018

IronPan deleted the yangpa/samples branch November 8, 2018 00:32

Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023

Add ssh for minikube testing. (kubeflow#76)

5a363bf

* Add ssh for minikube testing. * We use ssh to execute code on the VM used for minikube. * Log the file used to configure kubectl.

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Moved Custom sample to the samples directory (kubeflow#76)

812d0ac

HumairAK referenced this pull request in red-hat-data-services/data-science-pipelines Mar 11, 2024

add transformer test for annotations and labels (#76)

6df545a

HumairAK referenced this pull request in red-hat-data-services/data-science-pipelines Mar 11, 2024

Merge pull request #76 from harshad16/include-role

766729f

Include permission for get services

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile samples instead of hard code them in API server #76

Compile samples instead of hard code them in API server #76

IronPan commented Nov 6, 2018 •

edited

Loading

IronPan commented Nov 6, 2018

qimingj Nov 6, 2018

IronPan Nov 6, 2018

qimingj Nov 6, 2018

qimingj Nov 6, 2018

IronPan Nov 6, 2018

qimingj Nov 6, 2018

IronPan Nov 6, 2018

vicaire Nov 6, 2018

IronPan Nov 6, 2018

vicaire Nov 6, 2018

IronPan Nov 6, 2018

qimingj Nov 6, 2018

qimingj commented Nov 6, 2018

vicaire Nov 6, 2018

IronPan Nov 6, 2018

vicaire Nov 6, 2018

IronPan Nov 6, 2018

vicaire Nov 6, 2018

IronPan Nov 6, 2018

vicaire commented Nov 6, 2018

IronPan commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

IronPan commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

google-prow-robot commented Nov 6, 2018

IronPan commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

google-prow-robot commented Nov 6, 2018

vicaire commented Nov 6, 2018

Compile samples instead of hard code them in API server #76

Compile samples instead of hard code them in API server #76

Conversation

IronPan commented Nov 6, 2018 • edited Loading

IronPan commented Nov 6, 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

qimingj commented Nov 6, 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

vicaire commented Nov 6, 2018

IronPan commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

IronPan commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

google-prow-robot commented Nov 6, 2018

IronPan commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

google-prow-robot commented Nov 6, 2018

vicaire commented Nov 6, 2018

IronPan commented Nov 6, 2018 •

edited

Loading