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

Fix spans bug when scatter or client_desires_new_key creates a task #7886

Merged
merged 5 commits into from
Jun 7, 2023

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Jun 6, 2023

Of the whole test suite, this use case was triggered (not always) exclusively by test_queue.py::test_race.

@crusaderky crusaderky changed the title Fix spans bug when scatter or client_desires_new_key creates a task #18 Fix spans bug when scatter or client_desires_new_key creates a task Jun 6, 2023
@crusaderky crusaderky marked this pull request as ready for review June 6, 2023 23:15
@crusaderky crusaderky requested a review from fjetter as a code owner June 6, 2023 23:15
@crusaderky crusaderky self-assigned this Jun 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 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   11h 29m 19s ⏱️ - 1h 7m 51s
  3 675 tests +  6    3 564 ✔️ +  4     108 💤 ±0  2 +1  1 🔥 +1 
35 540 runs  +60  33 770 ✔️ +56  1 765 💤 +1  4 +2  1 🔥 +1 

For more details on these failures and errors, see this check.

Results for commit de55a46. ± Comparison against base commit 743826b.

♻️ This comment has been updated with latest results.

Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Thanks, @crusaderky!

distributed/scheduler.py Outdated Show resolved Hide resolved

@gen_cluster(client=True)
async def test_scatter_creates_ts(c, s, a, b):
"""A TaskState object is created by scatter, and only later becomes runnable
Copy link
Member

Choose a reason for hiding this comment

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

OOT: It's mildly scary that this is even legitimate behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be frank I think that the whole scatter design and implementation should be levelled and redone from scratch

Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Thanks, @crusaderky!

crusaderky and others added 3 commits June 7, 2023 09:57
Co-authored-by: Hendrik Makait <hendrik@makait.com>
commit b25afdc6d499a7b2c424579a3378bb0e5849d2e7
Author: crusaderky <crusaderky@gmail.com>
Date:   Wed Jun 7 00:08:06 2023 +0100

    f

commit d75c8faebfc136701b62ace152f04cc9cd9e3427
Author: crusaderky <crusaderky@gmail.com>
Date:   Wed Jun 7 00:02:57 2023 +0100

    Post fine performance metrics to spans

commit 640bda91adfe74874b445a2ed2d389e1b2f8bbd6
Author: crusaderky <crusaderky@gmail.com>
Date:   Wed Jun 7 00:04:01 2023 +0100

    Fix spans bug when scatter or client_desires_new_key creates a task
Merge branch 'main'

Conflicts:
	distributed/scheduler.py
	distributed/spans.py
	distributed/tests/test_spans.py
@crusaderky crusaderky merged commit b52024b into dask:main Jun 7, 2023
@crusaderky crusaderky deleted the spans/scatter branch June 7, 2023 11:50
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