-
Notifications
You must be signed in to change notification settings - Fork 1k
Add various Helm charts POCs #3154
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 various Helm charts POCs #3154
Conversation
|
[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 |
| # Example(Template) custom values for Kubeflow deployment | ||
|
|
||
| # Deploy spark-operator with custom configuration | ||
| spark-operator: |
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 @kunal-511.
I thought, we agreed that we are not going to have Helm Charts under kubeflow/manifests repository?
kubeflow/community#832 (comment)
Which means Helm Charts should be stored in the projects repos:
- https://github.com/kubeflow/spark-operator/tree/master/charts/spark-operator-chart
- https://github.com/kubeflow/trainer/tree/master/charts/kubeflow-trainer
cc @kubeflow/kubeflow-steering-committee @thesuperzapper @kimwnasptd
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.
Agreed upon was that the individual per project helm charts (KFP, Katib, Dashboard, ...) have the single source of truth long-term in the individual project repositories. But first we need to develop and test them. We need to see how they work together and use the same CI/CD. then remove them step by step again from /experimental. That is how I can mentor this GSOC project. Otherwise i will just not have the time for it if there is too much coordination overhead.
But as Andrey said @kunal-511, since there is already a Spark and trainer helm chart, you need to copy it from upstream automatically Maybe write a general script as you did for the kustomize manifests. The other ones you would have to do from scratch and upstream later at the end of the GSOC project. Long term we will synchronize all the individual helm charts as we now synchronize the kustomize manifests. Therefore it also makes sense to have them in an upstream folder as we do for kustomize.
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.
So i need to just create the 2 scripts that just fetch the upstream helm charts from there respective github ?? And add them under the experimental/helm/spark-operator and experimental/helm/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.
For the existing helm charts yes. But most other components do not have helm charts and there you have to build them from scratch. In the best case we can make them somewhat standardized. Then test whether kustomize and helm render the same output.
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.
Why do we need to duplicate Helm Charts from the individual repos in the kubeflow/manifests ? It would be hard to maintain 2 different Helm Charts and keep them in sync for every Kubeflow Project.
If you want to test installation for them, you can just define the script in kubeflow/manifests that deploys Helm Charts one after another from the upstream repositories.
As we discussed with @kimwnasptd and @thesuperzapper, kubeflow/manifests should not have Helm Charts manifests, since it focuses on Kustomize deployment.
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.
@kunal-511 The Kubeflow community has not yet agreed for all-in-one Helm chart. That’s why we should begin by working on individual Helm charts for each Kubeflow tool:
Kubeflow Pipelines
Kubeflow Trainer
Kubeflow Katib
Kubeflow Model Registry
Kubeflow Notebooks
Kubeflow Spark Operator
/hold
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.
Well yes the individual ones come first. They must be seperately deployable but they should be later combinable by downstream users.
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.
@juliusvonkohout Please can we keep this comment unresolved, since we still haven't agreed to have global Helm Chart for the entire Kubeflow Platform (e.g. Kubeflow Manifests) ?
cc @kimwnasptd @thesuperzapper
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.
@kunal-511 please if there is still one left, remove the combining chart for political reasons for now. This PR is open for way too long and we need to get it merged tomorrow.
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.
Okay I will just include the scripts for now and test them in the gha itself and not combining them.
|
Please can you check is this the right way now @juliusvonkohout |
|
@juliusvonkohout do i make the sync helm scripts to be same as the kustomize one which creates a seperate pr for that or include them directly here ? |
|
|
||
| - name: Install Helm | ||
| run: | | ||
| chmod +x tests/helm_install.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.
can you fix this directly on the file system ?
|
|
||
| - name: Run Helm Chart Tests | ||
| run: | | ||
| chmod +x experimental/helm/scripts/test-helm-charts.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.
can you fix this directly on the file system ?
| description: All-in-One Helm chart for deploying Kubeflow Platform | ||
| type: application | ||
| version: 0.1.0 | ||
| appVersion: "1.10.0" |
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.
master instead of 1.10.0 i assume
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
531a341 to
89a77b0
Compare
|
Rebase to master |
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
tests/helm_install.sh
Outdated
| set -e | ||
|
|
||
| echo "Installing Helm..." | ||
| curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 |
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.
the https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 script will force an update of helm which may or may not be something that the user wants. IMHO its better to have own script which checks if helm is already installed or not and then prompt user to continue if they need. Also, the above script will always install updated version of helm which,in future, may not be compatible with our 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.
Okay i will look for alternative
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.
But please no interactive stuff. Just install a fixed helm version.
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>
|
@kunal-511 @andreyvelich what is blocking the PR now ? |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@kunal-511 please rebase and get this read to merge. |
Pull Request Template for Kubeflow Manifests
✏️ Summary of Changes
🐛 Related Issues
✅ Contributor Checklist