-
Notifications
You must be signed in to change notification settings - Fork 1k
Pipeline Helm Charts #3237
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
base: master
Are you sure you want to change the base?
Pipeline Helm Charts #3237
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 |
|
Please rebase to master |
7c85ed8 to
d75fc35
Compare
|
@juliusvonkohout It is now ready for review |
|
@kunal-511 i think you have to remove the AWS and GCP specific stuff, because it it more than basic integration and could violate the charter. It is also mostly remove in upstream KFP now. |
|
/retest |
|
The kfp 2.15 release with fundamental changes is supposed to be close in the next week and manged by @HumairAK @droctothorpe and other maintainers. @kunal-511 do you want to wait for it before we merge ? |
|
Yes we can wait for that |
|
Hey, I have some experience with Helm Charts and Kubeflow. I created one Kubeflow Helm Chart some time ago as a PoC to show at least one possible approach for this initiative. My approach was to have One Big Helm Chart with a series of parameterizations. I see the approach here is to have separate Helm Charts which also makes sense. Just FYI, my work is available here: https://github.com/kromanow94/kubeflow-manifests/releases/tag/kubeflow-0.5.0 I want to let you know that I have some time to get back to the idea of Kubeflow Helm Chart and I'm willing to help with this initiative. In the next few days I'm planning to have a closer look at this PR and try to find some place where I could help. Let me know if there is something on your mind that I could do. Best, |
|
2b438e5 to
1345f9e
Compare
|
rebase this due to #3283 |
Thank you. First you can remove a lot of stuff. We deleted some environments and files such as sync.py changed completely which is not reflected here. I think many files are outdated now also with old image tags Also we need to find a way to cut the lines in this PR by roughly 75 %. For example most security contexts are already PSS restricted, so the should not be configurable. That already saves a few hundred lines. Also minio is gone entirely and we should only support seaweedfs. Also postgresql is not needed right now i would say. So most of the value files in the ci folder can also go. In the end we only need to support 2 flavors: manifests/example/kustomization.yaml Lines 70 to 72 in 9e1d4d2
and then we should expose the basic configurationmaps, usernames and passwords for seaweedfs / databases. We can step by step expose more if needed by customers, but lets start with a minimal setup that is easy to maintain and good enough for 80 % of users with 20% effort. |
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]>
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]>
333302c to
0b0082d
Compare
|
@juliusvonkohout I have updated the pipelines helm charts to support only 2 flavors and according to the new version |
Signed-off-by: kunal-511 <[email protected]>
|
|
||
| description: A Helm chart for Kubeflow Pipelines - ML Workflows on Kubernetes | ||
|
|
||
| version: 2.14.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.
We are at 2.15.0 please make sure to update all files. You really need to check each file in this PR.
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
| scheduledWorkflow: | ||
| additionalEnv: | ||
| - name: NAMESPACE | ||
| value: "" |
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 what do we need this empty value ?
| persistenceAgent: | ||
| additionalEnv: | ||
| - name: NAMESPACE | ||
| value: "" |
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 what do we need this empty value ?
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 directly picked from env where it is mentioned:- Empty namespace let viewer controller watch all namespaces
| crds: | ||
| install: true | ||
| application: false | ||
| webhook: false |
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.
Should this not depend on pipelineDefinition:
storage: database ?
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 add a comments in both values files that crds.webhook should match pipelineDefinition.storage for consistency, even though templates check pipelineDefinition.storage directly
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ include "kubeflow-pipelines.cache.serviceAccountName" . }} | ||
| namespace: {{ include "kubeflow-pipelines.namespace" . }} |
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.
It must anyway the same namespace. All KFP components must be in the same namespace so you can use a shared variable across all resources.
| name: kubeflow-pipelines-cache-role | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ include "kubeflow-pipelines.cache.serviceAccountName" . }} |
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.
Are you sure that KFP does not break if we change that? Please only make things configurable that are actually configurable.
Signed-off-by: kunal-511 <[email protected]>
Signed-off-by: kunal-511 <[email protected]>

Pull Request Template for Kubeflow Manifests
✏️ Summary of Changes
📦 Dependencies
🐛 Related Issues
✅ Contributor Checklist