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: basic support for bokeh.plotting.show #3796

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

akshayka
Copy link
Contributor

Patches bokeh.plotting.show() to display its first argument in the output area. Other arguments are ignored. This is similar to our patch of IPython.display.display.

Not an elegant fix, but should work for many use cases. The proper fix would be to support bokeh's output_notebook() — I believe this uses IPython.display.publish_display_dataunder the hood, which we don't yet support.

cc @bryevdv

image

Patches bokeh.plotting.show() to display its first argument in the
output area. Other arguments are ignored.

Not an elegant fix, but should work for most use cases.
Copy link

vercel bot commented Feb 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 14, 2025 4:45am
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 14, 2025 4:45am

@bryevdv
Copy link

bryevdv commented Feb 14, 2025

@akshayka I'll say I don't have an opinion on changes that you make on your end, if something works for you, that's great. I will state that all of the Bokeh documentation for use in notebooks starts by instructing users to execute output_notebook at the start. You should definitely expect that every Bokeh user will execute output_notebook, so you'd need to at least make sure that calling output_notebook doesn't actively interfere with anything. All that said, I think you could accomplish the same thing without monkeypatching by using Bokehs notebook hook registration function.

@bryevdv
Copy link

bryevdv commented Feb 14, 2025

For reference the default built-in Jupyter hooks are registered like this:

install_notebook_hook('jupyter', load_notebook, show_doc, show_app)

If you used this API you could make your load_notebook (which is what output_notebook does) be a no-op, and you could make show_app emit an exception, since Bokeh server apps can't be run in a WASM notebook. Then show_doc would be your customized version of show.

Edit: I guess you'd also want to set curstate().notebook_type = "marimo" or whatever name you pass to install_notebook_hook so that your hooks get used by default in your notebooks.

@akshayka
Copy link
Contributor Author

akshayka commented Feb 14, 2025

Thanks @bryevdv for the feedback.

That mostly works, except it appears output_notebook() resets curstate().notebook_type to Jupyter, and I don't think I can expect users to manually pass a notebook type to this function (since they'll just copy tutorials like you mentioned).

So it seems like we'd at least need to monkey-patch output_notebook to set curstate().notebook_type to marimo and do nothing else?

@bryevdv
Copy link

bryevdv commented Feb 14, 2025

That mostly works, except it appears output_notebook() resets curstate().notebook_type to Jupyter

If that's true it sounds like a bug or oversight on our end. Do you have a link to the code that you saw doing that handy?

Edit: well, here I guess

https://github.com/bokeh/bokeh/blob/dd4747fa6419fa76f2fe6aa25866457a74e384d6/src/bokeh/io/output.py#L123

So I think there probably out to be a way to override the function parameter default, and it was just an oversight that there is not one.

This reverts commit bbf143b.

Using the notebook hook API requires us to call output_notebook(), which
in turn adds a dependency on IPython. Monkey patching show and
output_notebook to be a NOOP is fine for now.
@akshayka
Copy link
Contributor Author

So I think there probably out to be a way to override the function parameter default, and it was just an oversight that there is not one.

Thanks, that appears to be it.

For now I think patching show and output_notebook is easiest. I experimented with using the hooks, but it appeared I still needed to call output_notebook(notebook_type="jupyter") for marimo's installed hook to take effect, but that brought in a dependency on IPython that we didn't need.

It's certainly possible I did something wrong, but happy with this simple patch for now.

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.

3 participants