Polish Helm charts #76
Conversation
hack/charts/batch-transform-jobs/templates/batch-transform-job.yaml
Outdated
Show resolved
Hide resolved
| @@ -1,7 +1,7 @@ | |||
| apiVersion: v1 | |||
| name: sagemaker-k8s-trainingjob | |||
| version: 0.1.0 | |||
| description: A Helm chart for deploying the SageMaker TrainingJob for Kubernetes. | |||
| description: A Helm chart for deploying a SageMaker TrainingJob from Kubernetes. | |||
There was a problem hiding this comment.
We should add appVersion to all, or none.
There was a problem hiding this comment.
To me it didn't make sense to have an "appVersion" on the jobs, since they don't necessarily correspond 1-to-1 with the controller. They will need to change if we make changes to the CRD specs, but I don't think this will be every release. Thoughts?
There was a problem hiding this comment.
Ah, that makes sense. If anything we should have an appVersion that corresponds to our API types (v1). That way when v2 comes around the switch is easier? Does the helm tool itself use these annotations, or are they for humans?
| The following table lists the configurable parameters for the chart and their default values. | ||
|
|
||
| Parameter | Description | Default | ||
| --- | --- | --- | ||
| `rolesArn` | The EKS service account role ARN | `<PLACEHOLDER>` | ||
| `image.repository` | image repository | `957583890962.dkr.ecr.us-east-1.amazonaws.com/amazon-sagemaker-operator-for-k8s` | ||
| `image.tag` | image tag | `<VERSION>` |
There was a problem hiding this comment.
Should we leave out or complete? I think instead of providing an incomplete table we should just refer them to the spec?
There was a problem hiding this comment.
This is complete for the installer chart. These are the only values in the values.yaml file they are able to modify.
There was a problem hiding this comment.
I left the README out of the specs since I would hope users refer to the documentation for the specification rather than a possibly out-of-date README.
There was a problem hiding this comment.
I see, I got confused between charts.
I left the README out of the specs since I would hope users refer to the documentation for the specification rather than a possibly out-of-date README.
Can you restate? Do you mean to say that you did not include these installation steps in the spec itself?
There was a problem hiding this comment.
I meant I did not include the README.md file in the training/hpo/batch charts since they use the same spec as the YAML charts, so it would be duplication of information that we would need to keep up-to-date.
| continuousParameterRanges: [] | ||
| categoricalParameterRanges: [] |
There was a problem hiding this comment.
This is todo ? @cadedaniel can u confirm that spec will work just that helm chart needs policing.
There was a problem hiding this comment.
Yeah nick made a task for these https://issues.amazon.com/issues/P32170625
| Install the operator using the following command: | ||
|
|
||
| ```bash | ||
| kubectl create namespace sagemaker-k8s-operator-system |
There was a problem hiding this comment.
This should be optional and our installer.yaml should have sagemaker-k8s-operator-system as default namespace.
|
Reminder to not merge this until PR in docs repo |
|
Merging alongside aws/sagemaker-python-sdk#1254 |
Polished the Helm charts to ensure they meet the quality of the latest Helm version.
Biggest change is that namespace, in Helm 3, is controlled at installation time. Users are now required to list
--namespace <namespace>when installing the chart. Therefore our documentation must match this new installation method to ensure the operator runs in an appropriate namespace.Modified all the variables to be in camelCase.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.