Add worker_key parameter to Adaptive#1992
Conversation
This allows adaptive clusters to intelligently close down groups of workers based on some logical association. See dask/dask-jobqueue#63 for motivation
|
@jhamman any luck with this? My current plan is to wait on you to try things. No rush, I just wanted to make sure that you weren't waiting on me to merge this. |
|
@mrocklin - So far so good. I'm testing this branch along with my mods in dask/dask-jobqueue#63 and things are working much better. I'll give this PR a quick review now. |
jhamman
left a comment
jhamman
left a comment
There was a problem hiding this comment.
This all seems to work much better so apart from a few questions, I'm happy to see these changes.
| if workers is None: | ||
| workers = self.workers_to_close() | ||
| workers = self.workers_to_close(key=self.worker_key, | ||
| minimum=self.minimum) |
There was a problem hiding this comment.
what is the logic for passing in minimum here? Should you also pass in maximum?
There was a problem hiding this comment.
Minimum here corresponds to the minimum number of workers that we want to keep. The workers_to_close function is a slightly nicer place to handle this than outside. For example if workers_to_close tells us that we need to close all of our workers, we then look at minimum and see that we need one, which worker should we keep? Also, which other workers should we keep that are in the same group. Suddenly we have to replicate the grouping logic.
Maximum doesn't make sense because we're only reducing the number of workers, not increasing them.
| future) | ||
| return future | ||
| else: | ||
| return sync(self.loop, func, *args, **kwargs) |
There was a problem hiding this comment.
Is this related to the grouped worker issue? (I'm not following)
There was a problem hiding this comment.
Something was slightly nicer if we switched to using self.sync here (this is a common pattern in the client). I decided to clean this up as part of this PR.
My thought is that we can wait on merging this until we have a strong reason to do so (either dask/dask-jobqueue#63 or pressure from somewhere else). I imagine its quite possible that we'll iterate a bit more here before I'm done with dask/dask-jobqueue#63 . |
|
@mrocklin - save my few questions above, this seems to working and I'd like to see it merged in the next few days. Currently, this is blocking dask/dask-jobqueue#63. |
|
I'm fine to merge this any time. I'll give you a chance to respond to recent commentary but I'll merge after you give the go-ahead. |
|
Thanks @mrocklin - go ahead and merge whenever now. |
2 participants
This allows adaptive clusters to intelligently close down groups of
workers based on some logical association.
See dask/dask-jobqueue#63 for motivation
cc @jhamman