-
Notifications
You must be signed in to change notification settings - Fork 251
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
attempt to commonize Kubeflow jobs Multikueue support methods #2795
attempt to commonize Kubeflow jobs Multikueue support methods #2795
Conversation
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/ok-to-test |
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.
That doesn't seem to be my expectations.
Could we commonize kubeflow adapters like kubeflow job reconcilers?
@alculquicondor you may want to have a look here too ;) |
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.
looks good overall :)
pkg/controller/jobs/kubeflow/kubeflowjob/kubeflowjob_multikueue_adapter.go
Outdated
Show resolved
Hide resolved
pkg/controller/jobs/kubeflow/kubeflowjob/kubeflowjob_multikueue_adapter.go
Outdated
Show resolved
Hide resolved
pkg/controller/jobs/kubeflow/kubeflowjob/kubeflowjob_multikueue_adapter.go
Outdated
Show resolved
Hide resolved
pkg/controller/jobs/kubeflow/kubeflowjob/kubeflowjob_multikueue_adapter.go
Outdated
Show resolved
Hide resolved
// add the prebuilt workload | ||
labels := remoteJob.GetLabels() | ||
if remoteJob.GetLabels() == nil { | ||
labels = make(map[string]string, 2) | ||
} | ||
labels[constants.PrebuiltWorkloadLabel] = workloadName | ||
labels[kueuealpha.MultiKueueOriginLabel] = origin | ||
remoteJob.SetLabels(labels) | ||
|
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 this code should even be part of the MK workload controller, but that can be left for a follow up.
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.
ok, noted
/release-note-edit
|
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.
Overall, looks good to me.
This is a comment for a followup.
Could you work on consolidating / commonizing the kubeflow MK integration tests (TFJob/PaddleJob/XGBoostJob/PyTorchJob)
ginkgo.It("Should run a TFJob on worker if admitted", func() { |
I'm wondering if we can reduce the kubeflow MK integration tests like kubeflowjob reconcilers.
pkg/controller/jobs/kubeflow/kubeflowjob/kubeflowjob_multikueue_adapter.go
Outdated
Show resolved
Hide resolved
I actually work on this one while doing MPIJob multikueue adapter, but I can add it here. |
It seems that this PR will be finalized soon. So, it may be helpful to work on test refactoring at another PR so that we do not block merging this PR. |
5f98f05
to
5ed0f3b
Compare
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: 282c50d2fe8cd61ed799f6f3196028fd327700e9
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, mszadkow 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 |
…etes-sigs#2795) * attempt to commonize Kubeflow jobs Multikueue support methods * make generic adapter to be specialized by job type * Move common Multikueue adapter to Kubeflowjob package * Changes after code review
/kind cleanup
What type of PR is this?
What this PR does / why we need it:
Attempt to unify code so it's not repeated for each kubeflow job multikueue adapter
Which issue(s) this PR fixes:
Relates #2552
Special notes for your reviewer:
Does this PR introduce a user-facing change?