-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
[bitnami/mlflow] Add support for Google Cloud Storage as artifact store #28938
Conversation
Signed-off-by: Nicolas Mauti <[email protected]>
Signed-off-by: Nicolas Mauti <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Signed-off-by: Carlos Rodríguez Hernández <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Thank you @mautini for this contribution! The CI failed for some reason. Let me retry it and I will review the PR. Thanks! |
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 your contribution!! 🎉 I left some suggestions and questions. Could you please look at them?
Also, could you share if you performed some specific validation of the changes on your own env? To better understand the use case as well.
@@ -273,6 +288,11 @@ spec: | |||
{{- if .Values.tracking.persistence.subPath }} | |||
subPath: {{ .Values.tracking.persistence.subPath }} | |||
{{- end }} | |||
{{- if and (include "mlflow.v0.gcs.enabled" .) .Values.externalGCS.useCredentialsInSecret .Values.externalGCS.existingSecret }} | |||
- name: google-application-credentials |
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.
There is no volume called like that, should it be gcs-key
? Or the other way around.
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.
You're right. I took this part from the original MR there: #26461 as I don't use this authentication method. Let's rename it to gcs-key
Co-authored-by: Andrés Bono <[email protected]> Signed-off-by: Nicolas Mauti <[email protected]>
Signed-off-by: Nicolas Mauti <[email protected]>
Hi @andresbono, thanks a lot for your careful review. I applied your suggested changes, responded to your questions and pushed a fix for the mounting of |
Signed-off-by: Bitnami Containers <[email protected]>
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 the additional context!
I just added a few minor suggestions. If you agree with them, you can apply them and the PR is ready to go 🚀
Co-authored-by: Andrés Bono <[email protected]> Signed-off-by: Nicolas Mauti <[email protected]>
All good for me, thanks! |
Signed-off-by: Bitnami Containers <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
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.
LGTM, thank you so much!
…re (bitnami#28938) * Add support for GCS artifact storage Signed-off-by: Nicolas Mauti <[email protected]> * Generate README from values file Signed-off-by: Nicolas Mauti <[email protected]> * Update CHANGELOG.md Signed-off-by: Bitnami Containers <[email protected]> * Update CHANGELOG.md Signed-off-by: Bitnami Containers <[email protected]> * Update CHANGELOG.md Signed-off-by: Bitnami Containers <[email protected]> * Apply suggestions from code review Co-authored-by: Andrés Bono <[email protected]> Signed-off-by: Nicolas Mauti <[email protected]> * Fix keys.json mounting Signed-off-by: Nicolas Mauti <[email protected]> * Update CHANGELOG.md Signed-off-by: Bitnami Containers <[email protected]> * Apply suggestions from code review Co-authored-by: Andrés Bono <[email protected]> Signed-off-by: Nicolas Mauti <[email protected]> * Update CHANGELOG.md Signed-off-by: Bitnami Containers <[email protected]> * Update README.md with readme-generator-for-helm Signed-off-by: Bitnami Containers <[email protected]> --------- Signed-off-by: Nicolas Mauti <[email protected]> Signed-off-by: Bitnami Containers <[email protected]> Signed-off-by: Carlos Rodríguez Hernández <[email protected]> Co-authored-by: Bitnami Containers <[email protected]> Co-authored-by: Carlos Rodríguez Hernández <[email protected]> Co-authored-by: Andrés Bono <[email protected]> Signed-off-by: Michael Faherty <[email protected]>
Description of the change
This PR follows this one #26461 and integration of
google-cloud-storage
library in bitnami mlflow Docker image: bitnami/containers#67246It adds support to Google Cloud Storage as backend store for mlflow artifacts. The use could specify credentials via workload identity or application credential.
Benefits
User will be able to use Google Storage as backend for artifact storage in mlflow tracking. This is useful for other that migrate from another mlflow instance (running with GCS as backend) or that cannot use other cloud provider.
Possible drawbacks
More complexity to the helm chart / configuration? But it's a feature supported by mlflow so I think we have to expose it.
Checklist
Chart.yaml
according to semver. This is not necessary when the changes only affect README.md files.README.md
using readme-generator-for-helm