Skip to content

Conversation

@crusaderky
Copy link
Collaborator

Follow-up from #7997 (comment)

Propagate task cancellation downwards from gather_from_workers to the get-data-from-* tasks. This should hopefully get rid of some of our "task was never awaited" errors.

@crusaderky crusaderky requested a review from fjetter as a code owner August 9, 2023 15:26
# Note: CancelledError and asyncio.TimeoutError are rare conditions
# that can be raised by the network stack.
# See https://github.com/dask/distributed/issues/8006
if isinstance(r, (OSError, asyncio.CancelledError, asyncio.TimeoutError)):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fjetter could you re-review after my latest commit?

If I don't catch this, the assertion on line 115 will raise.
XREF #8013

Copy link
Member

Choose a reason for hiding this comment

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

is this reproducible?

@crusaderky crusaderky requested a review from fjetter August 9, 2023 15:39
@crusaderky crusaderky self-assigned this Aug 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Unit Test Results

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

       20 files  ±0         20 suites  ±0   10h 52m 48s ⏱️ - 14m 11s
  3 766 tests ±0    3 655 ✔️ +4     108 💤 ±0  3  - 4 
35 161 runs  +1  33 441 ✔️  - 1  1 714 💤 +4  6  - 2 

For more details on these failures, see this check.

Results for commit a6c2e94. ± Comparison against base commit 163165b.

♻️ This comment has been updated with latest results.

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.

I'm -1 on catching the CancelledError just for good measure without us knowing how this can actually be provoked.

The impact of an exception here is very different and depending on where it is actually raised. I don't consider this catch safe.

In fact, with return_exceptions=True we should actually makes sure that BaseExceptions generally are reraised

@crusaderky
Copy link
Collaborator Author

@fjetter check now

@hendrikmakait hendrikmakait requested a review from fjetter August 14, 2023 11:53
@fjetter fjetter merged commit 6ce34d8 into dask:main Aug 14, 2023
@crusaderky crusaderky deleted the propagate_gather_cancel branch August 14, 2023 12:02
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