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.

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

Configuration #75

Merged
merged 12 commits into from
May 19, 2018
Merged

Configuration #75

merged 12 commits into from
May 19, 2018

Conversation

mrocklin
Copy link
Member

@mrocklin mrocklin commented May 5, 2018

We're updating configuration in dask/dask#3432

This affects downstream projects and makes it a bit easier for them to handle their own configuration files.

This PR shows how that configuration system might be used within dask-kubernetes. Review would be welcome. I recommend reading the documentation provided within dask/dask#3432 first.

cc @jacobtomlinson

@mrocklin mrocklin force-pushed the config-2 branch 4 times, most recently from b986806 to 49b9446 Compare May 7, 2018 16:16
@mrocklin mrocklin changed the title [RFC] Configuration Configuration May 7, 2018
@mrocklin
Copy link
Member Author

mrocklin commented May 7, 2018

I've removed the RFC label. This is hypothetically ready to merge. Critical feedback welcome.

@@ -381,6 +392,8 @@ def scale_up(self, n, pods=None, **kwargs):
--------
>>> cluster.scale_up(20) # ask for twenty workers
"""
if dask.config.get('kubernetes.count.max') is not None:
n = min(n, dask.config.get('kubernetes.count.max'))
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth issuing a warning when n > kubernetes.count.max? This may lead to surprising behavior when you can't scale beyond a certain threshold because of a parameter you didn't set.

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just one small comment.

@mrocklin mrocklin merged commit c773aad into dask:master May 19, 2018
@mrocklin mrocklin deleted the config-2 branch May 19, 2018 11:30
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.

2 participants