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.

Sign up for GitHub

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

Jump to bottom

Add SchedulerName in V1alpha2 #787

Merged
merged 4 commits into from
Aug 25, 2018
Merged

Add SchedulerName in V1alpha2 #787

merged 4 commits into from
Aug 25, 2018

Conversation

ChanYiLin
Copy link
Member

@ChanYiLin ChanYiLin commented Aug 20, 2018 •
edited
Loading

We have enablingGangScheduling option, so we add schedulerName option to TFJobSpec.

This change is Reviewable

@ChanYiLin
Copy link
Member Author

/assign @gaocegege

@ChanYiLin
Copy link
Member Author

Also fix the issue #787

@coveralls
Copy link

coveralls commented Aug 20, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 58.085% when pulling a9e17fc on ChanYiLin:master into 3061493 on kubeflow:master.

@johnugeorge
Copy link
Member

Please hold till #786 is merged.

Also, @jlewi @gaocegege we need some tracking to ensure that common changes are reflected in all operators.

@johnugeorge
Copy link
Member

/ok-to-test
/hold

ScorpioCPH
ScorpioCPH approved these changes Aug 21, 2018
View reviewed changes
Copy link
Member

@ScorpioCPH ScorpioCPH left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

pkg/apis/tensorflow/v1alpha2/types.go Outdated
@@ -48,6 +48,9 @@ type TFJobSpec struct {
// Default to Running.
CleanPodPolicy *CleanPodPolicy `json:"cleanPodPolicy,omitempty"`

// SchedulerName specifies the name of scheduler which should handle the TFJob
Copy link
Member

Choose a reason for hiding this comment

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

nits: missing period at the end of line.

@gaocegege
Copy link
Member

/lgtm

As @johnugeorge suggested, I think we could wait #786 .

Thanks for your contribution!

@ChanYiLin
Copy link
Member Author

Sure, no problem!

@johnugeorge
Copy link
Member

johnugeorge commented Aug 22, 2018 •
edited
Loading

/hold cancel

1 similar comment
@johnugeorge
Copy link
Member

/hold cancel

@ChanYiLin
Copy link
Member Author

@gaocegege
Would you help me to approve this PR? Thanks

@gaocegege
Copy link
Member

/approve

Thanks for your contribution!

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege

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

@gaocegege
Copy link
Member

/retest

1 similar comment
@ChanYiLin
Copy link
Member Author

/retest

@ChanYiLin
Copy link
Member Author

/test kubeflow-tf-operator-presubmit

@ChanYiLin
Copy link
Member Author

@jlewi
any idea why I got 0failed/16succeeded, but in log it showed

INFO|2018-08-24T04:58:05|/src/kubeflow/testing/py/kubeflow/testing/argo_client.py|23| Workflow kubeflow-tf-operator-presubmit-v1-787-3f08a4d-977-f888 in namespace kubeflow-test-infra; phase=Failed
INFO|2018-08-24T04:58:05|/src/kubeflow/testing/py/kubeflow/testing/argo_client.py|23| Workflow kubeflow-tf-operator-presubmit-v2-787-3f08a4d-977-d309 in namespace kubeflow-test-infra; phase=Running
INFO|2018-08-24T04:58:35|/src/kubeflow/testing/py/kubeflow/testing/argo_client.py|23| Workflow kubeflow-tf-operator-presubmit-v1-787-3f08a4d-977-f888 in namespace kubeflow-test-infra; phase=Failed
INFO|2018-08-24T04:58:35|/src/kubeflow/testing/py/kubeflow/testing/argo_client.py|23| Workflow kubeflow-tf-operator-presubmit-v2-787-3f08a4d-977-d309 in namespace kubeflow-test-infra; phase=Failed
INFO|2018-08-24T04:58:35|/src/kubeflow/testing/py/kubeflow/testing/run_e2e_workflow.py|247| Workflow kubeflow-test-infra/kubeflow-tf-operator-presubmit-v1-787-3f08a4d-977-f888 finished phase: Failed
INFO|2018-08-24T04:58:35|/src/kubeflow/testing/py/kubeflow/testing/run_e2e_workflow.py|247| Workflow kubeflow-test-infra/kubeflow-tf-operator-presubmit-v2-787-3f08a4d-977-d309 finished phase: Failed

and kubeflow-tf-operator-presubmit also failed?

@ChanYiLin
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 24, 2018
@TravisBuddy
Copy link

Travis tests have failed

Hey @ChanYiLin,
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.

2nd Build

gometalinter --config=linter_config.json --vendor ./... 
cmd/tf-operator.v2/app/options/options.go:1::warning: file is not goimported (goimports)

3rd Build

gometalinter --config=linter_config.json --vendor ./... 
cmd/tf-operator.v2/app/options/options.go:1::warning: file is not goimported (goimports)

travis_time:end:019f2411:start=1535124892323861997,finish=1535125045612160227,duration=153288298230

@ChanYiLin
Copy link
Member Author

/retest

@ChanYiLin
Copy link
Member Author

@gaocegege
I need your help again, would you please to help me to give it a look, thanks!

@ChanYiLin ChanYiLin mentioned this pull request Aug 25, 2018
Merged
@johnugeorge
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot merged commit e9bb7e1 into kubeflow:master Aug 25, 2018
@jlewi jlewi mentioned this pull request Aug 27, 2018
Closed
@gaocegege gaocegege mentioned this pull request Aug 30, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ScorpioCPH ScorpioCPH ScorpioCPH approved these changes

@codeflitting codeflitting Awaiting requested review from codeflitting

@jimexist jimexist Awaiting requested review from jimexist

Assignees

@johnugeorge johnugeorge

@gaocegege gaocegege

Labels
approved lgtm size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ChanYiLin @coveralls @johnugeorge @gaocegege @k8s-ci-robot @TravisBuddy @ScorpioCPH