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.

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

Replace loop.call_later and loop.add_callback with background tasks added to Server. #6603

Merged
merged 85 commits into from
Jun 23, 2022
Merged

Replace loop.call_later and loop.add_callback with background tasks added to Server. #6603

merged 85 commits into from
Jun 23, 2022

Conversation

graingert
Copy link
Member

Closes #6359
#6478 with conflicts fixed and a few suggestions applied

This PR introduces a TaskGroup class that encapsulates scheduling behavior behind call_soon and call_later methods and allows gracefully shutting down all its tasks at once with a timeout.

Outside of scope:

* Replacing `loop.add_callback` and `loop.call_later` if the call is performed to another loop or the loop is not running.

* Avoiding multiple concurrent calls to `Server.close()`, the fastest one wins as before, now it just cancels the others.


* [x]  Tests added / passed (failing tests are [test_stress_scatter_death #6305](https://github.com/dask/distributed/issues/6305), [flaky test diagnostics/tests/test_progress.py::test_group_timing #6452](https://github.com/dask/distributed/issues/6452))

* [x]  Passes `pre-commit run --all-files`

@hendrikmakait
Add background tasks and rename ongoing_coroutines to ongoing_comm_ha…
33ae2ef 
…ndlers
@hendrikmakait
Replace add_callback and call_later
e680be9
@hendrikmakait
Move close out of background tasks since it would cancel itself
0af0741
@hendrikmakait
Fix issue with non-running IO loop
d9ce34f
@hendrikmakait
Remove deprecated asyncio.coroutine
89f70db
@hendrikmakait
Add delay decorator to delay async function evaluation
76edf23
@hendrikmakait
Replace add_callback in nanny
e415d97
@hendrikmakait
Add docstring and rename to create_background_task
3d688ad
@hendrikmakait
Factor functionality out into TaskGroup and adjust interface to avoid…
d91a4cf 
… unawaited coroutine warning
@hendrikmakait
Rename
8312041
@hendrikmakait
Revert changes to log_event
488b111
@hendrikmakait
Revert changes to lifetime callback
9155235
@hendrikmakait
Fix test
62c9383
@hendrikmakait
Ignore cancelled error when awaiting finished()
96c6346
@hendrikmakait
Fix test
0248587
@hendrikmakait
Fix test for adaptive scaling by adjusting wait condition
3a5695a
@hendrikmakait
Enable tmate for remote debugging of flaking tests
aaa3cd4
@hendrikmakait
Re-raise exception unless cancelled
b271783
@hendrikmakait
Re-raise exception unless cancelled
5459964
@hendrikmakait
Fix '_GatheringFuture exception was never retrieved'
773188a
@hendrikmakait
Catch cancellederror on cancelled gather
734e893
@hendrikmakait
Revert changes to tests.yaml
bc45f99
@hendrikmakait
Replace _ongoing_comm_handlers with TaskGroup
a932b79
@hendrikmakait
Add docstrings
bb9d71b
@hendrikmakait
Improved docs
eda6b73
@hendrikmakait
Improve typing
79970d7
@hendrikmakait
Fix typing of delayed
bdb1b70
@hendrikmakait
Adjust stop()
d3f04a5
@hendrikmakait
Adjust stop()
0532a21
@hendrikmakait
Make call_soon and call_later public
ac53b94
hendrikmakait and others added 11 commits June 8, 2022 19:12
@hendrikmakait
Test idempotency
8873ccb
@graingert
Merge branch 'main' of github.com:dask/distributed into background-tasks
733e419
@graingert
do not return tasks from AsyncTaskGroup.call_soon and AsyncTaskGroup.…
6a308ba 
…call_later

it's very easy to accidentally call "await group.call_soon(set_flag)"
which is always wrong, this is especially bad because some type checkers
will warn you that you didn't await your awaitables here

also this prevents issues like
"task = group.call_soon(set_flag); ... await task" where it's easy to
confuse a cancellation coming from asyncio.current_task().cancel() and
task.cancelled() which should be handled differently
@graingert
avoid deprecated loop kwarg
35fc707
@graingert
don't log periodic callback starts
753c6ce
@graingert
move d.core.delayed into private d.utils._delayed
3902842 
delayed already has a meaning in dask so we should
come up with a new name before making it public
@graingert
remove unused IOLoop import
2cf6e91
@graingert
Merge branch 'main' of github.com:dask/distributed into background-tasks
e53245f
@graingert
back out cancellation changes to inproc
e0ae0f9 
this seems to have some difficult to understand consequences
and should be handled in dask#6548
@graingert
fix ERROR - Failed while closing connection to 'inproc://...': invali…
2714ccc 
…d state
@graingert
remove whitespace/typing import changes to distributed/utils.py
e6a0eeb
@graingert
Copy link
Member Author

seemed to fail with #4498 last time

@graingert
Copy link
Member Author

Tests / test (ubuntu-latest, 3.8, not ci1) : #6045

@graingert
Copy link
Member Author

I think this PR should be squash merged

@github-actions
Copy link
Contributor

github-actions bot commented Jun 21, 2022 •
edited
Loading

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   10h 7m 49s ⏱️ + 2h 59m 38s
  2 890 tests +  8    2 805 ✔️ +12    83 💤  - 1  2 ❌  - 3 
21 408 runs  +56  20 441 ✔️ +59  965 💤 +1  2 ❌  - 4 

For more details on these failures, see this check.

Results for commit fc2f7e1. ± Comparison against base commit 7a0649a.

♻️ This comment has been updated with latest results.

@graingert graingert requested a review from fjetter June 23, 2022 09:09
@fjetter fjetter merged commit 4779f84 into dask:main Jun 23, 2022
@fjetter
Copy link
Member

fjetter commented Jun 23, 2022

Thank you @graingert and @hendrikmakait ! Really looking forward to having this 🎉

@graingert graingert deleted the background-tasks branch June 23, 2022 12:02
@graingert graingert mentioned this pull request Jun 24, 2022
Closed
2 tasks
@crusaderky crusaderky mentioned this pull request Jun 26, 2022
Closed
@crusaderky
Copy link
Collaborator

This PR has caused a major performance degradation in CI: #6632

@fjetter fjetter mentioned this pull request Jun 28, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjetter fjetter fjetter 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.

Replace loop.call_later and loop.add_callback with background tasks
4 participants
@graingert @fjetter @crusaderky @hendrikmakait