Skip to content

Overhaul deprecations system in dask.config#10499

Merged
jrbourbeau merged 2 commits intodask:mainfrom
crusaderky:config.rename
Sep 14, 2023
Merged

Overhaul deprecations system in dask.config#10499
jrbourbeau merged 2 commits intodask:mainfrom
crusaderky:config.rename

Conversation

@crusaderky
Copy link
Copy Markdown
Collaborator

@crusaderky crusaderky commented Sep 8, 2023 •
edited
Loading

The config deprecation system was problematic:

  • dask/dask used check_deprecations, which worked for dask.config.set but not for ~/.config/dask.yaml
  • dask/distributed used rename, which worked for ~/.config/dask.yaml but not for dask.config.set

This PR overhauls and unifies the whole system.
Additionally, it adds support in rename for dot-separated old keys and None values.

Follow-up: dask/distributed#8179

crusaderky
crusaderky commented Sep 8, 2023
View reviewed changes
dask/config.py
--------
dask.config.get
dask.config.set
"""
Copy link
Copy Markdown
Collaborator Author

@crusaderky crusaderky Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliberately not linking this to the sphinx docs

@crusaderky crusaderky self-assigned this Sep 8, 2023
@crusaderky crusaderky changed the title config.rename support for dot-separated keys config.rename support for dot-separated keys and deprecations Sep 8, 2023
@crusaderky crusaderky mentioned this pull request Sep 8, 2023
Merged
@jrbourbeau jrbourbeau changed the title config.rename support for dot-separated keys and deprecations config.rename support for dot-separated keys and deprecations Sep 8, 2023
jrbourbeau
jrbourbeau reviewed Sep 8, 2023
View reviewed changes
dask/config.py Outdated
Comment on lines +609 to +612
warnings.warn(
f"dask config key {o!r} has been renamed to {n!r}",
FutureWarning,
)
Copy link
Copy Markdown
Member

@jrbourbeau jrbourbeau Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a separate check_deprecations method for handling deprecations here -- does this result in multiple warnings being emitted?

Copy link
Copy Markdown
Collaborator Author

@crusaderky crusaderky Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I completely missed that.
This looks like a bit of a mess. dask/dask uses check_deprecations, while dask/distributed uses rename. The two systems are completely independent.

I'll rework this PR to unify them somehow.

Copy link
Copy Markdown
Collaborator Author

@crusaderky crusaderky Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further inspection:

  • dask/dask uses check_deprecations, which works for dask.config.set but not for ~/.config/dask.yaml
  • dask/distributed uses rename, which works for ~/.config/dask.yaml but not for dask.config.set

I overhauled and unified the whole system.
Follow-up: dask/distributed#8179

@crusaderky crusaderky marked this pull request as draft September 8, 2023 14:41
@crusaderky crusaderky mentioned this pull request Sep 11, 2023
Merged
@crusaderky crusaderky marked this pull request as ready for review September 11, 2023 14:59
@crusaderky crusaderky changed the title config.rename support for dot-separated keys and deprecations Overhaul deprecations system in dask.config Sep 11, 2023
@crusaderky
Copy link
Copy Markdown
Collaborator Author

crusaderky commented Sep 11, 2023

@jrbourbeau this PR is ready for review again

@crusaderky
Copy link
Copy Markdown
Collaborator Author

crusaderky commented Sep 13, 2023

@jrbourbeau this is now blocking #10507

@crusaderky crusaderky force-pushed the config.rename branch 2 times, most recently from 81647cb to 6344cf4 Compare September 13, 2023 14:15
@crusaderky crusaderky mentioned this pull request Sep 13, 2023
Merged
@jrbourbeau jrbourbeau changed the title Overhaul deprecations system in dask.config Overhaul deprecations system in dask.config Sep 14, 2023
jrbourbeau
jrbourbeau approved these changes Sep 14, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @crusaderky

@jrbourbeau jrbourbeau merged commit d865459 into dask:main Sep 14, 2023
@crusaderky crusaderky deleted the config.rename branch September 14, 2023 15:45
@BitTheByte BitTheByte mentioned this pull request Sep 16, 2023
Closed
@bnaul
Copy link
Copy Markdown
Contributor

bnaul commented Sep 19, 2023 •
edited
Loading

I think this may have introduced an accidental change in the behavior of dask.config.get (I think dask/dask-kubernetes#816 is referring to the same thing, albeit somewhat cryptically 🙂):

# Previous behavior, 2023.9.1 and earlier
HEAD is now at 3316c38f0 bump version to 2023.9.1
(.venv)  ➜  dask git:(2023.9.1) ✗ python -c 'import dask; print(dask.config.get("array.chunk-size", override_with=None))'
128MiB

# New version after this PR
HEAD is now at 216c7c939 Release 2023.9.2 (#10514)
(.venv)  ➜  dask git:(2023.9.2) ✗ python -c 'import dask; print(dask.config.get("array.chunk-size", override_with=None))'
None

I believe this also contradicts the docstring so I think it's a clear bug and not just a neutral change

If override_with is not None this value will be passed straight back

cc @crusaderky @jrbourbeau @fjetter

edit: opened #10519 to discuss

@bnaul bnaul mentioned this pull request Sep 19, 2023
Closed
@crusaderky crusaderky mentioned this pull request Sep 19, 2023
Merged
@djhoese djhoese mentioned this pull request Mar 10, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjetter fjetter fjetter approved these changes

@jrbourbeau jrbourbeau jrbourbeau approved these changes

Assignees

@crusaderky crusaderky

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@crusaderky @bnaul @fjetter @jrbourbeau