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

Add version check for absolute url fix #8381

Merged
merged 1 commit into from Dec 1, 2023

Conversation

jacobtomlinson
Copy link
Member

Closes #8380

In #8347 I added a workaround for a breaking change introduced in bokeh 3.3.0. It turns out it wasn't as backward compatible as I thought, so this PR puts the shim behind a version check.

Copy link
Contributor

Unit Test Results

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

       27 files  ±0         27 suites  ±0   12h 2m 48s ⏱️ + 22m 32s
  3 934 tests ±0    3 821 ✔️  - 1     110 💤 ±0  3 +1 
49 488 runs  ±0  47 194 ✔️  - 2  2 291 💤 +1  3 +1 

For more details on these failures, see this check.

Results for commit 91c242c. ± Comparison against base commit 6d1e133.

@jrbourbeau jrbourbeau mentioned this pull request Nov 30, 2023
4 tasks
@fjetter
Copy link
Member

fjetter commented Dec 1, 2023

I'm fine with the fix but am wondering if this is something we can test. we have the mindeps build for the lower versions so I would hope we can cover both cases.

Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Thanks, @jacobtomlinson for the quick turnaround.

@fjetter: I agree that it would be good to have tests for the dashboards with the minimum version of bokeh. From what I see though, mindeps does not include bokeh as it's an optional dependency. We could add another build, but that will require more work than simply adding a single test. I'd rather merge this PR to include it in the release than delay it due to missing automated tests. I've tested this manually and the fix works, so I'd propose adding tests in a follow-up PR.

@jacobtomlinson
Copy link
Member Author

Yeah I agree it would be the right thing to do but didn't bother right now with a test when I spotted it wasn't included in mindeps.

@jrbourbeau jrbourbeau merged commit 051f82a into dask:main Dec 1, 2023
30 of 35 checks passed
@jacobtomlinson jacobtomlinson deleted the bokeh-absolute-gate branch December 4, 2023 08:59
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.

Dashboards fail with 500 status code when using bokeh<3.3.0
4 participants