use the priority of kube-batch #209

YesterdayxD · 2019-08-19T13:08:28Z

I add the priority of kube-batch in pytorch-operator. I changed the code of tf-operator and kubebatch in vender dir,because their code is not latest.

k8s-ci-robot · 2019-08-19T13:08:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign richardsliu
You can assign the PR to them by writing /assign @richardsliu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2019-08-19T13:08:43Z

Hi @YesterdayxD. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

googlebot · 2019-08-19T13:08:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

coveralls · 2019-08-19T13:14:52Z

Coverage remained the same at 85.281% when pulling 6c24d6e on YesterdayxD:master into 0b237bc on kubeflow:master.

gaocegege · 2019-08-20T01:35:32Z

pkg/apis/pytorch/v1/types.go

@@ -69,6 +69,10 @@ type PyTorchJobSpec struct {
 	//     "Worker": PyTorchReplicaSpec,
 	//   }
 	PyTorchReplicaSpecs map[PyTorchReplicaType]*common.ReplicaSpec `json:"pytorchReplicaSpecs"`
+
+	//添加判断优先级的属性


Please use English here.

gaocegege · 2019-08-20T01:35:57Z

pkg/apis/pytorch/v1/types.go

+
+	//添加判断优先级的属性
+	//add PriorityClassName
+	PriorityClassName string `json:"priorityClassName,omitempty"`


Suggested change

PriorityClassName string `json:"priorityClassName,omitempty"`

PriorityClassName *string `json:"priorityClassName,omitempty"`

Since it is optional, we can define it as a pointer.

gaocegege · 2019-08-20T01:39:02Z

pkg/controller.v1/pytorch/controller.go

-		_, err := pc.SyncPodGroup(job, minAvailableReplicas)
+		priorityClassName:=getPriorityClassName(job)
+		//_, err := pc.SyncPodGroup(job, minAvailableReplicas)
+		_, err := pc.SyncPodGroupTest(job, minAvailableReplicas,priorityClassName)


Why should we use SyncPodGroupTest instead of SyncPodGroup

I add these codes in SyncPodGroupTest：
Spec: v1alpha1.PodGroupSpec{ MinMember: minAvailable.IntVal, PriorityClassName: priorityClassName, },

the name of this function is inappropriate，it is used to test my idea.

@gaocegege Do I detele my SyncPodGroupTest function and move the code my wrote into original SyncPodGroup function?

I think so.

gaocegege · 2019-08-20T01:39:13Z

pkg/controller.v1/pytorch/status.go

@@ -69,6 +69,7 @@ func (pc *PyTorchController) updateStatusSingle(job *pyv1.PyTorchJob, rtype pyv1

 	// Expect to have `replicas - succeeded` pods alive.
 	commonType := common.ReplicaType(rtype)
+	//expected是成功的判断标志，等于0时，成功的数量等于副本数，认为成功


Please use English here

YesterdayxD · 2019-08-20T02:58:22Z

@gaocegege Thank you.I will change the code you mentioned

gaocegege · 2019-08-20T03:16:13Z

pkg/controller.v1/pytorch/controller.go:1::warning: file is not goimported (goimports)

pkg/controller.v1/pytorch/job.go:1::warning: file is not goimported (goimports)

pkg/controller.v1/pytorch/job.go:221:2:warning: should merge variable declaration with assignment on next line (S1021) (staticcheck)

Thanks

PS, please fix the linting issues.

/ok-to-test

YesterdayxD · 2019-08-20T06:15:49Z

I don't sure that what happen the goimported issue.
Is it that golang versions are different?
my golang is 1.12.6
Travis CI is go1.10
I don't know how to solve it.
can you tell me?
@gaocegege
Do I need to change my golang version?

gaocegege · 2019-08-20T07:26:37Z

@YesterdayxD Then you can address the comments above, I can have a look at the issues.

googlebot · 2019-08-20T08:12:57Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

…next line

YesterdayxD · 2019-08-22T02:36:58Z

@gaocegege I fixed the issue about goimports :)

gaocegege · 2019-08-23T05:54:17Z

pkg/controller.v1/pytorch/job.go

@@ -216,3 +216,8 @@ func getTotalFailedReplicas(job *pyv1.PyTorchJob) int32 {
 	}
 	return totalFailedReplicas
 }
