Make Dask configurable in our configuration #2616

schlunma · 2024-12-12T14:16:30Z

Description

This PR makes Dask configurable in our configuration. These are the default settings:

dask:
  client: {}  # keyword arguments for distributed.Client, can include "address" to use external cluster
  clusters:
    default:
      type: default
    debug:
      type: default
      scheduler: synchronous
    local:
      type: distributed.LocalCluster
      n_workers: 2
      threads_per_worker: 2
      memory_limit: 4GiB
  config: {}  # keyword arguments for dask.config.set
  run: default  # Start the `default` cluster defined above

The entries given under clusters can be selected via the run option; this also works in the command line, e.g.,

esmvaltool run --dask='{"run": "local"}' recipe_example.yml

I chose not to add a predefined SLURMCluster setting since this needs to be customized by the user anyway (account and scratch directory).

This PR also deprecates the usage of the file ~/.esmvaltool/dask.yml (see below).

Closes #2040
Closes #2369

Link to documentation: https://esmvaltool--2616.org.readthedocs.build/projects/ESMValCore/en/2616/quickstart/configure.html#dask-configuration

Deprecation (since v2.12.0, will be removed in v2.14.0) #

Usage of Dask configuration file ~/.esmvaltool/dask.yml: please use the new configuration option dask instead (see details above). If the deprecated file is present, it will be used but a warning will be printed. In this case, all dask configuration options are ignored. To use the new configuration option, delete or move ~/.esmvaltool/dask.yml. To force using the new Dask configuration option and ignore ~/.esmvaltool/dask.yml already now, you can set the environment variable ESMVALTOOL_USE_NEW_DASK_CONFIG to any truthy value.

Before you get started

☝ Create an issue to discuss what you are going to do

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

🧪 The new functionality is relevant and scientifically sound
🛠 This pull request has a descriptive title and labels
🛠 Code is written according to the code quality guidelines
🧪 and 🛠 Documentation is available
🛠 Unit tests have been added
🛠 Changes are backward compatible
🛠 Any changed dependencies have been added or removed correctly
🛠 The list of authors is up to date
🛠 All checks below this pull request were successful

To help with the number pull requests:

🙏 We kindly ask you to review two other open pull requests in this repository

codecov · 2024-12-12T14:45:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.95%. Comparing base (92746cc) to head (282df78).
Report is 106 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2616      +/-   ##
==========================================
+ Coverage   94.93%   94.95%   +0.01%     
==========================================
  Files         252      252              
  Lines       14620    14672      +52     
==========================================
+ Hits        13880    13932      +52     
  Misses        740      740

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

valeriupredoi

looks great, Manu! I would strongly recommend we removed all valid HPC account/user-related information - this is a public space, and that info is confidential in that it can ease hacking. Put some bogus ones instead 👍

valeriupredoi · 2024-12-16T14:14:26Z

doc/quickstart/configure.rst

+      slurm_cluster:
+        type: dask_jobqueue.SLURMCluster
+        queue: shared
+        account: bk1088


I'd remove this as public info, Manu

Done in a40fb69

valeriupredoi · 2024-12-16T14:15:14Z

doc/quickstart/configure.rst

+        memory: 7680MiB
+        processes: 2
+        interface: ib0
+        local_directory: "/scratch/b/b381141/dask-tmp"


valid user name, pls remove this as public information

Done in a40fb69

bouweandela

Very nice to see this finally happening @schlunma!

I just wondered about the format of the configuration file. Would it make sense to be able to specify the configuration parameters per 'cluster'?

How about using the following as a format:

dask:
  use: threaded  # Use the `threaded` profile defined below
  profiles:
    threaded:
      scheduler: threaded
      num_workers: 4
    debug:
      scheduler: synchronous
    local:
      scheduler: distributed.LocalCluster
        n_workers: 2
        threads_per_worker: 2
        memory_limit: 4GiB
    compute:
      scheduler: dask_jobqueue.SLURMCluster
        queue: compute
        account: bd0854
        cores: 128
        memory: 240GiB
        processes: 32
        interface: ib0
        local_directory: /scratch/b/b381141/dask-tmp
        n_workers: 32
        walltime: '2:00:00'
    external:
      scheduler: distributed.Client
         address: "tcp://127.0.0.1:43605"

So all the parameters from the profile selected with use will just be applied using dask.config.set, but we do have a special treatment of the scheduler key if it is not one of the values that are supported by Dask by default, we import the module and instantiate the class.

