[stable/dask] Add updated Dask chart by mrocklin · Pull Request #2914 · helm/charts

mrocklin · 2017-12-02T19:30:24Z

This creates a new Dask chart that replaces the dask-distributed chart. In addition to a slimmed down name this uses more modern docker images, supports environment customization through environment variables, links cpu and memory limits to dask cpu and memory limits, ...

See #2912

cc @danielfrg

k8s-ci-robot · 2017-12-02T19:30:26Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment 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 you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Details

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.

mrocklin · 2017-12-02T20:17:45Z

Related changes to docker image here: dask/dask-docker#5

mrocklin · 2017-12-04T13:57:35Z

@danielfrg if you have time to review the changes here and in dask/dask-docker#6 I would appreciate it.

Charts maintainers, I'm curious about how best to go about smoothly renaming a chart. I would like to rename this from dask-distributed to dask if possible.

I am also curious about how best to decompose this chart so that it can be used either with or without the Jupyter notebook service. My current thought is to create a dask-core chart that does not include Jupyter, and then have the Dask chart depend on the dask-core chart. I am unfamiliar with dependencies though, and so I may be doing something naive here.

I'm running the CLA past the legal department of my company. I hope for a response back in a day or so.

mrocklin · 2017-12-04T15:14:39Z

@k8s-ci-robot I have signed the CLA

danielfrg · 2017-12-04T16:41:55Z

Changes look good, I cannot test today but I will try to test tomorrow. Maybe lets merge the docker image one to it gets build on dockerhub first.

unguiculus · 2018-01-11T20:04:23Z

What's the reason for this? You rename the chart and decrease the version? Seems weird.

Happy to increase the version if desired. I decreased the version to 1.0 because of the name change. I'm happy to adhere to any standard though.

unguiculus · 2018-01-11T20:05:51Z

Should be a stable version.

unguiculus · 2018-01-11T20:07:47Z

@kubernetes/charts-maintainers The chart is about to be renamed. Is this something we can/want to handle?

mrocklin · 2018-01-11T20:09:25Z

There has been a fair amount of work on this over at https://github.com/dask/helm-chart and things have cleaned up a bit. If review is restarting let me know and I'll push more up.

unguiculus · 2018-01-13T14:23:34Z

@mrocklin Just keep changes coming. It probably doesn't make sense to review now if you push more substantial changes.

mrocklin · 2018-01-16T15:47:18Z

cc @jacobtomlinson I'd like to make you aware of this. After merging current efforts you may want to consider pushing the result upstream to the stable repo here.

unguiculus · 2018-01-16T16:22:00Z

FYI: Renaming the chart is not as easy. We'd have to keep the existing one and deprecate it. Think about if you really want this.

mrocklin · 2018-01-16T17:33:03Z

Thanks for the comment @unguiculus

Deprecating and creating a new one still sounds ideal to me. I may not be aware of maintenance costs of keeping around a deprecated chart though.

prydonius · 2018-01-16T17:48:59Z

Once the chart is deprecated there are no maintenance costs. Be aware that Helm doesn't really treat deprecated charts in any special way and e.g. a helm search dask will show both charts. When deprecating dask-distributed, we can update the description to begin with "DEPRECATED" so it's more obvious to users.

We need a separate PR to deprecate dask-distributed that:

sets deprecated: true in Chart.yaml
prepends "DEPRECATED" to the description in Chart.yaml
Updates the README and NOTES.txt to indicate that it's deprecated and stable/dask should be used instead

This PR should be updated to just add the new dask chart and not remove dask-distributed.

@danielfrg are you happy with renaming this?

danielfrg · 2018-01-16T18:26:44Z

Yes

This supercedes the previous dask-distributed chart, which will be deprecated. This also uses an accurate docker image for the Jupyter server, and pins docker images generally.

mrocklin · 2018-02-14T19:42:45Z

I've updated this PR to add a new dask chart. I'll submit a PR deprecating dask-distributed after (if) this gets merged. Feedback is generally quite welcome on the structure of the chart here.

mrocklin · 2018-02-14T19:43:26Z

@@ -0,0 +1,12 @@
+name: dask
+version: 0.17.0


Currently I'm pinning the version of the chart to the version of the main software package. I'm also happy to go directly to 1.0.0 and follow semver if preferred

Add appVersion which should be 0.17.0. The chart is versioned independently.

unguiculus

Please read our review guidelines again:
https://github.com/kubenetes/charts/blob/master/REVIEW_GUIDELINES.md

Make sure you configure things as suggested there (e. g. images, resources).

If you use helm create with Helm 2.8+ you'll get an initial setup with current best practices applied.

unguiculus · 2018-02-18T11:00:23Z

@@ -0,0 +1,12 @@
+name: dask
+version: 0.17.0


Add appVersion which should be 0.17.0. The chart is versioned independently.

mrocklin · 2018-03-02T20:00:50Z

OK, I'm currently a bit stuck on a couple things:

Despite linting fine my my local environment doesn't run this well. I don't understand the error messages it's giving me, nor have I yet been able to find suitable information from web searches.

 mrocklin@carbon:~/workspace/charts/stable/dask$ helm install .
 Error: release jumpy-heron failed: Deployment in version "v1beta2" cannot be handled as a Deployment: v1beta2.Deployment: Spec: v1beta2.DeploymentSpec: Template: v1.PodTemplateSpec: Spec: v1.PodSpec: Containers: []v1.Container: v1.Container: Image: ReadString: expects " or n, parsing 670 ...,"image":{... at {"apiVersion":"apps/v1beta2","kind":"Deployment","metadata":{"labels":{"app":"dask","chart":"dask-1.0.0","component":"jumpy-heron-jupyter-notebook","heritage":"Tiller","release":"jumpy-heron"},"name":"jumpy-heron-dask-jupyter","namespace":"default"},"spec":{"replicas":1,"strategy":{"type":"RollingUpdate"},"template":{"metadata":{"labels":{"app":"dask","component":"jumpy-heron-jupyter-notebook","release":"jumpy-heron"}},"spec":{"containers":[{"env":[{"name":"EXTRA_CONDA_PACKAGES","value":null},{"name":"EXTRA_PIP_PACKAGES","value":null},{"name":"EXTRA_APT_PACKAGES","value":null},{"name":"DASK_SCHEDULER_ADDRESS","value":"jumpy-heron-dask-scheduler:8786"}],"image":{"pullPolicy":"IfNotPresent","repository":"daskdev/dask-notebook","tag":"0.17.1"},"name":"jumpy-heron-dask-jupyter","ports":[{"containerPort":8888}],"resources":{},"volumeMounts":[{"mountPath":"/home/jovyan/.jupyter","name":"config-volume"}]}],"volumes":[{"configMap":{"name":"jumpy-heron-dask-scheduler-config"},"name":"config-volume"}]}}}}

I want to drop in a template containing a map in the midst of an ongoing map

          env:
            - name: DASK_SCHEDULER_ADDRESS
               value: {{ template "dask.fullname" . }}-scheduler:{{ .Values.scheduler.servicePort }}
{{ toYaml .Values.jupyter.env | indent 12 }}

If anyone has any time I would value advice on either of these particular issues.

There are also the issues raised by @jacobtomlinson . I would love to find a way to turn off the Jupyter server. Does anyone have any suggestions or examples of completely turning off a segment of the chart completely?

jacobtomlinson · 2018-03-02T20:10:50Z

In terms of turning off segments see the rbac section in zero to jupyterhub as an example. The whole template is wrapped in an if statement that just checks a boolean from the config file.

Also, combine all jupyter documents into one file

mrocklin · 2018-03-02T20:17:09Z

Ah, that seems simple enough. Done.

unguiculus · 2018-03-03T10:11:03Z

+    c = get_config()
+    c.NotebookApp.password = '{{ .Values.jupyter.password }}'
+
+---


Please create separate files per resource.

mrocklin · 2018-03-04T13:59:09Z

I remain stuck on these two issues: #2914 (comment)

I've raised one as a question upstream: helm/helm#3602

If anyone has thoughts on how to resolve these I would welcome advice, pointers to documentation etc..

If people have looked at these and genuinely don't know if there is a solution then that would be good feedback as well. Alternatives would be welcome.

unguiculus · 2018-03-04T16:30:34Z

I'll try and have a look.

mrocklin · 2018-03-04T18:51:34Z

Thank you for interceding here @unguiculus . This works flawlessly for me locally.

Looking at your changes, was the issue that calling toYaml on an empty mapping was generating something like {} rather than empty lines?

mrocklin · 2018-03-04T18:56:16Z

I'm going to take another pass over the README and NOTES. Is there anything else that's necessary here for a 1.0.0 release?

mrocklin · 2018-03-06T13:05:21Z

Just checking in here. I believe all feedback so far has been handled (except for some of @jacobtomlinson's comments, which I think can probably wait for future enhancements). Is there anything else I should be doing here to move this forward?

also cc @yuvipanda in case he wants to weigh in.

unguiculus · 2018-03-06T19:58:40Z

+    heritage: {{ .Release.Service | quote }}
+    release: {{ .Release.Name | quote }}
+    chart: {{ template "dask.chart" . }}
+    component: "{{ .Release.Name }}-{{ .Values.jupyter.component }}"


Let me ask again why you make components configurable. Also, they should not be tied to a release. I'd just hard-code them: jupyter, worker

Thanks for the nudge. Resolved. My apologies for letting that slip earlier.

To answer the direct question of why was this here. My answer is "I don't know". This PR is a copy-paste-modify job of the current chart in stable/dask-distributed, that can hopefully be deprecated once this cleaner version is in.

unguiculus · 2018-03-08T17:16:46Z

/ok-to-test

unguiculus · 2018-03-08T17:20:56Z

/lgtm

k8s-ci-robot · 2018-03-08T17:21:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrocklin, unguiculus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [unguiculus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mrocklin · 2018-03-08T17:29:31Z

Thank you for your patient support everyone. I'm very glad to see this in.

* Add Dask Chart This supercedes the previous dask-distributed chart, which will be deprecated. This also uses an accurate docker image for the Jupyter server, and pins docker images generally. * Comment out resource limits * Set chart version to 1.0.0, appVersion to 0.17.0 * Update README and chart information * specify nthreads and memory-limit explicitly in values.yaml * update notes * avoid setting --nthreads and --memory-limit if no resource limits * expand truncation to 63 characters, and trim "." * bump app version number * remove change to dask-distributed chart * remove NOTES.txt * use explicit repository and image tags * respond to feedback * remove specification of containerPorts * update README * resolve linting issues in values.yaml * Clarify extra packages in README * replace pip/conda/apt packages with generic env solution * Add option to turn off Jupyter Also, combine all jupyter documents into one file * add missing {{ end }} * split dask-jupyter to multiple files * Fix Yaml issues * Fix values.yaml * Update README with instructions for installing custom packages * Remove configurable components

* Add Dask Chart This supercedes the previous dask-distributed chart, which will be deprecated. This also uses an accurate docker image for the Jupyter server, and pins docker images generally. * Comment out resource limits * Set chart version to 1.0.0, appVersion to 0.17.0 * Update README and chart information * specify nthreads and memory-limit explicitly in values.yaml * update notes * avoid setting --nthreads and --memory-limit if no resource limits * expand truncation to 63 characters, and trim "." * bump app version number * remove change to dask-distributed chart * remove NOTES.txt * use explicit repository and image tags * respond to feedback * remove specification of containerPorts * update README * resolve linting issues in values.yaml * Clarify extra packages in README * replace pip/conda/apt packages with generic env solution * Add option to turn off Jupyter Also, combine all jupyter documents into one file * add missing {{ end }} * split dask-jupyter to multiple files * Fix Yaml issues * Fix values.yaml * Update README with instructions for installing custom packages * Remove configurable components Signed-off-by: voron <av@arilot.com>

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 2, 2017

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2017

mrocklin force-pushed the update-dask branch from d54cdcf to 955e9d2 Compare December 4, 2017 13:54

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 4, 2017

mrocklin mentioned this pull request Dec 4, 2017

[skip ci] Add Cloud deployment documentation dask/distributed#1611

Closed

unguiculus suggested changes Jan 11, 2018

View reviewed changes

unguiculus self-assigned this Jan 11, 2018

Add Dask Chart

50028bb

This supercedes the previous dask-distributed chart, which will be deprecated. This also uses an accurate docker image for the Jupyter server, and pins docker images generally.

mrocklin force-pushed the update-dask branch from 4df7c74 to 50028bb Compare February 14, 2018 19:41

mrocklin commented Feb 14, 2018

View reviewed changes

unguiculus suggested changes Feb 18, 2018

View reviewed changes

Comment out resource limits

346d3bd

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 2, 2018

Add option to turn off Jupyter

00fdc2c

Also, combine all jupyter documents into one file

add missing {{ end }}

322bce7

unguiculus suggested changes Mar 3, 2018

View reviewed changes

split dask-jupyter to multiple files

101c431

mrocklin mentioned this pull request Mar 3, 2018

Dask-distributed cloud issues #2912

Closed

unguiculus added 2 commits March 4, 2018 18:03

Fix Yaml issues

4cc418a

Fix values.yaml

d73298f

Update README with instructions for installing custom packages

db2232b

unguiculus suggested changes Mar 6, 2018

View reviewed changes

Remove configurable components

4ef697a

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2018

k8s-ci-robot merged commit 3ac741b into helm:master Mar 8, 2018

mrocklin deleted the update-dask branch March 8, 2018 17:29

mrocklin mentioned this pull request Mar 9, 2018

Deprecate dask-distributed chart #4083

Merged

Conversation

mrocklin commented Dec 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 2, 2017

Uh oh!

mrocklin commented Dec 2, 2017

Uh oh!

mrocklin commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Dec 4, 2017

Uh oh!

danielfrg commented Dec 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unguiculus commented Jan 11, 2018

Uh oh!

mrocklin commented Jan 11, 2018

Uh oh!

unguiculus commented Jan 13, 2018

Uh oh!

mrocklin commented Jan 16, 2018

Uh oh!

unguiculus commented Jan 16, 2018

Uh oh!

mrocklin commented Jan 16, 2018

Uh oh!

prydonius commented Jan 16, 2018

Uh oh!

danielfrg commented Jan 16, 2018

Uh oh!

mrocklin commented Feb 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unguiculus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrocklin commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacobtomlinson commented Mar 2, 2018

Uh oh!

mrocklin commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrocklin commented Mar 4, 2018

Uh oh!

unguiculus commented Mar 4, 2018

Uh oh!

mrocklin commented Mar 4, 2018

Uh oh!

mrocklin commented Mar 4, 2018

Uh oh!

mrocklin commented Mar 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unguiculus commented Mar 8, 2018

Uh oh!

unguiculus commented Mar 8, 2018

Uh oh!

k8s-ci-robot commented Mar 8, 2018

mrocklin commented Dec 2, 2017 •

edited

Loading

mrocklin commented Dec 4, 2017 •

edited

Loading

mrocklin commented Mar 2, 2018 •

edited

Loading