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

Fix ClusterManager.dashboard_link #291

Merged
merged 1 commit into from Jul 22, 2019
Merged

Conversation

@stuarteberg
Copy link
Member

@stuarteberg stuarteberg commented Jul 17, 2019

Fixes #288

Specifically, fixes this error in the dashboard_link attribute of ClusterManager:

  File "/miniconda/envs/myenv/lib/python3.7/site-packages/dask_jobqueue/deploy/cluster_manager.py", line 146, in dashboard_link
    port = self.scheduler.services["bokeh"].port
KeyError: 'bokeh'
@lesteve
Copy link
Member

@lesteve lesteve commented Jul 19, 2019

I think it's fine to do bokeh -> dashboard without backward compatibility as I said in #288 (comment).

Would it be possible and not too hard to add a test for this? Currently our tests are passing with distributed >= 2, e.g. https://travis-ci.org/dask/dask-jobqueue/jobs/560794336.

@stuarteberg stuarteberg force-pushed the fix-dashboard-link branch from f24c306 to 9321940 Jul 19, 2019
@stuarteberg
Copy link
Member Author

@stuarteberg stuarteberg commented Jul 19, 2019

Would it be possible and not too hard to add a test for this?

Ok, done!

@stuarteberg stuarteberg force-pushed the fix-dashboard-link branch from 9321940 to 90a8eb1 Jul 19, 2019
@lesteve
Copy link
Member

@lesteve lesteve commented Jul 21, 2019

Thanks a lot for the additional test!

format_dashboard_link has been added in distributed 2.1 in this commit dask/distributed@bf65f7a. I am in favour of requiring
distributed >= 2.1 (simpler). Note that 2.0 and 2.1 were released two weeks apart (25 June and 8 July): https://distributed.dask.org/en/latest/changelog.html#id1

@guillaumeeb any strong objections on this one?

@guillaumeeb
Copy link
Member

@guillaumeeb guillaumeeb commented Jul 21, 2019

I'm perfectly fine with this.

Note that you've got more than all my trust, and you're very welcome to take this kind of decision without me as a core maintainer, especially during summer holydays!

@lesteve
Copy link
Member

@lesteve lesteve commented Jul 22, 2019

Thanks for your feedback, it's always good to have a second opinion on this kind of decisions!

Let's merge this one then!

@lesteve lesteve merged commit f3858cb into dask:master Jul 22, 2019
1 check passed
@lesteve
Copy link
Member

@lesteve lesteve commented Jul 22, 2019

Thanks a lot for the PR @stuarteberg!

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.

3 participants