bouweandela · 2024-12-18T15:25:42Z

doc/quickstart/configure.rst

+    run: default  # This can be omitted
+    clusters:
+      default:
+        type: default


How about renaming this to threaded? That is what is called in the Dask documentation too: https://docs.dask.org/en/stable/scheduler-overview.html#configuring-the-schedulers

Renamed to local_threaded in a40fb69

esmvalcore/config/_config_validators.py

bouweandela · 2024-12-18T15:33:39Z

esmvalcore/config/_dask.py

+            client.dashboard_link,
+        )
+    else:
+        logger.warning(


Suggested change

logger.warning(

logger.info(

Maybe this is fine in many cases?

Done in d52a5ac

bouweandela · 2024-12-18T15:34:00Z

esmvalcore/config/_dask.py

+        )
+    else:
+        logger.warning(
+            "Using Dask default scheduler, checkout "


Maybe rephrase this as:

"Using the Dask threaded scheduler. The distributed scheduler is recommended, please read ..."

Done in d52a5ac

bouweandela · 2024-12-18T15:53:09Z

doc/quickstart/configure.rst

+.. code:: yaml
+
+  dask:
+    run: <NAME_OF_CLUSTER>


Suggested change

run: <NAME_OF_CLUSTER>

use: <NAME_OF_CLUSTER>

Would it be nicer to call this use instead of run?

Done in f9f4315

schlunma · 2024-12-19T09:29:59Z

Very nice to see this finally happening @schlunma!

I just wondered about the format of the configuration file. Would it make sense to be able to specify the configuration parameters per 'cluster'?

How about using the following as a format:

dask:
  use: threaded  # Use the `threaded` profile defined below
  profiles:
    threaded:
      scheduler: threaded
      num_workers: 4
    debug:
      scheduler: synchronous
    local:
      scheduler: distributed.LocalCluster
        n_workers: 2
        threads_per_worker: 2
        memory_limit: 4GiB
    compute:
      scheduler: dask_jobqueue.SLURMCluster
        queue: compute
        account: bd0854
        cores: 128
        memory: 240GiB
        processes: 32
        interface: ib0
        local_directory: /scratch/b/b381141/dask-tmp
        n_workers: 32
        walltime: '2:00:00'
    external:
      scheduler: distributed.Client
         address: "tcp://127.0.0.1:43605"

So all the parameters from the profile selected with use will just be applied using dask.config.set, but we do have a special treatment of the scheduler key if it is not one of the values that are supported by Dask by default, we import the module and instantiate the class.

I'm not quite sure I understand. This doesn't look like valid YAML to me 😬 My idea was that you should be able to use arbitrary kwargs for the scheduler, the client, and dask.config.set to be as flexible as possible.

bouweandela · 2024-12-19T11:25:08Z

I'm not quite sure I understand. This doesn't look like valid YAML to me 😬

🤦 Sorry for the confusion, I tried to write that a bit too fast.

Here is a working YAML file:

dask:
  use: local-threaded  # Use the `local-threaded` profile defined below
  profiles:
    local-threaded:
      scheduler: threaded
      num_workers: 4
    local-distributed:
      cluster:
        type: distributed.LocalCluster
        n_workers: 2
        threads_per_worker: 2
        memory_limit: 4GiB
    debug:
      scheduler: synchronous
    external:
      scheduler-address: "tcp://127.0.0.1:43605"
    dkrz-compute:
      cluster:
        type: dask_jobqueue.SLURMCluster
        queue: compute
        account: bd0854
        cores: 128
        memory: 240GiB
        processes: 32
        interface: ib0
        local_directory: /scratch/b/b381141/dask-tmp
        n_workers: 32
        walltime: '2:00:00'

I like the ability to set Dask configuration parameters, but think it would be better if they could be set per profile instead of as global configuration, therefore I propose to extend the definitions in clusters so they can include Dask configuration parameters. To capture this added option, I think profiles might be better name than clusters, but I'm open to other ideas. So in the example above, you would take the profile indicated by use, pop the cluster key and start a cluster with the provided information, and then pass all the remaining settings to dask.config.set.
use instead of run indicates better which profile will be used, but open to suggestions if you have a better idea
Having 4 (run, client, clusters, and config) options to choose from at the top level seems much. I had a look at the things that can be provided to distributed.Client and most of these options do not look very useful and some can be specified via a dask configuration parameter. It looks like the scheduler-address dask configuration parameter can be used to connect to an existing cluster, and that was the only one we've been using so far. Therefore propose we leave this one out for the moment until someone complains that they need to be able to configure this.
I used different names for the profiles vs the scheduler setting value, to avoid confusion

doc/quickstart/configure.rst

schlunma · 2024-12-19T17:00:24Z

Thanks for your comments @bouweandela! I implemented all your desired changes and tested this thoroughly, everything seems to be working fine! Let me know what you think.

bouweandela

Thanks for all the udpates @schlunma! I've now done a full review of the documentation and suggested a few improvements to the code.

esmvalcore/config/_dask.py

esmvalcore/experimental/recipe.py

doc/quickstart/configure.rst

Co-authored-by: Bouwe Andela <[email protected]>

schlunma · 2024-12-20T13:07:32Z

Thanks for all the udpates @schlunma! I've now done a full review of the documentation and suggested a few improvements to the code.

Thanks for the review! I think I address everything.

bouweandela

🍻

Make dask configurable in config

605b3d1

schlunma added deprecated feature dask related to improvements using Dask labels Dec 12, 2024

schlunma added this to the v2.12.0 milestone Dec 12, 2024

schlunma self-assigned this Dec 12, 2024

schlunma added 2 commits December 12, 2024 15:34

Fixed existing tests

f2dc306

Ignoring existing dask.yml files in tests

79ceb38

schlunma added 3 commits December 12, 2024 16:56

Add doc

06174b6

Add tests

3a02e29

100% coverage

6451ba6

schlunma marked this pull request as ready for review December 12, 2024 17:35

schlunma requested review from bouweandela and valeriupredoi December 12, 2024 17:35

schlunma changed the title ~~Make Dask configurable in configuration~~ Make Dask configurable in our configuration Dec 12, 2024

schlunma and others added 3 commits December 13, 2024 11:51

Add env variable to force using new Dask config

e1acf51

Mention ESMVALTOOL_USE_NEW_DASK_CONFIG in deprecation message

1fb0dec

Merge branch 'main' into configurable_dask

823fa67

valeriupredoi approved these changes Dec 16, 2024

View reviewed changes

bouweandela reviewed Dec 18, 2024

View reviewed changes

schlunma added 2 commits December 19, 2024 12:11

Renamed default cluster from "default" -> "threaded"

0a55fdf

Better message when using default scheduler

d52a5ac

"run" -> "use"

f9f4315

bouweandela reviewed Dec 19, 2024

View reviewed changes

doc/quickstart/configure.rst Outdated Show resolved Hide resolved

schlunma added 4 commits December 19, 2024 16:57

New format of dask config

a40fb69

Use correct scheduler name

b9d2695

Fix doc build

cf47899

Use nested update to update CFG with CLI args

cdfdb7b

bouweandela reviewed Dec 20, 2024

View reviewed changes

schlunma and others added 9 commits December 20, 2024 13:35

Optimize code to get cluster and client

fcbedee

Also consider "scheduler-address"

7d68c5a

Validate dask config in Recipe constructor

ab02a35

"default" -> "threaded"

f6fc37c

Remove unsupported scheduler

28d2ee0

Clearer doc

a536827

Update doc/quickstart/configure.rst

439a4b2

Co-authored-by: Bouwe Andela <[email protected]>

Remove double spaces

64a8a56

Fix RTD build

febec9b

Fix tests

282df78

bouweandela approved these changes Dec 20, 2024

View reviewed changes

bouweandela merged commit 23400b1 into main Dec 20, 2024
7 checks passed

bouweandela deleted the configurable_dask branch December 20, 2024 14:11

schlunma mentioned this pull request Feb 27, 2025

Update release version for v2.12.0 #2678

Merged

10 tasks

schlunma added the config label Jun 5, 2025

Make Dask configurable in our configuration #2616

Make Dask configurable in our configuration #2616

Uh oh!

Conversation

schlunma commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Deprecation (since v2.12.0, will be removed in v2.14.0) #

Uh oh!

codecov bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valeriupredoi left a comment

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!

Choose a reason for hiding this comment

Uh oh!

bouweandela left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlunma commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bouweandela commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

schlunma commented Dec 19, 2024

Uh oh!

bouweandela left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schlunma commented Dec 20, 2024

Uh oh!

bouweandela left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

schlunma commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

bouweandela left a comment •

edited

Loading

schlunma commented Dec 19, 2024 •

edited

Loading

bouweandela commented Dec 19, 2024 •

edited

Loading