Skip to content

Enable Client.wait_for_workers to optionally also wait when removing workers #6377

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jacobtomlinson
Copy link
Member

Closes #6374

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

Adds an absolute kwarg to Client.wait_for_workers that makes it wait for exactly n_workers instead of the current behaviour of waiting for at least n_workers. Default is False to continue with current behaviour.

No strong feelings about the kwarg name here, I chose absolute but something like exact would be fine too.

@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2022

Unit Test Results

       15 files  ±  0         15 suites  ±0   7h 16m 39s ⏱️ +15s
  2 802 tests  -   2    2 724 ✔️  -   1    78 💤  - 1  0 ±0 
20 777 runs   - 17  19 856 ✔️  - 16  921 💤  - 1  0 ±0 

Results for commit 88ec08d. ± Comparison against base commit 4d29246.

♻️ 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 think this feature overall makes sense but I'm wondering about the API. The behavior so far was basically a min and what you're proposing now is an equal but maybe it should be a max? (I don't have a strong opinion, just asking the question)

@jacobtomlinson
Copy link
Member Author

jacobtomlinson commented May 20, 2022

Thanks for the review @fjetter. Yeah, I also thought about min and max options and how n_workers basically already is a min.

Ideally, I would love to allow users to set either n_workers or min and max for ultimate flexibility. It would feel sort of like scale and adapt too. But in that scenario n_workers stops being a minimum and becomes an equal which would be a break.

For something as small as this I didn't really want to go through any deprecations or breaks.

One alternative could be to have a mode kwarg instead which defaults to MIN but can also be set to MAX or EQUAL (maybe via an enum).

Also thanks for the tip about status, I'll have a look.

@mrocklin
Copy link
Member

My preference would be a mode={"at least", "at most", "exactly"} keyword.

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.

The code looks good to me. Thanks for adding this functionality, I've just stumbled across a use case for this.

@gjoseph92 gjoseph92 self-requested a review July 21, 2022 15:01
@jacobtomlinson
Copy link
Member Author

Thanks for the review feedback @gjoseph92, @hendrikmakait, @ian-r-rose. I've tweaked the PR, but this is my first time switching out an Enum for a Literal so I'd appreciate you confirming that I've done what you intended.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2022

Unit Test Results

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

       15 files  ±  0         15 suites  ±0   6h 28m 36s ⏱️ - 5m 31s
  2 992 tests +  4    2 898 ✔️ ±  0       89 💤 +1  5 +3 
22 189 runs  +32  21 139 ✔️ +28  1 043 💤  - 1  7 +5 

For more details on these failures, see this check.

Results for commit 4b2b946. ± Comparison against base commit 4f6960a.

♻️ This comment has been updated with latest results.

jacobtomlinson and others added 2 commits July 26, 2022 11:46
Co-authored-by: Gabe Joseph <gjoseph92@gmail.com>
Co-authored-by: Lawrence Mitchell <wence@gmx.li>
@jacobtomlinson
Copy link
Member Author

Appreciate the review here @gjoseph92 @wence-! Addressed everything except making the condition check more DRY, I'm really struggling to think of a clean way to write this. Originally it was lambdas which were more readable, but flake8 wasn't happy with assigning lambdas. I like @wence-'s suggestion but can't quite think of how to handle the different kwargs.

Copy link
Collaborator

@gjoseph92 gjoseph92 left a comment

Choose a reason for hiding this comment

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

Thanks @jacobtomlinson!

@jacobtomlinson
Copy link
Member Author

Test failures are unrelated so I intend to merge this in 24 hours if there are no further comments.

@gjoseph92
Copy link
Collaborator

@jacobtomlinson are you going to merge this?

@jacobtomlinson
Copy link
Member Author

jacobtomlinson commented Aug 5, 2022

Thanks for the nudge @gjoseph92. @wence- made a suggestion on making it more readable so I've applied that and am waiting on CI again. Should be good to merge as long as no related tests fail.

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.

Allow client wait_for_workers to wait for an absolute number of workers
7 participants