Skip to content

Remove set_config when using default client #7482

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

Merged
merged 5 commits into from
Jan 27, 2023

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Jan 17, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Jan 17, 2023

Unit Test Results

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

       24 files  +       13         24 suites  +13   10h 36m 38s ⏱️ + 6h 58m 22s
  3 329 tests +         1    3 220 ✔️ +       15     106 💤  -      17  3 +3 
39 252 runs  +21 792  37 354 ✔️ +20 699  1 893 💤 +1 088  5 +5 

For more details on these failures, see this check.

Results for commit 8229ff2. ± Comparison against base commit 0161991.

♻️ This comment has been updated with latest results.

@@ -43,7 +43,7 @@ dependencies:
- zict # overridden by git tip below
- zstandard >=0.9.0
- pip:
- git+https://github.com/dask/dask
Copy link
Member Author

@fjetter fjetter Jan 20, 2023

Choose a reason for hiding this comment

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

  • Revert to dask/dask@main

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.

I went ahead and reverted the temporary environment changes since the corresponding PRs in dask/dask have been merged. Overall this looks good to me. The distributed/tests/test_client.py::test_default_get test failure does look related though. From skimming the test, I'm inclined to just remove it. We definitely don't need the config checks anymore and the other checks in that that are, I think, covered over in the dask/dask test suite (though please correct me if that's not the case)

@fjetter
Copy link
Member Author

fjetter commented Jan 24, 2023

Indeed, test_default_get is related but can be easily fixed by removing/replacing the config.get checks

The test_futures_in_subgraphs is a more interesting case and is actually a possible regression introduced in dask/dask.

The test is submitting a c.submit(dd.categorical.categorize, ddf, columns=["day"], index=False) which calculates the result using https://github.com/dask/dask/blob/96a72df0aeb4a7d04844e1ebc2fddb30265a0bc7/dask/dataframe/categorical.py#L152-L154
https://github.com/dask/dask/blob/96a72df0aeb4a7d04844e1ebc2fddb30265a0bc7/dask/base.py#L335-L341
by explicitly proving a cls option, a DataFrame.
dask/dask#9808 changed predecende such that cls wins over an implicit default client, see dask/dask#9808 (comment)

@fjetter
Copy link
Member Author

fjetter commented Jan 24, 2023

See for a change in dask/dask to fix the precedence. This is a regression we introduced in the last release dask/dask#9869

@fjetter fjetter force-pushed the remove_set_config_default_client branch from 72d84ff to adb6433 Compare January 26, 2023 17:10
@fjetter
Copy link
Member Author

fjetter commented Jan 27, 2023

I believe the test failures are unrelated. I'll go ahead and merge since this is quite deterministically failing a test in CI. I'm happy to come back and fix anything that's left

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.

distributed.tests.test_client.test_nested_compute failing
2 participants