Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalized namespaces, added limits and other workers #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ashahab
Copy link

@ashahab ashahab commented Oct 8, 2018

What this PR does / why we need it:

Add your description

Which issue(s) this PR is related to (optional, link to 3rd issue(s)):

Fixes #

Reference to #

Special notes for your reviewer:

/cc @your-reviewer

Release note:

NONE

@caicloud-bot caicloud-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. labels Oct 8, 2018
@caicloud-bot
Copy link
Contributor

[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: ddysher

Assign the PR to them by writing /assign @ddysher 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:

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

@TravisBuddy
Copy link

Travis tests have failed

Hey @ashahab,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

gometalinter --config=linter_config.json --vendor ./...
pkg/s2i/img/img.go:1::warning: file is not goimported (goimports)
pkg/s2i/simple/simple.go:1::warning: file is not goimported (goimports)
pkg/manager/manager.go:88:21:warning: error return value not checked (m.S2IClient.Cleanup(parameter)) (errcheck)
pkg/backend/kubeflow/generator/cm.go:53:2:warning: should merge variable declaration with assignment on next line (S1021) (gosimple)
pkg/interpreter/simple/simple.go:53:12:warning: unnecessary conversion (unconvert)
cmd/kubeflow-kernel/command/run.go:1::warning: file is not goimported (goimports)
pkg/backend/kubeflow/kubeflow.go:1::warning: file is not goimported (goimports)
pkg/backend/kubeflow/generator/cm.go:1::warning: file is not goimported (goimports)
pkg/manager/manager.go:1::warning: file is not goimported (goimports)
pkg/s2i/types.go:1::warning: file is not goimported (goimports)
pkg/s2i/configmap/configmap.go:1::warning: file is not goimported (goimports)
goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/kernel/*.go"
?   	github.com/caicloud/ciao/pkg/backend	[no test files]
?   	github.com/caicloud/ciao/pkg/backend/kubeflow	[no test files]
=== RUN   TestCMNewTFJob
--- PASS: TestCMNewTFJob (0.00s)
=== RUN   TestCMNewPyTorchJob
--- PASS: TestCMNewPyTorchJob (0.00s)
=== RUN   TestNewTFJob
--- PASS: TestNewTFJob (0.00s)
=== RUN   TestNewPyTorchJob
--- PASS: TestNewPyTorchJob (0.00s)
PASS
coverage: 4.3% of statements in github.com/caicloud/ciao/pkg/backend, github.com/caicloud/ciao/pkg/backend/kubeflow, github.com/caicloud/ciao/pkg/backend/kubeflow/generator, github.com/caicloud/ciao/pkg/config, github.com/caicloud/ciao/pkg/interpreter, github.com/caicloud/ciao/pkg/interpreter/simple, github.com/caicloud/ciao/pkg/kernel, github.com/caicloud/ciao/pkg/manager, github.com/caicloud/ciao/pkg/s2i, github.com/caicloud/ciao/pkg/s2i/configmap, github.com/caicloud/ciao/pkg/s2i/img, github.com/caicloud/ciao/pkg/s2i/mock, github.com/caicloud/ciao/pkg/s2i/simple, github.com/caicloud/ciao/pkg/types
ok  	github.com/caicloud/ciao/pkg/backend/kubeflow/generator	0.042s
?   	github.com/caicloud/ciao/pkg/config	[no test files]
?   	github.com/caicloud/ciao/pkg/interpreter	[no test files]
anager
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i/configmap
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i/img
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i/mock
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i/simple
2018/10/08 03:18:32 Processing line: %framework=tensorflow
2018/10/08 03:18:32 Processing line: %ps=1
2018/10/08 03:18:32 Processing line: %worker=1
2018/10/08 03:18:32 Processing line: some code here.
2018/10/08 03:18:32 Processing line: %framework=tensorflow
2018/10/08 03:18:32 Processing line: %ps=1
2018/10/08 03:18:32 Processing line: some code here.
2018/10/08 03:18:32 Processing line: %framework=tensorflow
2018/10/08 03:18:32 Processing line: %ps=1

2nd Build

View build log

gometalinter --config=linter_config.json --vendor ./...
pkg/backend/kubeflow/generator/cm.go:53:2:warning: should merge variable declaration with assignment on next line (S1021) (gosimple)
cmd/kubeflow-kernel/command/run.go:1::warning: file is not goimported (goimports)
pkg/backend/kubeflow/generator/cm.go:1::warning: file is not goimported (goimports)
pkg/manager/manager.go:1::warning: file is not goimported (goimports)
pkg/s2i/img/img.go:1::warning: file is not goimported (goimports)
pkg/manager/manager.go:88:21:warning: error return value not checked (m.S2IClient.Cleanup(parameter)) (errcheck)
pkg/backend/kubeflow/kubeflow.go:1::warning: file is not goimported (goimports)
pkg/s2i/types.go:1::warning: file is not goimported (goimports)
pkg/s2i/configmap/configmap.go:1::warning: file is not goimported (goimports)
pkg/s2i/simple/simple.go:1::warning: file is not goimported (goimports)
pkg/interpreter/simple/simple.go:53:12:warning: unnecessary conversion (unconvert)
goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/kernel/*.go"
?   	github.com/caicloud/ciao/pkg/backend	[no test files]
?   	github.com/caicloud/ciao/pkg/backend/kubeflow	[no test files]
=== RUN   TestCMNewTFJob
--- PASS: TestCMNewTFJob (0.00s)
=== RUN   TestCMNewPyTorchJob
--- PASS: TestCMNewPyTorchJob (0.00s)
=== RUN   TestNewTFJob
--- PASS: TestNewTFJob (0.00s)
=== RUN   TestNewPyTorchJob
--- PASS: TestNewPyTorchJob (0.00s)
PASS
coverage: 4.3% of statements in github.com/caicloud/ciao/pkg/backend, github.com/caicloud/ciao/pkg/backend/kubeflow, github.com/caicloud/ciao/pkg/backend/kubeflow/generator, github.com/caicloud/ciao/pkg/config, github.com/caicloud/ciao/pkg/interpreter, github.com/caicloud/ciao/pkg/interpreter/simple, github.com/caicloud/ciao/pkg/kernel, github.com/caicloud/ciao/pkg/manager, github.com/caicloud/ciao/pkg/s2i, github.com/caicloud/ciao/pkg/s2i/configmap, github.com/caicloud/ciao/pkg/s2i/img, github.com/caicloud/ciao/pkg/s2i/mock, github.com/caicloud/ciao/pkg/s2i/simple, github.com/caicloud/ciao/pkg/types
ok  	github.com/caicloud/ciao/pkg/backend/kubeflow/generator	0.041s
?   	github.com/caicloud/ciao/pkg/config	[no test files]
?   	github.com/caicloud/ciao/pkg/interpreter	[no test files]
=== RUN   TestPreprocess
--- FAIL: TestPreprocess (0.00s)
	simple_test.go:70: Expected &{tensorflow 1 1 0   }, got &{ 0 0 0   }
	simple_test.go:70: Expected &{tensorflow 1 0 0   }, got &{ 0 0 0   }
	simple_test.go:70: Expected &{tensorflow 1 0 0   }, got &{ 0 0 0   }
=== RUN   TestGetPreProcessedCode
--- PASS: TestGetPreProcessedCode (0.00s)
FAIL
coverage: 3.1% of statements in github.com/caicloud/ciao/pkg/backend, github.com/caicloud/ciao/pkg/backend/kubeflow, github.com/caicloud/ciao/pkg/backend/kubeflow/generator, github.com/caicloud/ciao/pkg/config, github.com/caicloud/ciao/pkg/interpreter, github.com/caicloud/ciao/pkg/interpreter/simple, github.com/caicloud/ciao/pkg/kernel, github.com/caicloud/ciao/pkg/manager, github.com/caicloud/ciao/pkg/s2i, github.com/caicloud/ciao/pkg/s2i/configmap, github.com/caicloud/ciao/pkg/s2i/img, github.com/caicloud/ciao/pkg/s2i/mock, github.com/caicloud/ciao/pkg/s2i/simple, github.com/caicloud/ciao/pkg/types
exit status 1
FAIL	github.com/caicloud/ciao/pkg/interpreter/simple	0.036s
exit status 1: warning: no packages being tested depend on github.com/caicloud/ciao/pkg/backend
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/backend/kubeflow
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/backend/kubeflow/generator
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/config
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/interpreter
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/kernel
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/manager
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i/configmap
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i/img
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i/mock
warning: no packages being tested depend on github.com/caicloud/ciao/pkg/s2i/simple
2018/10/08 03:18:35 Processing line: %framework=tensorflow
2018/10/08 03:18:35 Processing line: %ps=1
2018/10/08 03:18:35 Processing line: %worker=1
2018/10/08 03:18:35 Processing line: some code here.
2018/10/08 03:18:35 Processing line: %framework=tensorflow
2018/10/08 03:18:35 Processing line: %ps=1
2018/10/08 03:18:35 Processing line: some code here.
2018/10/08 03:18:35 Processing line: %framework=tensorflow
2018/10/08 03:18:35 Processing line: %ps=1

3rd Build

View build log

gometalinter --config=linter_config.json --vendor ./...
pkg/backend/kubeflow/generator/cm.go:1::warning: file is not goimported (goimports)
pkg/manager/manager.go:1::warning: file is not goimported (goimports)
pkg/s2i/types.go:1::warning: file is not goimported (goimports)
pkg/s2i/configmap/configmap.go:1::warning: file is not goimported (goimports)
pkg/s2i/img/img.go:1::warning: file is not goimported (goimports)
pkg/manager/manager.go:88:21:warning: error return value not checked (m.S2IClient.Cleanup(parameter)) (errcheck)
cmd/kubeflow-kernel/command/run.go:1::warning: file is not goimported (goimports)
pkg/s2i/simple/simple.go:1::warning: file is not goimported (goimports)
pkg/backend/kubeflow/generator/cm.go:53:2:warning: should merge variable declaration with assignment on next line (S1021) (gosimple)
pkg/interpreter/simple/simple.go:53:12:warning: unnecessary conversion (unconvert)
pkg/backend/kubeflow/kubeflow.go:1::warning: file is not goimported (goimports)
goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/kernel/*.go"
?   	github.com/caicloud/ciao/pkg/backend	[no test files]
?   	github.com/caicloud/ciao/pkg/backend/kubeflow	[no test files]
=== RUN   TestCMNewTFJob
--- PASS: TestCMNewTFJob (0.00s)
=== RUN   TestCMNewPyTorchJob
--- PASS: TestCMNewPyTorchJob (0.00s)
=== RUN   TestNewTFJob
--- PASS: TestNewTFJob (0.00s)
=== RUN   TestNewPyTorchJob
--- PASS: TestNewPyTorchJob (0.00s)
PASS
coverage: 70.6% of statements in github.com/caicloud/ciao/pkg/backend, github.com/caicloud/ciao/pkg/backend/kubeflow, github.com/caicloud/ciao/pkg/backend/kubeflow/generator, github.com/caicloud/ciao/pkg/config, github.com/caicloud/ciao/pkg/interpreter, github.com/caicloud/ciao/pkg/interpreter/simple, github.com/caicloud/ciao/pkg/kernel, github.com/caicloud/ciao/pkg/manager, github.com/caicloud/ciao/pkg/s2i, github.com/caicloud/ciao/pkg/s2i/configmap, github.com/caicloud/ciao/pkg/s2i/img, github.com/caicloud/ciao/pkg/s2i/mock, github.com/caicloud/ciao/pkg/s2i/simple, github.com/caicloud/ciao/pkg/types
ok  	github.com/caicloud/ciao/pkg/backend/kubeflow/generator	0.035s	coverage: 70.6% of statements in github.com/caicloud/ciao/pkg/backend, github.com/caicloud/ciao/pkg/backend/kubeflow, github.com/caicloud/ciao/pkg/backend/kubeflow/generator, github.com/caicloud/ciao/pkg/config, github.com/caicloud/ciao/pkg/interpreter, github.com/caicloud/ciao/pkg/interpreter/simple, github.com/caicloud/ciao/pkg/kernel, github.com/caicloud/ciao/pkg/manager, github.com/caicloud/ciao/pkg/s2i, github.com/caicloud/ciao/pkg/s2i/configmap, github.com/caicloud/ciao/pkg/s2i/img, github.com/caicloud/ciao/pkg/s2i/mock, github.com/caicloud/ciao/pkg/s2i/simple, github.com/caicloud/ciao/pkg/types
?   	github.com/caicloud/ciao/pkg/config	[no test files]
?   	github.com/caicloud/ciao/pkg/interpreter	[no test files]
=== RUN   TestPreprocess
2018/10/08 03:16:58 Processing line: %framework=tensorflow
2018/10/08 03:16:58 Processing line: %ps=1
2018/10/08 03:16:58 Processing line: %worker=1
2018/10/08 03:16:58 Processing line: some code here.
2018/10/08 03:16:58 Processing line: %framework=tensorflow
2018/10/08 03:16:58 Processing line: %ps=1
2018/10/08 03:16:58 Processing line: some code here.
2018/10/08 03:16:58 Processing line: %framework=tensorflow
2018/10/08 03:16:58 Processing line: %ps=1
--- FAIL: TestPreprocess (0.00s)
	simple_test.go:70: Expected &{tensorflow 1 1 0   }, got &{ 0 0 0   }
	simple_test.go:70: Expected &{tensorflow 1 0 0   }, got &{ 0 0 0   }
	simple_test.go:70: Expected &{tensorflow 1 0 0   }, got &{ 0 0 0   }
=== RUN   TestGetPreProcessedCode
--- PASS: TestGetPreProcessedCode (0.00s)
FAIL
coverage: 50.0% of statements in github.com/caicloud/ciao/pkg/backend, github.com/caicloud/ciao/pkg/backend/kubeflow, github.com/caicloud/ciao/pkg/backend/kubeflow/generator, github.com/caicloud/ciao/pkg/config, github.com/caicloud/ciao/pkg/interpreter, github.com/caicloud/ciao/pkg/interpreter/simple, github.com/caicloud/ciao/pkg/kernel, github.com/caicloud/ciao/pkg/manager, github.com/caicloud/ciao/pkg/s2i, github.com/caicloud/ciao/pkg/s2i/configmap, github.com/caicloud/ciao/pkg/s2i/img, github.com/caicloud/ciao/pkg/s2i/mock, github.com/caicloud/ciao/pkg/s2i/simple, github.com/caicloud/ciao/pkg/types
FAIL	github.com/caicloud/ciao/pkg/interpreter/simple	0.002s
exit status 1: warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/backend
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/backend/kubeflow
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/backend/kubeflow/generator
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/config
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/interpreter
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/kernel
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/manager
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/s2i
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/s2i/configmap
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/s2i/img
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/s2i/mock
warning: no packages being tested depend on matches for pattern github.com/caicloud/ciao/pkg/s2i/simple
TravisBuddy Request Identifier: d8274990-caa8-11e8-bd56-bd6e450aa6f6

@@ -38,25 +36,16 @@ RUN git clone https://github.com/genuinetools/img.git "$GOPATH/src/github.com/ge
&& cd "$GOPATH/src/github.com/genuinetools/img" \
&& make static && mv img /usr/bin/img

FROM alpine:3.7
MAINTAINER Ce Gao <[email protected]>
FROM docker.artifactory-test.corp.linkedin.com/tensorflow/lipy-relevance-image-hdfs:0.1.404
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot get the image from Linkedin's private registry, so I think we cannot update the code here.

@@ -17,7 +17,7 @@ package command
import (
"fmt"
"log"

"os"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here should be placed in the first section.

@@ -1,3 +1,3 @@
kubeconfig: /var/run/kubernetes/admin.kubeconfig
kubeconfig: /var/run/kubernetes/tensorflow.corp-ltx1.kubeconfig
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default config template, then I think we should keep admin

@@ -16,18 +16,17 @@ package generator

import (
"fmt"

"os"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, os should be placed in the first section.

s2iconfigmap "github.com/caicloud/ciao/pkg/s2i/configmap"
pytorchv1alpha2 "github.com/kubeflow/pytorch-operator/pkg/apis/pytorch/v1alpha2"
tfv1alpha2 "github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1alpha2"
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"k8s.io/apimachinery/pkg/api/resource"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The third party package should be placed in the second section.

mountPath := fmt.Sprintf("/%s", parameter.Image)
filename := fmt.Sprintf("/%s/%s", parameter.Image, s2iconfigmap.FileName)

image_name := os.Getenv("IMAGE_NAME")
cpu_image_name := os.Getenv("CPU_IMAGE_NAME")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name here should be camel case.

WorkerPrefix: "%worker=",
PSPrefix: "%ps=",
MasterPrefix: "%master=",
WorkerPrefix: "kf_worker=",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no need to change % to kf_ since % is the convention in the community.

@gaocegege
Copy link
Contributor

/cc @ddysher

@caicloud-bot caicloud-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2018
@caicloud-bot
Copy link
Contributor

@ashahab: PR needs rebase.

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.

@caicloud-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@caicloud-bot caicloud-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 7, 2019
@gaocegege
Copy link
Contributor

gaocegege commented Mar 22, 2019

@ashahab Thanks for the PR! I will implement the feature based on your awesome work, since it seems that you have no time to update it.

@ashahab
Copy link
Author

ashahab commented Mar 22, 2019 via email

@gaocegege
Copy link
Contributor

@ashahab OK Welcome your contribution! Of course, I can wait for you.

I am a little busy in the past six months while now I think I have time to maintain the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants