-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat(dask): Dask integration (#821) #821
base: master
Are you sure you want to change the base?
Conversation
bd35dcc
to
aed3588
Compare
aed3588
to
81a79a8
Compare
81a79a8
to
302172f
Compare
302172f
to
5d9c0c0
Compare
5d9c0c0
to
0e74810
Compare
@@ -22,6 +22,16 @@ rules: | |||
- apiGroups: ["metrics.k8s.io"] | |||
resources: ["pods", "nodes"] | |||
verbs: ["get", "list", "watch"] | |||
# Custom dask kubernetes resources |
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.
It would be good to wrap the Dask stuff into a conditional variable such as dask.enabled
-- similarly as we have traefik.enabled
-- because some REANA deployments may not want to (or may not be allowed to) support Dask operators in their deployments.
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.
Also, when dask.enabled
is true, it would be good to deploy Dask operator automatically, in quite the same way as we do for traefik; see the dependencies in Chart.yaml
.
a939c31
to
79e08ba
Compare
79e08ba
to
c3c3a8d
Compare
7873d62
to
7f8fc41
Compare
7f8fc41
to
d97d228
Compare
d97d228
to
c48c45a
Compare
ff5363c
to
4147327
Compare
cf62dfd
to
60c5688
Compare
helm/reana/README.md
Outdated
@@ -60,6 +60,13 @@ This Helm automatically prefixes all names using the release name to avoid colli | |||
| `components.reana_workflow_engine_snakemake.environment` | [REANA-Workflow-Engine-Snakemake](https://github.com/reanahub/reana-workflow-engine-snakemake) environment variables | `{}` | | |||
| `components.reana_workflow_engine_snakemake.image` | [REANA-Workflow-Engine-Snakemake image](https://hub.docker.com/r/reanahub/reana-workflow-engine-snakemake) to use | `docker.io/reanahub/reana-workflow-engine-snakemake:<chart-release-version>` | | |||
| `compute_backends` | List of supported compute backends (kubernetes, htcondorcern, slurmcern) | "kubernetes" | | |||
| `dask.enabled` | Install dask-kubernetes-operator custom resources in the cluster to enable dask workflows | true | |
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.
| `dask.enabled` | Install dask-kubernetes-operator custom resources in the cluster to enable dask workflows | true | | |
| `dask.enabled` | Install dask-kubernetes-operator custom resources in the cluster to support Dask workflows? | false | |
- minor verbiage change;
- dunno if we want to keep it on
true
yet; better to let it onfalse
until the feature is stable.
helm/reana/README.md
Outdated
@@ -60,6 +60,13 @@ This Helm automatically prefixes all names using the release name to avoid colli | |||
| `components.reana_workflow_engine_snakemake.environment` | [REANA-Workflow-Engine-Snakemake](https://github.com/reanahub/reana-workflow-engine-snakemake) environment variables | `{}` | | |||
| `components.reana_workflow_engine_snakemake.image` | [REANA-Workflow-Engine-Snakemake image](https://hub.docker.com/r/reanahub/reana-workflow-engine-snakemake) to use | `docker.io/reanahub/reana-workflow-engine-snakemake:<chart-release-version>` | | |||
| `compute_backends` | List of supported compute backends (kubernetes, htcondorcern, slurmcern) | "kubernetes" | | |||
| `dask.enabled` | Install dask-kubernetes-operator custom resources in the cluster to enable dask workflows | true | | |||
| `dask.cluster_default_cores_limit` | Default cores limit for dask clusters | 4 | |
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.
Cosmetics: s/dask/Dask/g
here and elsewhere, the term is usually written with capital D.
helm/reana/README.md
Outdated
@@ -60,6 +60,13 @@ This Helm automatically prefixes all names using the release name to avoid colli | |||
| `components.reana_workflow_engine_snakemake.environment` | [REANA-Workflow-Engine-Snakemake](https://github.com/reanahub/reana-workflow-engine-snakemake) environment variables | `{}` | | |||
| `components.reana_workflow_engine_snakemake.image` | [REANA-Workflow-Engine-Snakemake image](https://hub.docker.com/r/reanahub/reana-workflow-engine-snakemake) to use | `docker.io/reanahub/reana-workflow-engine-snakemake:<chart-release-version>` | | |||
| `compute_backends` | List of supported compute backends (kubernetes, htcondorcern, slurmcern) | "kubernetes" | | |||
| `dask.enabled` | Install dask-kubernetes-operator custom resources in the cluster to enable dask workflows | true | | |||
| `dask.cluster_default_cores_limit` | Default cores limit for dask clusters | 4 | | |||
| `dask.cluster_max_cores_limit` | Max cores limit for dask clusters | 16 | |
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.
In order to better distinguish between (individual) worker-based specs and (overall) cluster-based specs, what about renaming the values to something like:
dask.cluster_cores
dask.cluster_memory
dask.worker_cores
dask.worker_memory
The similar terms would be closer in the naming, and the fact that some of these are "defaults" we can express in the documentation verbiage.
(Dunno about the word "limit" that we could perhaps leave out or perhaps include, e.g. worker_cores_limit
.)
And, if we expose all these for the user to possibly modify in their reana.yaml
files, then we'd need the user limits for all these:
dask.cluster_cores_max_user_limit
dask.cluster_memory_max_user_limit
dask.worker_cores_max_user_limit
dask.worker_memory_max_user_limit
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.
Example: a user asks for:
resources:
dask:
image: coffeateam/coffea-dask-almalinux8
number_of_workers: 3
single_worker_memory: "3Gi"
We would need just two max_user_limit
values for these, whilst the admin will set desired default values in values.yaml
.
(And if we don't need to specify cores, and Dask uses "auto" detect or some such, then we don't even have to specify them.)
15ff5cf
to
c927f28
Compare
c927f28
to
1d04b4d
Compare
1d04b4d
to
9fefa51
Compare
9fefa51
to
7729cc7
Compare
7729cc7
to
c6faf71
Compare
This pull request contains the ongoing work of dask integration into REANA.