Thanks for this, I'm glad to see people experimenting with these policies on Kubernetes. Two main comments regarding the code here:

1. This could use tests
2. It would be good to avoid modifying the scheduler directly. We've intentionally kept the Adaptive code separate from the Scheduler code and it would be useful to keep it that way in the short term. I expect other Adaptive strategies to arise and we can't have all of them adding attributes to the scheduler. Do you think it would be possible to put the min/max_workers values on the Adaptive class and specify them in your preload script?

For Kubernetes in particular I've been thinking that maybe the right thing to do here isn't to scale-up/scale-down but rather to set a target number of workers given what we know about the workload. For example, we can look at Scheduler.total_occupancy and get a pretty good estimate of how many CPU-seconds we need. This is larger in scope that this PR, but might be worth considering.

Hi @mrocklin (I co-wrote the PR with @jacobtomlinson ),

1. That's fair - we thought you might say that 😄
2. We agree, but without a fairly significant refactor, we couldn't see how to do this, as a lot of the logic about add/removing nodes is contained in the Scheduler. (I guess this is what you're driving at with "larger scope than this PR")

(btw I'm presenting some of this stuff at AGU on Monday, if you're about - talk called "Jade: using on-demand cloud analysis to give scientists back their flow")

I've also been looking at extending Adaptive, and there are various strategies for which scheduler.workers_to_close() is too inflexible.

Perhaps this function itself should be moved into the Adaptive class, as it that seems to be the only place it's used? This would mean that future improvements in Adaptive strategies would be less likely to need to touch the scheduler.

I agree with @rbubley.

We did start with min_workers and max_workers in the adaptive class but found ourselves basically reimplementing workers_to_close as it wasn't flexible enough for what we were trying to do. Hence why we were editing the scheduler itself.

One thing I was thinking while working on this is that scaling up and down in the adaptive class in inconsistent. Scaling up tells you how many workers you should have total, scaling down effectively says "I've terminated these workers please garbage collect them". While this makes sense from a practical point perhaps it could be possible for the adaptive class to handle retiring the workers as well as removing them. And for scaling up perhaps it should be "please add three more", as that feels more consistent with "please remove these three".

I've raised a separate issue for adaptive kubernetes clusters here, just in case this conversation extends beyond the lifetime of this PR. #1619

We did start with min_workers and max_workers in the adaptive class but found ourselves basically reimplementing workers_to_close as it wasn't flexible enough for what we were trying to do.

Perhaps we can improve workers_to_close? What was restrictive?

I would be fine with a minimum keyword in the retire_workers method if that would solve things.

A slightly more general way would be to have a list of do_not_retire workers.

If you only care about a numeric minimum you could just select some arbitrarily, but in some cases there may be distinguished workers that you want to keep alive: perhaps you get a discount if the same worker stays alive for a long time, or perhaps these workers are using harvested time from computers doing something else, etc.

This could be achieved with a function parameter, but could also be achieved by using the existing worker resources functionality - which in some ways might make more sense: they could declare themselves with a resource of “always-on”, or something.

Have you had concrete experience with systems that need this functionality or is this more hypothetical?

I wouldn't say 'need': it's a want. Both of the examples I gave are live issues on our roadmap.

Would you mind looking at #1632 and checking if the modifications made in that PR could be used in the resolution of this issue?

It would, at a high level, move the question of adaptive scaling behavior to Adaptive subclasses as previously suggested by providing an override hook in Adaptive but maintain compatibility with the current scheduler API.

As @rbubley suggested, it seems a touch strange that this logic lives in the scheduler but I can see a compelling argument for providing basic scale-down behavior in the scheduler interface, as is currently provided by workers_to_close and retire_workers. Adding the ability to hook into and override this behavior via Adaptive seems preferable to extending the core scheduler class with arbitrarily complex scaling logic.

@asford I see #1632 as a complimentary PR to this one. I need to make some changes to this one anyway before it can be merged, however I get the feeling progress will be slow this week with AGU.

It seems that one of the primary reasons this change touches scheduler.py is to provide support for command-line configuration for your autoscaling logic. I've run into the same issue, but don't think it's a great idea to overload scheduler as a configuration container and/or rely on modification of the dask-scheduler CLI.

Would a change allowing pass-through arguments to modules loaded via --preload be a workable solution for you? I'd envision this functioning in a manner similar to a compiler, in which subtool-specific flags may be specified on the command line. For example, clang -Xlinker --foo=bar passes --foo=bar as arguments to ld. In this case your interface might look like:

dask-scheduler --preload scheduler_autoscaling.py -Xpreload --min-workers=2 -Xpreload --max-workers=10

Where scheduler_autoscaling.py is a application specific plugin you've written that handles your scaling logic.

Passing through keyword arguments to the preload script makes sense to me. In general I'm usually in favor of improving the functioning of escape hatches like preload scripts.

@jacobtomlinson Just to clarify, I think this PR and #1632 are compatible and I would be stoked to see all of this logic land somewhere in distributed together.

I've opened #1634 as a strawman implementation of the "command line passthrough" concept. Would you mind taking a look at that pull and seeing if it could be used as a base for this functionality?

@asford Yes I think that would be a good option. Let me know when it's in and I'll update this PR to make use of it.

I'm looking for an update on this issue. @jacobtomlinson, do you expect to revisit things here (#1632 was merged).

@jhamman yes keen to revisit if @mrocklin is still happy with the proposal

In general I agree that adding min/max workers to Adaptive is a good idea

I've implemented min/max in Adaptive here: #1797

Note that this doesn't pass anything through the scheduler. This just focuses on the Adaptive piece in isolation.

I suppose this can be closed now?

Thanks for starting this @jacobtomlinson !