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

Restrict bokeh=3 support #7329

Merged
merged 4 commits into from
Nov 18, 2022
Merged

Restrict bokeh=3 support #7329

merged 4 commits into from
Nov 18, 2022

Conversation

gjoseph92
Copy link
Collaborator

See discussion in #7327

  • reverts YAML changes from Allow bokeh=3  #5648
  • adds < 3 back into error messages
  • adds an explicit check that the bokeh version is < 3, and does not show the dashboard otherwise. This is new. I would rather not show the dashboard at all and tell users why than silently show something misleading.
  • adds tests for dashboard behavior with different bokeh versions. I couldn't find any; maybe there are some and I just didn't know where they were?
  • Tests added / passed
  • Passes pre-commit run --all-files

This reverts commit d4adb3a.

Leaving all code changes, just:
* reinstating the `< 3` version restriction
* including `< 3` in error messages
@jrbourbeau jrbourbeau changed the title Restrict bokeh 3 support Restrict bokeh=3 support Nov 17, 2022
@gjoseph92
Copy link
Collaborator Author

Hmm clearly the mocking I added doesn't work when other tests have run before it.

@jrbourbeau jrbourbeau mentioned this pull request Nov 17, 2022
4 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

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   6h 46m 13s ⏱️ + 9m 14s
  3 224 tests +  3    3 138 ✔️ +  1    84 💤 +1  2 +1 
23 839 runs  +22  22 922 ✔️ +14  915 💤 +7  2 +1 

For more details on these failures, see this check.

Results for commit 5e57470. ± Comparison against base commit 69b74f9.

♻️ This comment has been updated with latest results.

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 @gjoseph92

Comment on lines +38 to +43
# Remove these imports, so when the scheduler imports
# `distributed.dashboard.scheduler`, it has to re-import `dashboard.core`, which
# is where the bokeh version detection happens at import time, via
# `dashboard.utils.BOKEH_VERSION`.
sys.modules.pop("distributed.dashboard.scheduler", None)
sys.modules.pop("distributed.dashboard.core", None)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking the time to add this test coverage and figure out the weirdness that was happening with these mocks not originally working as expected (when the entire test suite was run). It's good to see things working, but I will say the logic here looks a bit brittle and it would be good to simplify if possible. Let's include things here as they are, since they work, and will help us get the release out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, it's highly brittle (as evidenced by it working the first time but failing when run in CI). Since all this logic happens at import time, brittle mocks are kind of the only option—we'd probably want to refactor things into a function, so patching it is much more predictable.

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.

None yet

2 participants