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

Prepare for Dask 2.2 release #303

Merged
merged 7 commits into from Jul 30, 2019
Merged

Prepare for Dask 2.2 release #303

merged 7 commits into from Jul 30, 2019

Conversation

@mrocklin
Copy link
Member

@mrocklin mrocklin commented Jul 29, 2019

This is just to test against new versions prior to a release upstream

@mrocklin mrocklin mentioned this pull request Jul 29, 2019
8 tasks
@mrocklin mrocklin changed the title Pip install dask/distributed git master in CI Prepare for Dask 2.2 release Jul 29, 2019
@mrocklin
Copy link
Member Author

@mrocklin mrocklin commented Jul 29, 2019

OK, this passes with both master and latest release. I've changed the title. I think that this is ready for review.

@guillaumeeb
Copy link
Member

@guillaumeeb guillaumeeb commented Jul 30, 2019

Hm, this is difficult to review without context... There is little changes here, I suspect this is the minimum to make it work, but that we should rewrite a lot of the code base to comply with SpecCluster.

@mrocklin
Copy link
Member Author

@mrocklin mrocklin commented Jul 30, 2019

I suspect this is the minimum to make it work

Right, the thing to notice here is the two green check CI marks at the end of the list of commits. One is for using git master, and one is for using the current release (See the last commit, which transitions from git master back to current release). If we don't merge this then dask-jobqueue will not be compatible with the next release.

we should rewrite a lot of the code base to comply with SpecCluster.

I agree. I'm changing SpecCluster now to include most of the functionality that dask-jobqueue has had to build on its own. Hopefully the rewrite will be very simple once I'm done. I think that that will happen after the Dask 2.2 release though, so we should merge and release this before the rewrite happens.

@@ -548,5 +548,10 @@ def _job_id_from_submit_output(self, out):

return job_id

def worker_key(self, worker_state):
@staticmethod
def worker_key(worker_state):
Copy link
Member Author

@mrocklin mrocklin Jul 30, 2019

Choose a reason for hiding this comment

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

The method wasn't easy to serialize because the class has some unserializable elements. Fortunately we don't need self here, so I changed this to a staticmethod in order to remove need to serialize the class.


@property
def scheduler_comm(self):
return self.local_cluster.scheduler_comm
Copy link
Member Author

@mrocklin mrocklin Jul 30, 2019

Choose a reason for hiding this comment

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

This is for compatibility with SpecCluster, which some elements, like adaptive, are starting to expect

try:
self._adaptive = Adaptive(self.scheduler, self, **self._adaptive_options)
except Exception:
self._adaptive = Adaptive(self, **self._adaptive_options)
Copy link
Member Author

@mrocklin mrocklin Jul 30, 2019

Choose a reason for hiding this comment

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

We dropped the need to explicitly pass in the scheduler in recent versions. We will query the scheduler over a network connection instead so that it can be remote in the future.

Copy link
Member

@guillaumeeb guillaumeeb Jul 30, 2019

Choose a reason for hiding this comment

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

That's perfect!

@guillaumeeb
Copy link
Member

@guillaumeeb guillaumeeb commented Jul 30, 2019

Thanks for doing this!

@guillaumeeb guillaumeeb merged commit 8e508ed into dask:master Jul 30, 2019
1 check passed
@mrocklin mrocklin deleted the test-master branch Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants