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

#71 Simplify accelerators config #90

Merged
merged 6 commits into from
Oct 26, 2017
Merged

#71 Simplify accelerators config #90

merged 6 commits into from
Oct 26, 2017

Conversation

wbuchwalter
Copy link
Contributor

@wbuchwalter wbuchwalter commented Oct 25, 2017

  • Add a cloud value that accepts gke or azure and will create the corresponding ConfigMap
  • Add a custom-config.yaml file at the chart's root allowing users to specify a custom config. If cloud is not passed, custom-config.yaml will be used instead.
  • Update README to document this

Remarks:

  • I didn't add a config-file flag, because in order to import a file in a Helm template, this file has to be at the root of the chart. Not doing so will result in a silent error (ConfigMap will just not be created).
    Since this provides a bad user experience, I chose to instead direct users to modify custom-config.yaml that will be already present at the the root of the cart.
  • I updated the README to be as clear as I could, but if you feel I removed to much info let me know.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@k8s-ci-robot
Copy link

Hi @wbuchwalter. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@wbuchwalter
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@wbuchwalter wbuchwalter requested a review from jlewi October 26, 2017 00:01
@jlewi
Copy link
Contributor

jlewi commented Oct 26, 2017

/ok-to-test

Copy link
Contributor

@jlewi jlewi left a comment

Choose a reason for hiding this comment

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

Thanks

README.md Outdated

1. Deploy the operator

For non-RBAC enabled clusters:
```
CHART=https://storage.googleapis.com/tf-on-k8s-dogfood-releases/latest/tf-job-operator-chart-latest.tgz
helm install ${CHART} -n tf-job --wait --replace
helm install ${CHART} -n tf-job --wait --replace --set cloud=<gce or azure>
Copy link
Contributor

Choose a reason for hiding this comment

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

gce->gke

README.md Outdated
```

For RBAC-enabled clusters:
```
CHART=https://storage.googleapis.com/tf-on-k8s-dogfood-releases/latest/tf-job-operator-chart-latest.tgz
helm install ${CHART} -n tf-job --wait --replace --set rbac.install=true
helm install ${CHART} -n tf-job --wait --replace --set rbac.install=true cloud=<gce or azure>
Copy link
Contributor

Choose a reason for hiding this comment

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

gce->gke

README.md Outdated

The TfJob controller can be configured with a list of volumes that should be mounted from the host into the container
to make GPUs work. Here's an example [ControllerConfig](https://github.com/tensorflow/k8s/blob/master/pkg/spec/controller.go):
For **GCE**
Copy link
Contributor

Choose a reason for hiding this comment

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

GKE

README.md Outdated
to make GPUs work. Here's an example [ControllerConfig](https://github.com/tensorflow/k8s/blob/master/pkg/spec/controller.go):
For **GCE**
```
helm install ${CHART} -n tf-job --wait --replace --set cloud=gce
Copy link
Contributor

Choose a reason for hiding this comment

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

gce -> gke

README.md Outdated
```

If the cluster is not hosted on GCE or Azure, you will need specify a custom configuration.
To do so edit `${CHART}\custom-config.yaml` with your desired settings.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think editing the chart is undesirable because it requires the user download/unpack the chart.

What if instead of passing in a file to helm, the user has to create the config map manually. e.g.

kubectl create configmap tf-job-operator-config --from-file=path/to/bar
helm install ${CHART} -n tf-job --wait --replace --set cloud=none

if cloud=none then the helm chart doesn't include the tf-job-operator-config defined in the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Slight difference, is that you don't need to specify any value for cloud. As long as it it neither gke nor azure the chart will not create a ConfigMap.

Copy link
Contributor

@jlewi jlewi left a comment

Choose a reason for hiding this comment

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

Thanks!

@jlewi jlewi merged commit 6d8f9f9 into master Oct 26, 2017
@jhseu jhseu deleted the accelerators-config branch November 3, 2017 23:29
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.

4 participants