Executor leaves no background work upon exiting start contextmanager
#2920
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the dask executors could leave work continuuing in the
background upon exit from the `start` contextmanager. This could lead to
odd behavior, where tasks from a flow run might still be running after
`flow.run()` has completed.
This PR changes the semantics of the `start` contextmanager so that no
lingering work remains after an executor has exited the `start`
contextmanager.
- For a `DaskExecutor` with a temporary cluster, we shutdown the
cluster, so no lingering work can persist.
- For a `DaskExecutor` with an external cluster (specified with an
`address`), we stop all pending tasks and wait for all running tasks
to finish.
- For a `LocalDaskExecutor`, we terminate the backing pool as quickly as
possible, then wait for the pool to close.
- For a process pool, this terminates the processes
- For a thread pool, we attempt to interrupt the threads (CPython
specific).
- A `LocalExecutor` runs everything in the main thread, so no background
tasks can persist.
This is a precursor to getting Cancellation to work in a robust way.
The current limits are really old.
cicdw
requested changes
Jul 8, 2020
Member
cicdw
left a comment
There was a problem hiding this comment.
Nice, this feels like a solid enhancement. Left a few minor comments.
Author
|
I believe all comments have been addressed. |
cicdw
approved these changes
Jul 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously when the
executor.start()contextmanager would exit early (say on an interrupt or error), there might be background work still happening, since the executor wouldn't wait for or cancel the remaining submitted tasks.We now attempt to cancel all pending work running on the executor, and wait for tasks that haven't completed in cases where we can't efficiently/robustly cancel them. This ensures that when
executor.start()exits, no tasks will continue to progress.DaskExecutorwith a temporary cluster, we shutdown the cluster, so no lingering work can persist.DaskExecutorwith an external cluster (specified with anaddress) or aninproccluster (using local threads only), we stop all pending tasks and wait for all running tasks to finish.LocalDaskExecutor, we terminate the backing pool as quickly as possible, then wait for the pool to close.LocalExecutorruns everything in the main thread, so no background tasks can persist.This is a precursor for the cancellation implementation (#2771).
Please describe your work and make sure your PR:
changes/directory (if appropriate)docs/outline.tomlfor API reference docs (if appropriate)