Skip to content

Conversation

@Matt711
Copy link
Member

@Matt711 Matt711 commented May 30, 2023

Closes #725 .

@Matt711 Matt711 requested a review from jacobtomlinson as a code owner May 30, 2023 15:21
Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for raising this. I wonder if we can somehow allow folks to specify these options if they really want to.

For example with this change a user wouldn't be able to do the following

cluster = KubeCluster.from_name("foo", shutdown_on_close=True)

Comment on lines 870 to 875
return cls(
name=name,
create_mode=CreateMode.CONNECT_ONLY,
shutdown_on_close=False,
**kwargs,
)
Copy link
Member

Choose a reason for hiding this comment

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

Pseudo code and probably wont pass linting. But we could use the dict pipe merge now that 3.9 is our minimum!

Suggested change
return cls(
name=name,
create_mode=CreateMode.CONNECT_ONLY,
shutdown_on_close=False,
**kwargs,
)
defaults = {"create_mode": CreateMode.CONNECT_ONLY, "shutdown_on_close": False}
kwargs = defaults | kwargs
return cls(
name=name,
**kwargs,
)

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for raising this. I wonder if we can somehow allow folks to specify these options if they really want to.

For example with this change a user wouldn't be able to do the following

cluster = KubeCluster.from_name("foo", shutdown_on_close=True)

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Looks like CI is passing here. Could I ask you to add/update a test to cover this functionality?

@Matt711 Matt711 force-pushed the shutdown-on-close branch from 133cef2 to 41b3908 Compare June 12, 2023 13:00
Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

The first kr8s PR has landed with #721 so we can start using it 🎉.

I made some quick suggestions here that you should be able to use to swap them out.

@Matt711
Copy link
Member Author

Matt711 commented Jun 12, 2023

Awesome thanks!

The first kr8s PR has landed with #721 so we can start using it 🎉.

I made some quick suggestions here that you should be able to use to swap them out.

Sweet! thanks

@jacobtomlinson jacobtomlinson merged commit 6970a8c into dask:main Jun 13, 2023
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.

KubeCluster.shutdown_on_close defaults to True

2 participants