Skip to content
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 katib to Helm Chart and update example/helm #8

Merged
merged 10 commits into from
Sep 27, 2024
Merged

Conversation

kromanow94
Copy link
Owner

Description

I discussed with @doncorsean that I wanted to do some changes and testing for his original PR in #6. We aligned that I'll take his branch and put my changes on it top of it to honor his contribution and move forward.

I did some changes and testing and from the general perspective it seems to be working but we need a closer look. I asked @MaxKavun from my team to have a look at what's happening here, compare what we have with what we expect and possibly make some changes for formatting, parameterization and readability.

Other contributions in form of testing, suggestions, or even PRs to this PR are also welcomed!

Things already included

  • katib manifests to the Kubeflow Helm Chart.
  • CRDs for Experiments, Suggestions, Trials to the Kubeflow Helm Chart.
  • Example katib experiment in example/helm/katib-experiment.random.yaml.
  • values.yaml for MySQL contains initdbScripts that adds katib databse.
  • Configured katib dbmanager to use the same MySQL Instance as for Kubeflow in example/helm/values.kubeflow.yaml but with different database.

Todo

  • Have a look at the conventions, maybe there is something that might be improved.
  • Not all manifests are properly parameterized (charts/kubeflow/templates/katib/user-roles/*, maybe more).
  • Have a look at the values.yaml::katib.config - cleanup needed and possibly there are some differences between upstream 1.8.0 and this.
  • Have a general and criticizing look at everything because it's a lot and it's easy to miss something.

@kromanow94 kromanow94 mentioned this pull request Sep 16, 2024
1 task
@@ -19,6 +19,21 @@ pipelines:
secretKeyRef:
name: mlpipeline-minio-artifact

katib:
dbmanager:
config:
Copy link
Collaborator

@MaxKavun MaxKavun Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to create default db configuration since we use the same secrets/host for

  1. pipeline component
  2. katib

And add ability to override for each component ? I see this is quite common approach to avoid repeating the same configs

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea but let's handle that as a separate issue.

#9

@@ -4,3 +4,25 @@ auth:
existingSecret: db-credentials
commonLabels:
sidecar.istio.io/inject: "false"

initdbScripts:
Copy link
Collaborator

@MaxKavun MaxKavun Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also not sure if this is right approach to tight such scripts with database deployment itself

Bitnami mentions it's only for fresh instance and first boot, should we consider a separate Job for database initialisation instead ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for example configuration. Having this in mind, it should be fine. Our main product is the Helm Chart and the example configuration is just for reference. But, we should document this.

I created an issue for documentation here: #10

@kromanow94
Copy link
Owner Author

Please also generate the kubeflow/manifests for Kubeflow 1.8 with Kustomize and have a look if there any differences between the generated Helm Chart and the generated manifests Kustomize.

You can use this example kustomization.yaml to generate the manifest directly from upstream:

$ cat kustomization.yaml 
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization

resources:
- github.com/kubeflow/manifests/example?ref=v1.8.1
$ kustomize build . > kubeflow-manifests-aio-1.8.1.yaml

* Update cluster roles

* Fix a few indentations

* fix yaml complaints

* Add condition to execute script only on primary node

* Update template name and fix the bug

* Remove check on script since it's only for replication architecture; fix secret refences
@kromanow94
Copy link
Owner Author

@MaxKavun can you just fix the CICD for Lint and Test Charts / list-tests? After that we can squash and merge!

@MaxKavun MaxKavun merged commit 9824c39 into helmcharts Sep 27, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants