-
Notifications
You must be signed in to change notification settings - Fork 18
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing 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 |
Travis tests have failedHey @ashahab, 1st Buildgometalinter --config=linter_config.json --vendor ./...
goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/kernel/*.go"
2nd Buildgometalinter --config=linter_config.json --vendor ./...
goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/kernel/*.go"
3rd Buildgometalinter --config=linter_config.json --vendor ./...
goveralls -service=travis-ci -v -package ./pkg/... -ignore "pkg/kernel/*.go"
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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=", |
There was a problem hiding this comment.
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.
/cc @ddysher |
@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. |
Issues go stale after 90d of inactivity. Prevent issues from auto-closing with an If this issue is safe to close now please do so with /lifecycle stale |
@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. |
Actually I do have time next week if you are fine waiting. This is an
awesome project, I'd like to contribute.
…On Thu, Mar 21, 2019, 11:35 PM Ce Gao ***@***.***> wrote:
@ashahab <https://github.com/ashahab> Thanks for the PR! I will implement
the feature based on your awesome work, since it seems that you have to
time to update it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#52 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAqJPVd17-veS6EVhm4lxpVFjpRED-S2ks5vZHnHgaJpZM4XMMPt>
.
|
@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. |
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: