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

Restore bokeh as optional dependency with default cluster setup #7230

Merged
merged 1 commit into from Oct 31, 2022

Conversation

oliverholworthy
Copy link
Contributor

Closes #7227

  • Tests added / passed
    • To test this functionality, we'd need to add an environment without bokeh installed, which would be nice to do, but beyond the intended scope of this PR
  • Passes pre-commit run --all-files

Goal

Restoring the bokeh as an optional dependency.

Details

  • Prior to Update minimum bokeh version message #7172 , the bokeh package was an optional dependency. (In the sense that you could start a cluster without bokeh installed)
    • In the most recent release (2022.10.1), bokeh became a required depdency due to the import of distributed.dashboard.core in missing_bokeh which raises an ImportError if bokeh is not installed.
      • Not strictly required, since the dashboard can be disabled by passing dashboard_address=None to the constructor of the Cluster. Which allows use of distributed==2022.10.1 without bokeh installed.

This PR moves the min bokeh variable to a different module that doesn't import code from the dashboard.

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@github-actions
Copy link
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   7h 19m 30s ⏱️ + 40m 23s
  3 165 tests ±0    3 080 ✔️ +1    83 💤  - 1  2 ±0 
23 416 runs  ±0  22 506 ✔️  - 3  904 💤 +1  6 +2 

For more details on these failures, see this check.

Results for commit f06b272. ± Comparison against base commit 1db2595.

@mrocklin
Copy link
Member

Thank you for the fix @oliverholworthy . This makes sense to me.

I'm going to look briefly at the test failures and make sure that they aren't related. I'll merge shortly afterwards though.

@mrocklin
Copy link
Member

Merging this in. There are failing tests around shuffle / bad disk with the experimental system. I don't see how these could be related to this change and, even if they were I'm not sure I would care all that much.

I've tested locally and this certainly fixes the problem. It also makes lots of sense.

A good follow-up would be trimming Bokeh from our minimal versions CI run.

Regardless, merging in. Thank you for tracking this down @oliverholworthy . I owe you a beer.

@mrocklin mrocklin merged commit aa2cd65 into dask:main Oct 31, 2022
Copy link
Member

@jrbourbeau jrbourbeau 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 fixing @oliverholworthy! Apologies for accidentally introducing the problematic bokeh import.

A good follow-up would be trimming Bokeh from our minimal versions CI run.

distributed doesn't yet have one of these, though it'd be great if it did. #4794 started adding this build, but is currently stalled.

@ian-r-rose
Copy link
Collaborator

Thanks @oliverholworthy!

@mrocklin
Copy link
Member

FYI @fjetter when you return you might want to look at the CI failures here. No rush though.

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.

RuntimeError: Cluster failed to start: No module named 'bokeh'
5 participants