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

Overhaul deprecations system in dask.config #10499

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Sep 8, 2023

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

--------
dask.config.get
dask.config.set
"""
Copy link
Collaborator Author

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
@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
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
Member

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
Collaborator Author

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
Collaborator Author

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 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
Collaborator Author

@jrbourbeau this PR is ready for review again

@crusaderky
Copy link
Collaborator Author

@jrbourbeau this is now blocking #10507

@jrbourbeau jrbourbeau changed the title Overhaul deprecations system in dask.config Overhaul deprecations system in dask.config Sep 14, 2023
Copy link
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
41 of 43 checks passed
@crusaderky crusaderky deleted the config.rename branch September 14, 2023 15:45
@bnaul
Copy link
Contributor

bnaul commented Sep 19, 2023

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

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.

4 participants