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

Refactor semaphore._Watch into general-purpose Deadline utility #7238

Merged
merged 2 commits into from
Nov 9, 2022

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Nov 2, 2022

This PR extracts some changes from #7184 into a stand-alone PR. The Deadline utility can be used in multiple places where timeout-based cancellation of pipelined operations is desired. For example, Worker.close currently uses its timeout to limit the duration of individual operations which makes it impossible to estimate (or limit) its own total duration.

Note

I'm not entirely sure where to best put this, distributed.utils feels overused and a dedicated new module feels like a bit too much for this. We may want to drop the private scope, but I'd leave it for now to avoid over-exposing API details.

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

cc @fjetter

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

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 41m 21s ⏱️ - 1m 33s
  3 173 tests +  7    3 089 ✔️ +  8    84 💤 ±0  0  - 1 
23 476 runs  +52  22 571 ✔️ +48  905 💤 +5  0  - 1 

Results for commit 69d0cca. ± Comparison against base commit 5a14053.

♻️ This comment has been updated with latest results.

from distributed.metrics import time


class Deadline:
Copy link
Member

Choose a reason for hiding this comment

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

The module name is a bit confusing. Cancellation is a bit too generic for my taste. I'm not sure if this thing warrants a dedicated module

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this thing warrants a dedicated module

Agreed, as noted in the description I'm not sure where to best put this. I'm not a huge fan of cramming everything into distributed.utils either but that might be the best spot for now. Any suggestions?

@fjetter fjetter merged commit 48d0c6b into dask:main Nov 9, 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