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.

Sign up for GitHub

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

Jump to bottom

Share thread pool among P2P shuffle runs #7621

Merged
merged 2 commits into from
Mar 10, 2023
Merged

Share thread pool among P2P shuffle runs #7621

merged 2 commits into from
Mar 10, 2023

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Mar 7, 2023 •
edited
Loading

This PR avoids having many thread pools when multiple shuffles are running at the same time. This also sets the foundation for sharing the thread pool with the buffers.

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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023 •
edited
Loading

Unit Test Results

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

       26 files  ±0         26 suites  ±0   11h 14m 46s ⏱️ - 29m 13s
  3 491 tests ±0    3 386 ✔️ ±0     103 💤 ±0  2 ❌ ±0 
44 123 runs  ±0  42 051 ✔️  - 3  2 069 💤 +3  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit 11fc82b. ± Comparison against base commit 310fc95.

♻️ This comment has been updated with latest results.

@fjetter
Copy link
Member

fjetter commented Mar 8, 2023

cc @wence- in case you have bandwidth for a review

@wence-
Copy link
Contributor

wence- commented Mar 8, 2023

cc @wence- in case you have bandwidth for a review

I can look tomorrow, thanks

wence-
wence- approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

I think this looks fine. The diff is quite large, but it's mostly whitespace changes. The main thing is just promoting the construction of the thread pool.

distributed/shuffle/tests/test_rechunk.py
if barrier_first_worker:
barrier_worker = shuffles[0]
else:
barrier_worker = random.sample(shuffles, k=1)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is only code movement, but I always much prefer destructuring over indexing in these situations:

(barrier_worker,) = random.sample(shuffles, k=1)

But this may not be for everyone.

@fjetter fjetter merged commit 8a3b44c into dask:main Mar 10, 2023
@fjetter fjetter mentioned this pull request Mar 14, 2023
Open
ypogorelova pushed a commit to ypogorelova/distributed that referenced this pull request Mar 16, 2023
@hendrikmakait @ypogorelova
Share thread pool among P2P shuffle runs (dask#7621)
d75e858
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wence- wence- wence- approved these changes

@fjetter fjetter fjetter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hendrikmakait @fjetter @wence-