Add customizable worker_threads

[AndreaGiardini]

Hey

I added the possibility to customize the number of threads per worker.
Until now worker_threads = cores but that's not always the best combination.

If worker_threads is not set then it will fall-back to the default.

Let me know if you have any feedback

Fixes #364
Fixes #362

[lorenzori]

nice thanks! so we can use it as an argument in gateway.new_cluster()?

[AndreaGiardini]

Hey @jcrist
I had a look at the failing CI and it looks like dask/distributed does not communicate the number of cores but only the nthreads, this is why the tests for the widget fail:

https://github.com/dask/distributed/blob/b4dfc925bac32a488be2016a5930a9b7dd95cec5/distributed/scheduler.py#L1707-L1728

Do you have any preference on how to tackle this?

[AndreaGiardini]

Friendly ping to @consideRatio . Maybe you can give me some feedback on this?

[erl987]

Very useful initiative, relates to issue #362.

[consideRatio]

Thanks for your work @AndreaGiardini! I'm not representing this project but hopefully my review can be helpful.

First of all I think its sensible to allow worker_threads to be customizable, so the feature idea is sound.

What I would suggest iterating on is mainly the default value of worker_threads in the normal ClusterConfig and in the KubeClusterConfig.

The ClusterConfig has a default value of worker_threads to be 1, but in the past, it was matching the amount of CPU. The KubeClusterConfig deriving from ClusterConfig has a dynamic default value of worker_threads set to be worker_cores, but in the past int(worker_cores_limit) was used.

As the PR summary or title doesn't indicate an intent to change the default behavior, I think what makes sense to recommend, is to update the PRs code suggestion to implement a dynamic _default_worker_threads function with the @default decorator in both ClusterConfig and KubeClusterConfig with the different defaults previously used.

[consideRatio]

I think worker_threads is of an Integer type already (traitlets), while worker_cores_limit was a Float traitlet type and needed the cast to integer first.

Suggested change

	str(int(config.worker_threads)),
	str(config.worker_threads),

[AndreaGiardini]

@consideRatio Thank you for the valuable input

I fixed the PR as you suggest, I can always squash as final step before merging if needed.
Still, the problem remains with the widget unfortunately... How do you think we should tackle this?

[AndreaGiardini]

Hey @consideRatio and @jcrist

I took some time to rebase the PR and make CI green without upgrading the distributed dep.
I found an issue with the docs generation, that should be fixed as well.

Since there seems to be quite some interest in this feature, can you tell me if something else is needed to get this merged? Thank you

[martindurant]

@consideRatio , are you happy with the changes here?

This all looks fair as far as I can tell.

@jcrist , are you likely to have time to have a quick look?

@jacobtomlinson , this is one of the few repos still on Travis, which still appears to work for now. I expect it might be more complex than most to transform.

[jcrist]

Apologies for letting this linger, this looks good to me. Merging. Thanks for the contribution!

[martindurant]

Thanks, @jcrist

[AndreaGiardini]

Thank you to all of you! :) Would you mind tagging a new release whenever possible? We could really benefit from this

[consideRatio]

Wieee! 🎉