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

A few changes to config.set #5226

Merged
merged 2 commits into from Aug 6, 2019
Merged

A few changes to config.set #5226

merged 2 commits into from Aug 6, 2019

Conversation

jcrist
Copy link
Member

@jcrist jcrist commented Aug 5, 2019

The main intent of this PR is to support setting config values through
keyword parameters instead of requiring a dictionary for nested config
values. We follow the same convention as we do for environment
variables, where double-underscores are interpreted as a ., and
indicate an increase in nesting level. Now the following are equivalent:

with dask.config.set({"foo.bar.baz": 1}):
    ...

with dask.config.set(foo__bar__baz=1):
    ...

If a mapping argument and keyword arguments are both provided, the
mapping is applied first, then the keyword arguments (the keywords are
effectively overrides for the mapping).

In the process, I discovered a bug in the rollback code when dealing
with multiple keys interacting with the same nested value. To deal with
this I rewrote the rollback code in a way that was simpler for me to
understand, and hopefully others as well.

  • Tests added / passed
  • Passes black dask / flake8 dask

The main intent of this PR is to support setting config values through
keyword parameters instead of requiring a dictionary for nested config
values. We follow the same convention as we do for environment
variables, where double-underscores are interpreted as a ``.``, and
indicate an increase in nesting level. Now the following are equivalent:

```python
with dask.config.set({"foo.bar.baz": 1}):
    ...

with dask.config.set(foo__bar__baz=1):
    ...
```

If a mapping argument and keyword arguments are both provided, the
mapping is applied first, then the keyword arguments (the keywords are
effectively overrides for the mapping).

In the process, I discovered a bug in the rollback code when dealing
with multiple keys interacting with the same nested value. To deal with
this I rewrote the rollback code in a way that was simpler for me to
understand, and hopefully others as well.
@jcrist
Copy link
Member Author

@jcrist jcrist commented Aug 5, 2019

cc @djhoese, this may interest you.

@jcrist
Copy link
Member Author

@jcrist jcrist commented Aug 5, 2019

Failure is unrelated, and may be fixed by #5228.

@djhoese
Copy link
Contributor

@djhoese djhoese commented Aug 5, 2019

Thanks for the mention. This makes sense to me from a higher level. I'm not super excited about having to mirror the rollback changes to donfig, but that's my fault for not finishing my PRs getting dask on an object oriented config object.

@djhoese
Copy link
Contributor

@djhoese djhoese commented Aug 5, 2019

You mentioned a bug in the rollback, does your new test cover this case?

@jcrist
Copy link
Member Author

@jcrist jcrist commented Aug 6, 2019

Yeah, the issue had to do with multiple keys in a set operation interfering, mixing keywords and the mapping handles this case. FWIW I don't think this is a bug that could easily be hit without allowing mixing mapping and keyword arguments (and still is unlikely to be hit). I find the new rollback code easier to understand, and it's deterministic (we iterate through a list in the reverse order of operations, not through a dict which may be unordered depending on python version).

@jcrist
Copy link
Member Author

@jcrist jcrist commented Aug 6, 2019

@mrocklin, quick review? Planning to merge this later today otherwise.

@mrocklin
Copy link
Member

@mrocklin mrocklin commented Aug 6, 2019

Can I trade you for reviewing dask/distributed#2913 ?

Copy link
Member

@mrocklin mrocklin left a comment

The support of __ looks good to me. The rewrite of writing and unrolling config I understand less, but it all seems reasonable. I'm +1

dask/config.py Show resolved Hide resolved
@jcrist jcrist merged commit ee6b1c6 into dask:master Aug 6, 2019
@jcrist jcrist deleted the config-set-kwargs branch Aug 6, 2019
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.

None yet

3 participants