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

Add dask config set #10921

Conversation

milesgranger
Copy link
Contributor

@milesgranger milesgranger commented Feb 13, 2024


  • CLI to set config values. ie. dask config set dataframe.query-planning-warning False.

Copy link
Contributor

github-actions bot commented Feb 13, 2024

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

     15 files  ±  0       15 suites  ±0   3h 24m 46s ⏱️ + 5m 59s
 13 040 tests +  8   12 109 ✅ +  8     931 💤 ±0  0 ❌ ±0 
161 287 runs  +120  144 752 ✅ +119  16 535 💤 +1  0 ❌ ±0 

Results for commit 2804fab. ± Comparison against base commit 76b255e.

♻️ This comment has been updated with latest results.

@crusaderky crusaderky self-requested a review February 14, 2024 15:24
Copy link
Collaborator

@crusaderky crusaderky left a comment

Choose a reason for hiding this comment

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

The new command dumps out your whole dask config to ~/.config/dask/dask.yaml.

I would like to treat this as an antipattern, as it will cause many problems down the line when some developer changes a default of some obscure setting but the users don't pick up the change. What's worse, wiping and rebuilding the conda environment won't reflect the change. Also, the user will have no indication, when they do open that config file, about which settings were their own intentional changes from a month before and which were just copy-pasted defaults.

Could you change the code to add the new explicit config override to whatever was already there in ~/.config/dask/dask.yaml, but nothing in addition to that?

Finally, could you document the new command in docs/source/configuration.rst?

dask/dask.yaml Outdated Show resolved Hide resolved
dask/tests/test_cli.py Outdated Show resolved Hide resolved
dask/cli.py Outdated Show resolved Hide resolved
@milesgranger milesgranger force-pushed the milesgranger/10917-dask-expr-deprecation-warning-refactor branch from 90df0ad to b58cdfd Compare February 15, 2024 05:44
@milesgranger
Copy link
Contributor Author

milesgranger commented Feb 15, 2024

Thanks for the review @crusaderky, clearly the CLI will take a bit more work, so I've moved the config option over to #10925.

The new command dumps out your whole dask config to ~/.config/dask/dask.yaml.

Indeed, I wasn't sure if this was correct either but saw this is the same flow being done with coiled config set and thus followed suit. And will anyway address this and the remainder later today.

@milesgranger milesgranger changed the title DataFrame 2.0 deprecation warning rework Add dask config set Feb 15, 2024
@milesgranger milesgranger marked this pull request as draft February 15, 2024 08:30
@milesgranger milesgranger force-pushed the milesgranger/10917-dask-expr-deprecation-warning-refactor branch 2 times, most recently from eb3df34 to 0b40656 Compare February 15, 2024 10:02
@milesgranger milesgranger marked this pull request as ready for review February 15, 2024 10:16
@milesgranger milesgranger force-pushed the milesgranger/10917-dask-expr-deprecation-warning-refactor branch from 0b40656 to de31dd2 Compare February 15, 2024 10:18
dask/cli.py Outdated Show resolved Hide resolved
dask/config.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@crusaderky crusaderky left a comment

Choose a reason for hiding this comment

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

Could you document the new command in docs/source/configuration.rst?
Everything else LGTM

@milesgranger
Copy link
Contributor Author

Could you document the new command in docs/source/configuration.rst?

Ah nuts, sorry I'd forgotten that, done now.

image

@milesgranger milesgranger force-pushed the milesgranger/10917-dask-expr-deprecation-warning-refactor branch from e9f643b to 2804fab Compare February 15, 2024 12:52
@mrocklin
Copy link
Member

cc @jacobtomlinson @ntabris

@ntabris
Copy link
Contributor

ntabris commented Feb 15, 2024

this should link to #9746 rather than #10917, right?

@ntabris
Copy link
Contributor

ntabris commented Feb 15, 2024

The new command dumps out your whole dask config to ~/.config/dask/dask.yaml.

Indeed, I wasn't sure if this was correct either but saw this is the same flow being done with coiled config set

FYI that's not what coiled config set does:

image

@crusaderky crusaderky merged commit 0ad38fc into dask:main Feb 15, 2024
28 checks passed
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.

CLI to get/set dask config values
4 participants