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

Move core functionality from SpecCluster to Cluster #2913

Merged
merged 9 commits into from Aug 6, 2019

Conversation

@mrocklin
Copy link
Member

commented Aug 1, 2019

This moves standard functionality from SpecClsuter to the Cluster
superclass. It also removes the assumption that the Scheduler will be
local to the Cluster class.

cc @jcrist

Move core functionality from SpecCluster to Cluster
This moves standard functionality from SpecClsuter to the Cluster
superclass.  It also removes the assumption that the Scheduler will be
local to the Cluster class.
@mrocklin

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@mmccarty , this is what came out of our meeting. We wanted to find the set of functionality in SpecCluster that was general enough to be used within projects like dask-yarn and dask-gateway.

Hopefully everything here is generic enough, but we'll have to test things out to see.

Also cc @jacobtomlinson , though I suspect that he is busy.

mrocklin added 5 commits Aug 1, 2019
@mrocklin mrocklin referenced this pull request Aug 6, 2019
2 of 2 tasks complete
@jcrist
Copy link
Member

left a comment

Overall this seems good and useful to me. Thanks for working through this.


self.status = "closed"

def close(self, timeout=None):

This comment has been minimized.

Copy link
@jcrist

jcrist Aug 6, 2019

Member

Should _close/close be idempotent? I think right now it is just because all the operations in _close happen to be idempotent themselves, but it would maybe be good to be explicit with this.

This comment has been minimized.

Copy link
@mrocklin

mrocklin Aug 6, 2019

Author Member

I've added a check to pass if the status is closed. However there are still cases where this gets called twice that could be problematic.


def __del__(self):
if self.status != "closed":
self.loop.add_callback(self.close)

This comment has been minimized.

Copy link
@jcrist

jcrist Aug 6, 2019

Member

Is it safe to use the loop in __del__ in all scenarios?

This comment has been minimized.

Copy link
@mrocklin

mrocklin Aug 6, 2019

Author Member

It's worked well so far (I think?)

It's a good point though. For now I've protected it against Attribute and Runtime errors (runtime errors arise when the loop is closed, as might be the case during shutdown)

@mrocklin mrocklin merged commit f6c8818 into dask:master Aug 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mrocklin mrocklin deleted the mrocklin:spec-to-cluster branch Aug 6, 2019

@jcrist

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I'm currently looking at using this, and found a few things that need changing (mostly around startup, shutdown, and inadequate separation from SpecCluster). AFAIK there's no non-me projects looking at using this class directly currently? Trying to determine if SpecCluster is the only public interface I need to worry about breaking.

@mrocklin

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.