KEP-672: Serial Job Execution with DependsOn API #680
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
kind/api-change
|
@andreyvelich: GitHub didn't allow me to assign the following users: shravan-achar, johnugeorge, akshaychitneni. Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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-sigs/prow repository. |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @andreyvelich! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @andreyvelich. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for kubernetes-sigs-jobset ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
keps/672-execution-policy/README.md
Outdated
| - name: model-initializer | ||
| persistentVolumeClaim: | ||
| claimName: model-initializer | ||
| - name: Node |
There was a problem hiding this comment.
nit: "Node" name is a bit confusing to me since this is for a fine tuning replicatedJob, maybe rename to "finetuner" or something like that? Same for story 2.
There was a problem hiding this comment.
I think, we make the Node name here on purpose, since from the distributed training perspective this ReplicatedJob represents the single Training Node.
Here is the table which explains it: https://github.com/kubeflow/training-operator/tree/master/docs/proposals/2170-kubeflow-training-v2#worker-and-node-definition.
That allows users to easily understand that once this Job spawns multiple Pods each Pod is a separate distributed training Node.
cc @tenzen-y
There was a problem hiding this comment.
We discussed today with @tenzen-y that we probably name this Job template as TrainerNode to make it clearer for user.
keps/672-execution-policy/README.md
Outdated
| // Names of the replicated Jobs that applied the status. | ||
| TargetReplicatedJobs []string `json:"targetReplicatedJobs"` | ||
|
|
||
| // Status in which the next replicated Jobs will be created. |
There was a problem hiding this comment.
| // Status in which the next replicated Jobs will be created. | |
| // Status the target replicated jobs much reach before subsequent replicated jobs begin executing. |
keps/672-execution-policy/README.md
Outdated
|
|
||
| Open Questions: | ||
|
|
||
| 1. How we should calculate quota for JobSet with execution policy ? |
There was a problem hiding this comment.
Is quota here referring to Kueue? If so, can you elaborate on what makes quota calculation tricky here? Is it the serial execution potentially allowing for node re-use which complicates calculating the required quota?
There was a problem hiding this comment.
That is correct, the tricky part if we allow Kueue to admit JobSet if resources are available for the Initializer Job, but not available for the Trainer Job.
@tenzen-y How do you think we should implement the ExecutionPolicy initially in terms of Kueue integrations ?
keps/672-execution-policy/README.md
Outdated
|
|
||
| #### Story 3 | ||
|
|
||
| TODO: Add HPC use-case with Job sequence |
There was a problem hiding this comment.
The HPC use case could be similar to the already mentioned ML ones - e.g., "As an HPC user, I want to run a set of simulations with MPI that complete successfully (or some percentage) before using the data for a next step analysis. For the workload manager - "as a workload manager developer I want to ensure my lead broker is up and Ready before starting the workers to connect to it." Or a third case - "As an HPC user I want to scale up my simulation job only after a small set have completed successfully." And for that case, I could imagine essentially the same replicatedJob done twice, just with a larger size. If the outcome of the first isn't success you wouldn't launch the larger bulk of work.
keps/672-execution-policy/README.md
Outdated
|
|
||
| // TODO: What statuses do we want to support in the first version ? | ||
| const ( | ||
| ReadyStatus ReplicatedJobsStatusOption = "Ready" |
There was a problem hiding this comment.
If we use an execution policy which specifies prior replicatedJobs must have a "Ready" status before subsequent ones can execute, isn't this effectively the same thing as using a StartupPolicy?
If so, perhaps we could eventually deprecate StartupPolicy in favor of this more flexible ExecutionPolicy over a couple releases
There was a problem hiding this comment.
If we use an execution policy which specifies prior replicatedJobs must have a "Ready" status before subsequent ones can execute, isn't this effectively the same thing as using a StartupPolicy?
That is correct. Eventually, we just run the StartupPolicy functionality here.
If so, perhaps we could eventually deprecate StartupPolicy in favor of this more flexible ExecutionPolicy over a couple releases
I am fine with that, but want to hear @kannon92 @ahg-g @vsoch @tenzen-y opinion on that.
There was a problem hiding this comment.
StartupPolicy seems more intuitive (from a user perspective) than an ExecutionPolicy -> Ready state, but I do think the latter is more flexible. I don't have a strong opinion either way, but if something is deprecated it should be well documented, etc.
There was a problem hiding this comment.
I think we need to phase out StartupPolicy over a few releases.
So I think this KEP should comment on compatibility between StartupPolicy and ExecutionPolicy.
If I specify StartupPolicy and execution Policy together, what would happen? Should we enforce validation so that these are "either or".
There was a problem hiding this comment.
If I specify StartupPolicy and execution Policy together, what would happen? Should we enforce validation so that these are "either or".
Yes, I think we should enforce "either or" on the first release then deprecate startup policy over the next few releases.
There was a problem hiding this comment.
Sure, @kannon92 @danielvegamyhre do we need separate section to explain it ?
Currently, I explained it in the validation section:
StartupPolicy should be equal to AnyOrder when ExecutionPolicy is used.
The StartupPolicy API will be deprecated over the next few JobSet releases, since
ExecutionPolicy can be used with Ready status of ReplicatedJobs.
There was a problem hiding this comment.
StartupPolicy should be equal to AnyOrder when ExecutionPolicy is used. The StartupPolicy API will be deprecated over the next few JobSet releases, since ExecutionPolicy can be used with Ready status of ReplicatedJobs.
This looks good to me.
There was a problem hiding this comment.
We could also extend the StartupPolicy API with the Succeeded semantics instead of creating a anew ExecutionPolicy field and deprecate the StartupPolicy one.
There was a problem hiding this comment.
Yes, I mentioned it in one of the alternatives: https://github.com/kubernetes-sigs/jobset/blob/4a77dab7aabbc99fee259f61f6b4e4c7efb51470/keps/672-execution-policy/README.md#add-executionpolicyrule-parameter-into-the-startuppolicy-api.
|
/remove-kind api-change |
k8s-ci-robot
removed
the
kind/api-change
keps/672-execution-policy/README.md
Outdated
|
|
||
| - ExecutionPolicy is immutable. | ||
| - ExecutionPolicyOrder of `AnyOrder` is default setting. | ||
| - StartupPolicy should be empty if ExecutionPolicy is set |
There was a problem hiding this comment.
I think StartupPolicy uses AnyOrder actually. So you would need to validate that StartupPolicy is not InOrder.
andreyvelich
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
keps/672-execution-policy/README.md
Outdated
|
|
||
| type ReplicatedJobsStatusOption string | ||
|
|
||
| // TODO: What statuses do we want to support in the first version ? |
There was a problem hiding this comment.
Ready and Succeeded I think are the only ones we should support.
There was a problem hiding this comment.
Though I would like to hear @ahg-g thoughts on ready..
There was a problem hiding this comment.
I agree with that. Support for Failed could be tricky given the failurePolicy API.
Remove Failed status
Co-authored-by: Michał Woźniak <mimowo@users.noreply.github.com>
Co-authored-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
Add Kueue info
Co-authored-by: Abdullah Gharaibeh <40361897+ahg-g@users.noreply.github.com>
Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
Signed-off-by: Andrey Velichkevich <andrey.velichkevich@gmail.com>
andreyvelich
force-pushed
the
kep-672
branch
from
d7aa5aa to
ea622a6
Compare
|
|
||
| User should configure JobSet as follows to use the existing functionality of StartupPolicy: | ||
|
|
||
| ```yaml |
There was a problem hiding this comment.
This was a bit confusing to me. For now, one could use startup policy or they can replicate it with the current API.
There was a problem hiding this comment.
No, startupPolicy and dependsOn APIs are mutually exclusive, which means only one of them can be set.
I mentioned it in this section: https://github.com/kubernetes-sigs/jobset/pull/680/files/ea622a6d6ba8cfd876a7d662da2e141c977ecddc#diff-760e1d37e0293ac3b2c0170c72e6ac764d7abd930c1f746f1764db1abe6a12b7R403
There was a problem hiding this comment.
Yea I understand. Reading this text it was not clear to me.
There was a problem hiding this comment.
Maybe it was pre coffee brain. Anyway I think this is fine.
|
|
||
| - `controllers`: `01/19/2024` - `30.2%` | ||
|
|
||
| We will add this functionality to the startup_policy API for the functionality. |
There was a problem hiding this comment.
If we are going to deprecate startup policy I would probably put the execution policy elsewhere.
There was a problem hiding this comment.
What do you mean by that ?
Similar to what @ahg-g proposed above, but with the executionPolicy field name: #680 (comment) ?
There was a problem hiding this comment.
We will add this functionality to the startup_policy API for the functionality.
You are going to put the code in startup_policy.go but we are going to deprecate startup_policy. That seems odd to me.
There was a problem hiding this comment.
Oh, I think, we should put the code under depends_on.go or execution_policy.go file, but I am open to the various options.
There was a problem hiding this comment.
Yea I think depends_on.go is fine but that is mostly an implementation detail.
|
/lgtm Thanks @andreyvelich |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, andreyvelich 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 |
k8s-ci-robot
added
the
approved
What type of PR is this?
Part of: #672.
/kind feature
/kind api-change
What this PR does / why we need it:
I created initial KEP for the ExecutionPolicy API.
A few points:
I was thinking a bit more on the
ExecutionPolicyOptionand we might want to introduce theExecutionPolicyRuleAPI, since the ReplicatedJob status can be different in the Job sequence.For example, when MPIJob is created, the Initializer Job should be in
Succeededstate, but the Launcher should be in theReadystate before running thempiruncommand. WDYT about this API @tenzen-y @alculquicondor ?@vsoch Please can you help us with the HPC use-case when Job sequence can be useful ?
Quota management via Kueue is still under discussion. What do you think we should add to this KEP ?
Special notes for your reviewer:
/assign @tenzen-y @johnugeorge @ahg-g @kannon92 @danielvegamyhre @googs1025 @akshaychitneni @shravan-achar