Skip to content

Never use an asynchronous Client when calling top level compute function #11790

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.

Sign up for GitHub

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

Jump to bottom
Merged
phofl merged 8 commits into from
Feb 27, 2025
Merged

Never use an asynchronous Client when calling top level compute function #11790

phofl merged 8 commits into from
Feb 27, 2025

Conversation

@fjetter
Copy link
Member

@fjetter fjetter commented Feb 27, 2025 •
edited
Loading

When using an asynchronous version of the distributed Client, the function dask.compute and Collections.compute methods are not properly working because the Client.get is returning a coroutine object instead of the actual result. This typically crashes with the repacking logic in dask.compute.

Making this just work is not possible from my understanding of asyncio and I see two options. For ordinary users it is best to raise in this situation. For our testing infrastructure it is still nice if there was a well defined behavior. For example, read_parquet is triggering a quantile computation this way regardless of the client used.

xref #11736

@fjetter fjetter changed the title Do not use asynchronous clients in get_scheduler Never use an asynchronous Client when calling top level compute function Feb 27, 2025
@fjetter fjetter force-pushed the ensure_get_scheduler_sync branch from 0d6b643 to 6501572 Compare February 27, 2025 11:28
@github-actions
Copy link
Contributor

github-actions bot commented Feb 27, 2025 •
edited
Loading

Unit Test Results

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

      9 files  ± 0        9 suites  ±0   3h 30m 49s ⏱️ + 2m 57s
 17 808 tests + 3   16 593 ✅ + 2   1 214 💤 ± 0  0 ❌ ±0  1 🔥 +1 
159 346 runs  +27  147 245 ✅ +82  12 099 💤  - 57  1 ❌ +1  1 🔥 +1 

For more details on these errors, see this check.

Results for commit d6d379c. ± Comparison against base commit 5f61e42.

This pull request removes 18 and adds 21 tests. Note that renamed tests count towards both. 
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-False-csv]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-False-hdf]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-False-parquet]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-None-csv]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-None-hdf]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-None-parquet]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-True-csv]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-True-hdf]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-True-parquet]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[True-False-csv]
…

dask.array.tests.test_array_core ‑ test_map_blocks_unique_name_enforce_dim
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-csv]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-hdf]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[False-parquet]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[True-csv]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[True-hdf]
dask.tests.test_distributed ‑ test_blockwise_dataframe_io[True-parquet]
dask.tests.test_distributed ‑ test_client_compute_parquet
dask.tests.test_imports ‑ test_array
dask.tests.test_imports ‑ test_bokeh
…

♻️ This comment has been updated with latest results.

@fjetter
Copy link
Member Author

fjetter commented Feb 27, 2025

There's probably some fallout in the distributed CI. Particularly persist is no longer working as before. persist worked fine for an async client but now it is raising.

hendrikmakait
hendrikmakait reviewed Feb 27, 2025
View reviewed changes
dask/dask-schema.yaml Outdated
type: [string, 'null']
description: |
If False, raise an exception if dask.compute is attempting to select
an asynchronous Client. If True, fall back to a sync scheduler.
Copy link
Member

@hendrikmakait hendrikmakait Feb 27, 2025

Choose a reason for hiding this comment

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

It looks like this is supposed to be boolean, not a string? Also, what happens on null?

Copy link
Member Author

@fjetter fjetter Feb 27, 2025

Choose a reason for hiding this comment

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

That description is off

@fjetter
Copy link
Member Author

fjetter commented Feb 27, 2025

Test failure is unrelated. Nanny couldn't come up due to some windows foo and the test timed out

@phofl phofl merged commit 6038c83 into dask:main Feb 27, 2025
22 of 24 checks passed
@phofl
Copy link
Collaborator

phofl commented Feb 27, 2025

thanks

@fjetter fjetter deleted the ensure_get_scheduler_sync branch February 27, 2025 15:49
@TomAugspurger TomAugspurger mentioned this pull request Feb 28, 2025
Closed
@hendrikmakait hendrikmakait mentioned this pull request Mar 11, 2025
Merged
2 tasks
This was referenced Mar 21, 2025
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hendrikmakait hendrikmakait hendrikmakait left review comments

@phofl phofl phofl approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fjetter @phofl @hendrikmakait