+
+func getPriorityClassName(job *pyv1.PyTorchJob) string {
+	priorityClassName := *(job.Spec.PriorityClassName)


There may be an runtime error: invalid memory address or nil pointer dereference

I need to remove the bracket

right？ @gaocegege

No I think you need to check if it is nil.

aws-kf-ci-bot · 2020-12-17T02:49:53Z

@YesterdayxD: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
kubeflow-pytorch-operator-presubmit-e2e	`6c24d6e`	link	`/test kubeflow-pytorch-operator-presubmit-e2e`
kubeflow-pytorch-operator-presubmit	`6c24d6e`	link	`/test kubeflow-pytorch-operator-presubmit`

Full PR test history. Your PR dashboard.

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.

YesterdayxD added 2 commits August 19, 2019 16:34

add priority

9b5cdab

add priority on kube-batch PriorityClassName

e3b3ea0

k8s-ci-robot requested review from andreyvelich and gaocegege August 19, 2019 13:08

k8s-ci-robot added needs-ok-to-test size/M labels Aug 19, 2019

gaocegege reviewed Aug 20, 2019

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Aug 20, 2019

YesterdayxD added 2 commits August 20, 2019 16:05

add English comment and add priority in SyncPodGroup function

aeb093e

merge develop

09d6c1b

YesterdayxD added 9 commits August 20, 2019 16:49

add priority in v1beta2

ba553a4

Merge branch 'develop'

a50a725

debug undefined: priorityClassName in v1beta2

6e17f2c

Merge branch 'develop'

3683cfa

add priority in zz_generated.deepcopy.go

47fed48

Merge branch 'develop'

a49a402

recover origin code

aad3b4c

fix goimports

2442fdc

fix gometalinter about merge variable declaration with assignment on …

6c24d6e

…next line

gaocegege reviewed Aug 23, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use the priority of kube-batch #209

use the priority of kube-batch #209

YesterdayxD commented Aug 19, 2019

k8s-ci-robot commented Aug 19, 2019

k8s-ci-robot commented Aug 19, 2019

googlebot commented Aug 19, 2019

coveralls commented Aug 19, 2019 •

edited

Loading

gaocegege Aug 20, 2019

gaocegege Aug 20, 2019

gaocegege Aug 20, 2019

YesterdayxD Aug 20, 2019

YesterdayxD Aug 20, 2019

gaocegege Aug 20, 2019

YesterdayxD Aug 20, 2019

gaocegege Aug 20, 2019

YesterdayxD commented Aug 20, 2019

gaocegege commented Aug 20, 2019

YesterdayxD commented Aug 20, 2019 •

edited

Loading

gaocegege commented Aug 20, 2019

googlebot commented Aug 20, 2019

YesterdayxD commented Aug 22, 2019

gaocegege Aug 23, 2019

YesterdayxD Aug 23, 2019

YesterdayxD Aug 23, 2019

gaocegege Aug 23, 2019

YesterdayxD Aug 23, 2019

aws-kf-ci-bot commented Dec 17, 2020

	PriorityClassName string `json:"priorityClassName,omitempty"`
	PriorityClassName *string `json:"priorityClassName,omitempty"`

use the priority of kube-batch #209

Are you sure you want to change the base?

use the priority of kube-batch #209

Conversation

YesterdayxD commented Aug 19, 2019

k8s-ci-robot commented Aug 19, 2019

k8s-ci-robot commented Aug 19, 2019

googlebot commented Aug 19, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

coveralls commented Aug 19, 2019 • edited Loading

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

YesterdayxD commented Aug 20, 2019

gaocegege commented Aug 20, 2019

YesterdayxD commented Aug 20, 2019 • edited Loading

gaocegege commented Aug 20, 2019

googlebot commented Aug 20, 2019

YesterdayxD commented Aug 22, 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

aws-kf-ci-bot commented Dec 17, 2020

coveralls commented Aug 19, 2019 •

edited

Loading

YesterdayxD commented Aug 20, 2019 •

edited

Loading