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

use async with Client: in tests #6921

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

graingert
Copy link
Member

@graingert graingert commented Aug 19, 2022

This is just the refactors extracted from #6836

This should make it easier to do optimizations like #6920 so when a failure happens we can propagate it quickly and close the Client

  • Tests added / passed
  • Passes pre-commit run --all-files


try:
c = await default_client()
c = default_client()
Copy link
Member Author

Choose a reason for hiding this comment

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

you pretty much never need to await the default client, because it's already started

with dask.config.set({"distributed.comm.timeouts.connect": "10s"}):
assert await asyncio.gather(
run_client(),
run_scheduler_after_2_seconds(),
Copy link
Member Author

Choose a reason for hiding this comment

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

quite a big refactor of this test case because of the client_start_fut = asyncio.ensure_future(c)

@graingert
Copy link
Member Author

async def async_fn():
result = None
with dask.config.set(config):
async with _cluster_factory() as (s, workers), _client_factory(
Copy link
Member Author

Choose a reason for hiding this comment

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

also sorry about adding more difficult to review indentation changes to gen_cluster, but hopefully this is the last one as we can add more stuff to the async with _cluster_factory() as (s, workers), _client_factory( expression

@@ -282,6 +284,7 @@ async def f(s, a, b):


def test_lingering_client_2(loop):
# assert where the client went
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
# assert where the client went
# TODO: assert where the client went

Comment on lines 1022 to 1024
# TODO: `await get_client()` raises a deprecation warning to use
# `async with get_client()` and that will kill the workers' client
# if you do that. We really don't want users to do that.
Copy link
Member Author

Choose a reason for hiding this comment

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

this isn't true anymore - but we still don't want to await get_client()

Suggested change
# TODO: `await get_client()` raises a deprecation warning to use
# `async with get_client()` and that will kill the workers' client
# if you do that. We really don't want users to do that.
# TODO: the existence of `await get_client()` implies the possibility
# of `async with get_client()` and that will kill the workers' client
# if you do that. We really don't want users to do that.
# https://github.com/dask/distributed/pull/6921/

@github-actions
Copy link
Contributor

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   6h 29m 42s ⏱️ + 6m 50s
  3 007 tests +  2    2 923 ✔️ +1    83 💤 +1  1 ±0 
22 255 runs  +16  21 279 ✔️ +8  975 💤 +8  1 ±0 

For more details on these failures, see this check.

Results for commit a10d2b7. ± Comparison against base commit a1d2011.

Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

Changes LGTM

Test failure is #6896

Thank you @graingert !

@fjetter fjetter merged commit b7e184a into dask:main Aug 22, 2022
gjoseph92 pushed a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022
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.

2 participants