Centralize configuration by mrocklin · Pull Request #3432 · dask/dask

mrocklin · 2018-04-23T18:38:57Z

This creates a dask.config module to serve as central repository for all configuration in Dask and Dask sub-projects. It includes utilities to find config files (in yaml format), parse environment variables, establish standardized locations for those files, and migrate configurations over time.

Fixes #3367

Tests added / passed
Passes flake8 dask
Fully documented, including docs/source/changelog.rst for all changes
and one of the docs/source/*-api.rst files for new API

martindurant

Several minor comments here.

martindurant · 2018-04-24T19:35:40Z

+from .compatibility import FileExistsError
+
+
+config_paths = [


These appear windows-unfriendly

Yeah, I'm not yet sure what the windows equivalent is for /etc/. This is still a TODO. Wisdom welcome here.

martindurant · 2018-04-24T19:36:17Z

+]
+
+if 'DASK_CONFIG' in os.environ:
+    config_paths.append(os.environ['DASK_CONFIG'])


Should be override, i.e., in position 0?

-edit-
I see below that later wins. Perhaps a comment here, then. Is it a surprise when providing config explicitly like this, that other values are also read, if not overridden?

I get the sense that it's nicer to stack things. This allows, for example, administrators to specify certain low-level configuration values while also letting users add their own. I think that this is the best approach, but would welcome alternatives.

martindurant · 2018-04-24T19:38:26Z

+    return result
+
+
+def collect_yaml(paths=config_paths):


OK to use mutable default?

We don't mutate it or pass it on, so I think so. I've been burned by this before though. If you see a possible concern please let me know.

martindurant · 2018-04-24T19:39:17Z

+    for path in paths:
+        if os.path.exists(path):
+            if os.path.isdir(path):
+                file_paths.extend(sorted([


Looks like a job for glob?

I'm not sure how best to replace this code with glob. It seems more complex than glob's common use.

Perhaps:

file_paths.extend(sorted(chain(*map(glob, ['*.json', '*.yaml', '*.yml']))))

It's also not clear to me what benefit we get from supporting multiple configuration formats. If yaml is the common one, then we already have to rely on a third-party library.

martindurant · 2018-04-24T19:40:51Z

+    # Parse yaml files
+    for path in file_paths:
+        with open(path) as f:
+            data = yaml.load(f.read() or {})


yaml.load(f.read()) or {} - but probably should have try/except (with logger warnings), so syntax errors in the files don't break config.

Indeed. Thanks for the {} fixs .

I'm inclined to fail hard on syntax errors in yaml files. This is something that users opt-in to, so I think it's ok to give them immediate hard feedback.

martindurant · 2018-04-24T19:43:37Z

+        import shutil
+        if not os.path.exists(os.path.dirname(destination)):
+            try:
+                os.mkdir(os.path.dirname(destination))


Maybe prettier to use our own makedirs(path, exists_ok=True), since FileExistsError is a port anyway in py2.

I'll give it a shot

martindurant · 2018-04-24T19:44:57Z

+        tmp = '%s.tmp.%d' % (destination, os.getpid())
+        shutil.copy(source, tmp)
+        try:
+            os.rename(tmp, destination)


Is this necessarily atomic? May need a lock (a file in the directory, in this case) to be certain.

It seems to work well in practice. It's infrequently used (often just on first access of a library). I'm inclined to avoid the complications that come along with file-based locks if we can get away with it (which so far, we have).

martindurant · 2018-04-24T19:46:16Z

+
+
+@contextmanager
+def set_config(arg=None, **kwargs):


I assume this is also thread-unsafe - is it only ever called from the client?

No expectations are made about calling this. Also this isn't just for the distributed scheduler, this is for everything. Locks might be appropriate, I'm not sure. In practice it hasn't been used that much outside of tests.

martindurant · 2018-04-24T19:47:26Z

+    try:
+        yield
+    finally:
+        for key in kwargs:


or config.clear() and for key in old.

Maybe. Currently we don't copy the entire old config, only the changed keys. I'd be happy either way. We'll need to improve this over time anyway to handle nested keys.

martindurant · 2018-04-24T19:47:39Z

+    old = {}
+    for key in kwargs:
+        if key in config:
+            old[key] = config[key]


dask/dask#3432 centralizes configuration in dask/dask. This commit updates dask/distributed for these changes. Notably we centralize all default values in distributed/config.yaml which is now un-commented. We merge it into the global configuration by default on startup.

This builds on dask/dask#3432 to centralize configuration within Dask. This adds a new jobqueue.yaml config file that allows users to specify configuration in a file and have that be used across all of their use of the dask-jobqueue project. This is also something that might allow IT to help standardize configuration options for a given cluster.

mrocklin · 2018-04-30T15:59:29Z

Here are two RFC PRs that show how the proposed system would work under downstream projects:

jacobtomlinson · 2018-05-01T14:33:23Z

I'm curious to know whether appdirs has been considered for setting the config directory?

mrocklin · 2018-05-01T14:36:11Z

#3367 (comment)

…

On Tue, May 1, 2018 at 10:33 AM, Jacob Tomlinson ***@***.***> wrote: I'm curious to know whether appdirs <https://github.com/ActiveState/appdirs> has been considered for setting the config directory? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3432 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszOZkp76AOGwyon3FTbx9whZ5dyBXks5tuHIzgaJpZM4TgaWM> .

jacobtomlinson · 2018-05-01T15:22:02Z

Interesting! So what I gathered from @minrk's comment was that consistency between Mac and Linux wins out over being good platform citizens but for things other than config appdirs is a good way to go?

mrocklin · 2018-05-02T11:43:41Z

@shoyer you might also find this interesting. I'd welcome any feedback.

mrocklin · 2018-05-02T11:44:19Z

To me this feels good enough to merge. I'm going to wait until the release, but I'd like to merge this soon afterwards. Critical feedback on this PR would be very welcome

lesteve · 2018-05-02T15:26:00Z

+    '/etc/dask',
+    os.path.join(sys.prefix, 'etc', 'dask'),
+    os.path.join(os.path.expanduser('~'), '.config', 'dask'),
+    os.path.join(os.path.expanduser('~'), '.dask')


Should ~/.dask not be before ~/.config/dask? IIUC the former is deprecated so I would expect that it does not win against the latter.

For backwards compatibility I think we need to leave this as-is. We might choose to raise a deprecation warning though if there is any non-trivial configuration in ~/.dask/. Or we might choose to just leave it here. It doesn't do much concrete harm, except by spreading around the places to look for config.

It doesn't do much concrete harm, except by spreading around the places to look for config.

I guess the use case I had in mind was:

you have an old config in ~/.dask that you forgot even existed

you are trying to configure something in ~/.config/dask because you have heard that this is the modern way of doing things. This does not have any effect because your config is overriden by the one in ~/.dask

Maybe it is unlikely to happen because people do not tweak dask configuration files very often.

I suggest that we keep ~/.dask/ higher priority, but warn if that directory has any configuration in it. This will keep existing deployments functional, but will also inform them that something needs to change.

Can we have a "list configurations" function? It should give the locations searched, and if anything was found in each, with timestamps.

That's possible, yes, although this is also trivial enough to do that I'm somewhat inclined not to maintain that functionality within Dask, but instead make it clear where the information is and the format that it is in.

lesteve · 2018-05-02T15:33:09Z

+locations:
+
+1.  The ``~/.config/dask`` directory in the user's home directory
+2.  The ``{sys.executable}/etc/dask`` directory local to the Python executable


Maybe sys.prefix rather than sys.executable?

Ah, good catch. Fixed!

lesteve · 2018-05-02T15:59:26Z

+
+*Note: for historical reasons we also look in the ``~/.dask`` directory for
+config files.  This is deprecated and will soon be removed.*
+


Maybe it would be worth showing a snippet from yaml file. You could use scheduler.work_stealing and scheduler.allowed_failures, i.e. the same example as environment variables below or something more complete as https://distributed.readthedocs.io/en/latest/configuration.html#user-wide-configuration, not sure.

Good idea, I've added an example yaml file snippet

jcrist

My main issue with this is the use of a global configuration object. I have a slight preference for moving the global config state and functions into a class, and initing an instance of that class on load. This would consolidate the implementation into a single object, which would be clearer to me what's global and what's not. It also would allow for changing the configuration files after init and reloading them.

This is similar to the hadoop ecosystem, where services subclass Configuration, and applications create an instance of Configuration to use throughout. A super simple version of this can be seen in python here

jcrist · 2018-05-02T20:59:45Z

+    for path in paths:
+        if os.path.exists(path):
+            if os.path.isdir(path):
+                file_paths.extend(sorted([


Perhaps:

file_paths.extend(sorted(chain(*map(glob, ['*.json', '*.yaml', '*.yml']))))

jcrist · 2018-05-02T21:00:50Z

+    for path in paths:
+        if os.path.exists(path):
+            if os.path.isdir(path):
+                file_paths.extend(sorted([


It's also not clear to me what benefit we get from supporting multiple configuration formats. If yaml is the common one, then we already have to rely on a third-party library.

jcrist · 2018-05-02T21:08:43Z

+configs = []
+
+try:
+    import yaml


This logic feels a bit off to me, as functions rely on yaml being defined globally, but the import is hidden in the middle of the file. I'd prefer to see something like:

try: import yaml except ImportError: yaml = None ... if yaml is None: configs.extend(collect_yaml())

mrocklin · 2018-05-03T10:47:46Z

My main issue with this is the use of a global configuration object. I have a slight preference for moving the global config state and functions into a class, and initing an instance of that class on load. This would consolidate the implementation into a single object, which would be clearer to me what's global and what's not. It also would allow for changing the configuration files after init and reloading them.

OK, I'm hearing two possibly separate (although possibly not) ideas here:

Configurations shouldn't be global
We should combine the config dict and the collect/get/set methods into an object

Global

With the current implementation subprojects could use their own config dicts (all functions in dask.config take in an optional config= parameter) but I agree that the design wasn't intended for this multiple separate configurations.

So yes, by convention I'm proposing a single unified configuration space for all Dask projects. I believe that in the end this is simpler. Sub-projects can rely on namespaces within the configuration (see dask/dask-jobqueue#48 for an example) or can mix if they like. I believe that this will be both simpler and more convenient overall, but that's only based on the experience that I've had so far with a limited number of systems.

@jcrist I would be curious to learn more about your experiences dealing with multiple separate configs if you have time.

For context, a single global namespaced config seemed natural to me after dealing with Kubernetes specs and Helm charts, which are probably a strong influence in my design here.

So the current approach is global by convention, but not by requirement.

Combine configuration and functions into an object

Yeah, I can see how that would make sense. This is the kind of situation for which objects were designed. I typically have an aversion to this. I really like that someone can take their Dask config and directly dump it to a file to give to someone else without reading documentation on how Dask Config objects work. With things being global the dask.config module ends up being the organizational structure, which I think works decently.

Other

It also would allow for changing the configuration files after init and reloading them

I think that this is a good idea, and that we can achieve it regardless of which path we choose above

mrocklin · 2018-05-03T10:55:17Z

It's also not clear to me what benefit we get from supporting multiple configuration formats. If yaml is the common one, then we already have to rely on a third-party library.

Because yaml and yml are both somewhat common we have to have a check here. Yaml is a superset of JSON, so my thinking here was "why not". A use case would be that someone sends a machine-generated configuration file over some json-serialized system and dumps it down.

mrocklin · 2018-05-03T10:57:10Z

file_paths.extend(sorted(chain(map(glob, ['.json', '.yaml', '.yml']))))

This doesn't take into account cases where people point us to specific yaml files with DASK_CONFIG. The if-else branch in there makes it slightly trickier to use something like glob. The current code seems readable enough to me (though I have writer's bias) that the for loops don't seem like a big deal.

mrocklin · 2018-05-03T18:10:24Z

@jcrist I believe that the recent commits may address some of your concerns about making it easier to refresh configuration. I would also value feedback on the Downstream Libraries section of the documentation

mrocklin · 2018-05-03T22:34:12Z

There is still some work to do here in replacing dask.context.set_options and dask.context._global. This turns out to be slightly involved (dask/dask uses this namespace somewhat more freely than is typical in dask.config) but seems doable.

I would like to wait on that work for another PR that will follow soon on this one. @jcrist any objections?

mrocklin · 2018-05-03T22:36:55Z

I would like to merge this soon. I will continue to be work on this topic for the next few work-days and so will be responsive to concerns if they arise.

I'm inclined to merge this tomorrow morning (US Eastern time) if no one has any objections and no other questions arise.

This looks through a list of standard locations for config files, parses them, and then adds them to a global dask.config.config dictionary. See dask#3367

These make it clear how to update configuration other than at import time.

This caused them to be dumped to sphinx output

dask/dask#3432 centralizes configuration in dask/dask. This commit updates dask/distributed for these changes. Notably we centralize all default values in distributed/config.yaml which is now un-commented. We merge it into the global configuration by default on startup.

This is a large overhaul of our configuration. It depends on dask/dask#3432 which centralizes configuration code in dask/dask. In particular this does the following: 1. The config dict moves from distributed.config to dask.config.config 2. We now depend on the config values in distributed/distributed.yaml being in the config, allowing us to drop default values that were previously sprinkled around the codebase 3. We change our configuration keys to be more hierarchical, so rather than `{'scheduler-wait-time': ...}` we get values more like `{'scheduler': {'wait-time': ...}}`. 4. We establish a mechanism to migrate old config names to new ones.

This builds on dask/dask#3432 to centralize configuration within Dask. This adds a new jobqueue.yaml config file that allows users to specify configuration in a file and have that be used across all of their use of the dask-jobqueue project. This is also something that might allow IT to help standardize configuration options for a given cluster.

martindurant reviewed Apr 24, 2018

View reviewed changes

mrocklin mentioned this pull request Apr 25, 2018

Configuration files #3367

Closed

mrocklin force-pushed the config branch from 6c2f309 to 921da61 Compare April 25, 2018 10:49

mrocklin mentioned this pull request Apr 30, 2018

Overhaul configuration dask/distributed#1948

Merged

mrocklin mentioned this pull request Apr 30, 2018

Add configuration file dask/dask-jobqueue#48

Merged

mrocklin changed the title ~~[WIP] Add standard configuration file handling~~ Add standard configuration file handling Apr 30, 2018

mrocklin changed the title ~~Add standard configuration file handling~~ Centralized configuration May 1, 2018

lesteve reviewed May 2, 2018

View reviewed changes

jcrist reviewed May 2, 2018

View reviewed changes

mrocklin force-pushed the config branch from b9e1ac1 to 125c405 Compare May 4, 2018 11:39

mrocklin added 3 commits May 4, 2018 08:50

Add standard configuration file handling

abb07d3

This looks through a list of standard locations for config files, parses them, and then adds them to a global dask.config.config dictionary. See dask#3367

add config.get function

e600d99

add configuation documentation

779e576

mrocklin added 18 commits May 4, 2018 08:50

skip config doctests

1f2d2d4

clean up docs and use of set_config

75a68fd

use deepcopy within set_config

b402efd

clean up config destination file handling

efd0c94

avoid excess endlines

82284e3

Create directory and then file

5b751b5

rename set_config to set

078bee6

Support using dask.config.set without a context manager

b73116b

Add dask.config.rename

2a3a0b3

Support nested access in environment variables

aaf31b0

update config documentation

4cef760

[skip ci] sys.executable -> sys.prefix

dbd788a

add yaml file example to docs

316b497

set yaml to None if not available

6034de7

fix yaml none issue

6596fc2

Add dask.config.refresh and dask.config.collect functions

2c757aa

These make it clear how to update configuration other than at import time.

add downstream libraries documentation section

01e8040

avoid placing os.environ in parameters

887fffa

This caused them to be dumped to sphinx output

mrocklin force-pushed the config branch from 125c405 to f22f1fd Compare May 4, 2018 12:50

Add distributed nesting to environment config docs

c40d27a

mrocklin force-pushed the config branch from f22f1fd to c40d27a Compare May 4, 2018 12:51

mrocklin changed the title ~~Centralized configuration~~ Centralize configuration May 5, 2018

mrocklin mentioned this pull request May 5, 2018

Configuration dask/dask-kubernetes#75

Merged

mrocklin merged commit 5c2787f into dask:master May 5, 2018

mrocklin deleted the config branch May 5, 2018 23:09

lesteve mentioned this pull request May 7, 2018

Use released version of distributed. dask/dask-jobqueue#54

Merged

jrbourbeau mentioned this pull request Jan 14, 2022

Refactor config default search path retrieval #8573

Merged


		*Note: for historical reasons we also look in the ``~/.dask`` directory for
		config files. This is deprecated and will soon be removed.*



		@contextmanager
		def set_config(arg=None, **kwargs):

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

martindurant 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!

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!

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!

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!

mrocklin commented Apr 30, 2018

Uh oh!

jacobtomlinson commented May 1, 2018

Uh oh!

mrocklin commented May 1, 2018 via email

Uh oh!

jacobtomlinson commented May 1, 2018

Uh oh!

mrocklin commented May 2, 2018

Uh oh!

mrocklin commented May 2, 2018

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!

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

mrocklin commented Apr 23, 2018 •

edited

Loading