Skip to content

Partial review of task streams#9230

Open
crusaderky wants to merge 1 commit intodask:mainfrom
crusaderky:taskstream
Open

Partial review of task streams#9230
crusaderky wants to merge 1 commit intodask:mainfrom
crusaderky:taskstream

Conversation

@crusaderky
Copy link
Copy Markdown
Collaborator

@crusaderky crusaderky commented Apr 29, 2026

Quick and dirty cleanup of the taskstream functionality.

  • The context manager now embeds a very necessary time adjustment to avoid missing tasks. This is ugly and bug prone. It should be fixed properly by parsing the index, but that would be a much more invasive change.
  • The async context manager was untested and broken. Fixed.
  • Fixed flakiness in test_get_task_stream_save, which was caused by using the context manager without the 0.1s offset.



@gen_cluster(client=True)
async def test_no_startstops(c, s, a, b):
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a completely artificial situation. worker_state_machine.TaskFinishedMsg and TaskErredMsg ensure that there are always startstops.

# Other methods require `kwargs` to have a non-empty list of `startstops`
return
if start == "processing" and finish in ("memory", "erred"):
assert kwargs["startstops"]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

worker_state_machine.TaskFinishedMsg and TaskErredMsg ensure that there are always startstops.

left = mid + 1
else:
right = mid
return left
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

functionally identical to before but marginally faster

Comment thread distributed/client.py
Comment on lines +6258 to +6260
# Smooth over time differences of client vs. workers
# FIXME this is very crude. We should query TaskStreamPlugin.index instead.
self.start = time() - 0.1
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is ugly and bug prone. It should be fixed properly by parsing the index but that would be a much more invasive change, so I skipped it.

Comment thread distributed/client.py
self._filename = filename
self.figure = None
self.client = client or default_client()
self.client.get_task_stream(start=0, stop=0) # ensure plugin
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was doing nothing in the async client.

Comment thread distributed/client.py

# Smooth over time differences of client vs. workers
# FIXME this is very crude. We should query TaskStreamPlugin.index instead.
self.start = time() - 0.1
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was completely missing, making the async context manager broken.

@crusaderky crusaderky added bug Something is broken flaky test Intermittent failures on CI. labels Apr 29, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 29, 2026

Unit Test Results

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

    31 files  ± 0      31 suites  ±0   11h 8m 2s ⏱️ - 6m 1s
 4 125 tests + 1   4 018 ✅ + 3    104 💤 ±0  3 ❌  - 2 
59 813 runs  +15  57 335 ✅ +16  2 474 💤 ±0  4 ❌  - 1 

For more details on these failures, see this check.

Results for commit 6afdcb1. ± Comparison against base commit f431280.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.
distributed.diagnostics.tests.test_task_stream ‑ test_no_startstops
distributed.diagnostics.tests.test_task_stream ‑ test_client_ctx
distributed.diagnostics.tests.test_task_stream ‑ test_client_ctx_sync

♻️ This comment has been updated with latest results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is broken flaky test Intermittent failures on CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant