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 flaky test_shuffle_extension.py::test_get_partition #5689

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

gjoseph92
Copy link
Collaborator

@gjoseph92 gjoseph92 commented Jan 24, 2022

A test was assuming the order of the workers passed by gen_cluster matched the order of scheduler.workers, which is not guaranteed.

I reread all the rest of the tests and didn't see any other such potential issues. But I did make a couple small tweaks to improve readability.

`ext.get_output_partition(metadata.id, 7)` was making an assumption about the order of the workers passed by `gen_cluster` matching the order of `scheduler.workers`, which is not guaranteed.
Comment on lines +264 to +267
for addr, ext in exts.items():
if metadata.worker_for(0) != addr:
with pytest.raises(AssertionError, match="belongs on"):
ext.get_output_partition(metadata.id, 0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the change that fixes the flaky test. The rest are just drivebys for readability.

@crusaderky
Copy link
Collaborator

LGTM - however it doesn't close #5688; the issue reports 3 other tests, all around rebalance(), that became flaky.

@gjoseph92
Copy link
Collaborator Author

Let's move the rebalance discussion to #5688 for now.

gjoseph92 added a commit to gjoseph92/distributed that referenced this pull request Jan 24, 2022
gjoseph92 added a commit to gjoseph92/distributed that referenced this pull request Jan 24, 2022
gjoseph92 added a commit to gjoseph92/distributed that referenced this pull request Jan 24, 2022
@crusaderky crusaderky added the flaky test Intermittent failures on CI. label Jan 24, 2022
@gjoseph92
Copy link
Collaborator Author

I'll open separate PRs for other things; I think we should merge this one now.

cc @jrbourbeau

@gjoseph92 gjoseph92 self-assigned this Jan 24, 2022
@crusaderky crusaderky merged commit 4e2c1b3 into dask:main Jan 24, 2022
gjoseph92 added a commit to gjoseph92/distributed that referenced this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky test Intermittent failures on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: P2P shuffle skeleton (#5520) causes test flakiness
2 participants