-
Notifications
You must be signed in to change notification settings - Fork 71
feat: Add successpolicy #181
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: cegao <[email protected]>
/assign @terrytangyuan @Jeffwan @zw0610 |
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 great. Although I wonder if we can use this opportunity to re-think the approach. Instead of enums, would a label selector similar to what Argo Workflows is doing here be more flexible and allows more fine-grained control over the success conditions/policies? https://github.com/argoproj/argo-workflows/blob/master/examples/k8s-kubeflow-jobs.yaml#L24-L25
It is used in Katib. I think it works, but I think we should support successPolicy to keep API consistency. |
Yes, we are using |
Let's use kubeflow/training-operator#1507 to track and discuss separately. |
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 LGTM for consistency. Others PTAL.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: terrytangyuan 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 |
LGTM. Meanwhile, could you add descriptions for |
SGTM |
/hold |
SuccessPolicy is used in both PyTorchJob and TFJob. Thus I propose to add it in common.
Signed-off-by: cegao [email protected]