-
Notifications
You must be signed in to change notification settings - Fork 730
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
Add Helm chart for kubeflow trainer #2435
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Yi Chen <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Yi Chen <[email protected]>
charts/trainer/README.md
Outdated
@@ -0,0 +1,105 @@ | |||
# trainer |
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.
Thank you for doing this @ChenYi015!
Did you get a chance to explore Kueue script to generate Helm Charts from Kustomize manifests ?
I think, that should significantly help us to keep Kustomize and Charts in sync.
https://github.com/kubernetes-sigs/kueue/blob/main/hack/update-helm.sh
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 will take a look at the shell script, it is a bit complicated. I think it may be more easier to sync manifests templated by Helm charts to Kustomize manifests, WDYT?
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 have added a shell script hack/sync-manifests
. Now one can execute make sync-manifests
to sync the Kustomize manifests from the manifests templated by the Helm chart.
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.
Thanks for this effort @ChenYi015!
@tenzen-y @kannon92 @ahg-g @astefanutti @Electronic-Waste @kubeflow/wg-training-leads What do you think about it ?
Should we go other way around to sync Helm Charts to Kustomize Manifests ?
We can use the same approach for JobSet/Kueue if that is easier.
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.
cc @kubeflow/wg-manifests-leads @kubeflow/release-team @varodrig to review script of sync Kustomize + Helm automatically.
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.
# limitations under the License. | ||
# | ||
|
||
apiVersion: v2 |
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.
Is there any way for us to import JobSet Helm charts into Kubeflow Trainer Helm charts ?
Or user has to install two Helm charts separately ?
E.g. JobSet might have charts in 0.8.0 release: kubernetes-sigs/jobset#774
cc @kannon92 @ahg-g @astefanutti @Electronic-Waste @kubeflow/wg-training-leads
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.
We can declare the JobSet Helm chart as a dependency in Chart.yaml
as follows if it has been released:
dependencies:
- name: jobset
version: 1.0.0
repository: https://kubernetes-sigs.github.io/jobset
Then we do not have to install two Helm charts separately, ref: https://helm.sh/docs/chart_best_practices/dependencies/.
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.
@ChenYi015 are you able to take over kubernetes-sigs/jobset#774 and address remaining comments? The person who raised the PR is not responsive, and I am unfortunately not a Helm chart expert, so hard for me to advise and continue his work.
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.
@ahg-g Yes, I am willing to raise another PR to do that.
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.
Wonderful, very much appreciated 👍
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.
Signed-off-by: Yi Chen <[email protected]>
…y the trainer Helm chart Signed-off-by: Yi Chen <[email protected]>
Signed-off-by: Yi Chen <[email protected]>
e0f0bfe
to
0623c20
Compare
Signed-off-by: Yi Chen <[email protected]>
7dc781c
to
c0b1d7d
Compare
What this PR does / why we need it:
Close #1197
Checklist: