Notebook + Server Not Working

[bryevdv]

It's possible the examples just need to be updated to current practice somehow, but I suspect there is little more to it than that.

examples/plotting/notebook/animated.ipynb:
 notebook + server does not work

examples/plotting/notebook/interact_numba.ipynb:
 image update performance degraded

examples/plotting/notebook/line_server.ipynb:
 notebook + server does not work

examples/plotting/notebook/random_walk.ipynb:
 notebook + server does not work

[bryevdv]

OK, "got this working". Seems to boil down to:

• now needing to open up bokeh server for connections from notebook server with --allow-websocket-origin=localhost:8888
• session ids mismatching
• unncessary push (is io.push good for anything? should it be removed?)

With this patch

diff --git a/bokeh/io.py b/bokeh/io.py
index d7b593f..6c8d3d0 100644
--- a/bokeh/io.py
+++ b/bokeh/io.py
@@ -299,7 +299,6 @@ def _show_file_with_state(obj, state, new, controller):

 def _show_notebook_with_state(obj, state):
     if state.server_enabled:
-        push(state=state)
         snippet = autoload_server(obj, session_id=state.session_id_allowing_none, url=state.url, app_path=state.app_path)
         publish_display_data({'text/html': snippet})
     else:

And if I explicitly add a session id to session = push_session(curdoc(), session_id='default') (as shown in (https://github.com/bokeh/bokeh/blob/notebook_server/examples/plotting/notebook/animated.ipynb) then things work as expected with . I guess this is mostly a documentation issue, but if there are ideas to make the notebook use-case closer to "just works" they are welcome

[havocp]

Removing that push seems logical since I think we push to the server elsewhere anyway right?

Could we get the session id from the server push and then set it as the state's session id and be sure we do the notebook push second so that autoload_server gets it ?

Unfortunate that the notebook is on a different port so can't use the websocket. Possible solution: when we are using the default --host, also default to allowing websocket port 8888 if that's the notebook default ... we should probably be careful not to enable that websocket origin if the --host has been changed to a real prod host though.

[havocp]

Here's an untested diff of things I saw, including your removal of push:
notebook-changes.txt

There's an "elif" in _show_with_state that I think is wrong, and I played with the idea here of saving the session id from the server push (which means _show_with_state needs to push to server first before doing notebook... which makes sense anyway so the server is ready for the notebook to load the output).
In fact with this setup maybe defaulting output_server to DEFAULT_SESSION_ID doesn't make sense any more and we could get rid of DEFAULT_SESSION_ID, I"m not sure.

Needs testing, and probably could have some unit tests too, etc.

[havocp]

Here's that thing inline, hadn't ever tried the github file attach feature, it appears to be limited.

diff --git a/bokeh/io.py b/bokeh/io.py
index d7b593f..2cdbf3f 100644
--- a/bokeh/io.py
+++ b/bokeh/io.py
@@ -280,14 +280,17 @@ def show(obj, browser=None, new="tab"):
 def _show_with_state(obj, state, browser, new):
     controller = browserlib.get_browser_controller(browser=browser)

-    comms_handle = None
+    # we show ALL enabled outputs, they are not mutually exclusive.
+
+    # show server first so if we send an autoload_server snippet
+    # to the notebook, the server will be ready.
+    if state.server_enabled:
+        _show_server_with_state(obj, state, new, controller)

+    comms_handle = None
     if state.notebook:
         comms_handle = _show_notebook_with_state(obj, state)

-    elif state.server_enabled:
-        _show_server_with_state(obj, state, new, controller)
-
     if state.file:
         _show_file_with_state(obj, state, new, controller)

@@ -299,7 +302,6 @@ def _show_file_with_state(obj, state, new, controller):

 def _show_notebook_with_state(obj, state):
     if state.server_enabled:
-        push(state=state)
         snippet = autoload_server(obj, session_id=state.session_id_allowing_none, url=state.url, app_path=state.app_path)
         publish_display_data({'text/html': snippet})
     else:
@@ -399,6 +401,7 @@ def _push_to_server(session_id, url, app_path, document, io_loop):
     session = push_session(document, session_id=session_id, url=url, app_path=app_path, io_loop=io_loop)
     session.close()
     session.loop_until_closed()
+    return session.id

 def push(session_id=None, url=None, app_path=None, document=None, state=None, io_loop=None, validate=True):
     ''' Update the server with the data for the current document.
@@ -439,7 +442,6 @@ def push(session_id=None, url=None, app_path=None, document=None, state=None, io
         app_path = state.app_path

     # State is supposed to ensure these are set
-    assert session_id is not None
     assert url is not None
     assert app_path is not None

@@ -452,8 +454,13 @@ def push(session_id=None, url=None, app_path=None, document=None, state=None, io
     if validate:
         document.validate()

-    _push_to_server(session_id=session_id, url=url, app_path=app_path,
-                    document=document, io_loop=io_loop)
+    new_session_id = _push_to_server(session_id=session_id, url=url, app_path=app_path,
+                                     document=document, io_loop=io_loop)
+    # fill in the session id we used, so _show_notebook_with_state
+    # can use it and so any future push() maybe after modifying
+    # the document further can use it.
+    if session_id is None:
+        state._session_coords.session_id = new_session_id

 def push_notebook(document=None, state=None, handle=None):
     ''' Update the last-shown plot in a Jupyter notebook with the new data
diff --git a/bokeh/resources.py b/bokeh/resources.py
index ca2ffa0..807a571 100644
--- a/bokeh/resources.py
+++ b/bokeh/resources.py
@@ -114,6 +114,10 @@ class _SessionCoordinates(object):
             self._session_id = generate_session_id()
         return self._session_id

+    @session_id.setter
+    def session_id(self, value):
+        self._session_id = value
+
     @property
     def session_id_allowing_none(self):
         """ Session ID provided in kwargs, keeping it None if it hasn't been generated yet.
diff --git a/bokeh/tests/test_io.py b/bokeh/tests/test_io.py
index 74fbd28..9d368e7 100644
--- a/bokeh/tests/test_io.py
+++ b/bokeh/tests/test_io.py
@@ -352,7 +352,6 @@ class Test_ShowNotebookWithState(DefaultStateTester):
         mock_autoload_server.return_value = "snippet"

         io._show_notebook_with_state("obj", s)
-        self._check_func_called(mock_push, (), {"state": s})
         self._check_func_called(mock_publish_display_data, ({"text/html":"snippet"},), {})

     @patch('bokeh.io.get_comms')

[bryevdv]

The elif is to keep it from opening a new browser tab with a server autoload in it, in addition to inlining a server autoload in the notebook. It also keeps comms and server exclusive, since the notebook server handling is only handled in _show_notebook_with_state where one branch does comms and another does server.

[bryevdv]

Regarding opening the websocket port, how about --notebook that is just an alias for adding an --allow-websocket-origin=localhost:8888

It's slightly more work to make users add --notebook but I'm just thinking "foo is a shortcut for bar" is easier to document than "foo does bar, except when it does baz" This would also be cleaner to implement and easier to test. We could add it as a suggestion in the "can't connect" error message too: "if you are using the Jupyter notebook, need --notebook" etc.

@pzwang do you have UX thoughts here? Do you have a strong preference?

[bryevdv]

Regarding the release: I'm -1 on changing the logic much but I will see about keeping track of ID's for users implicitly in the notebook case in case that is simple.

[havocp]

I think the elif needs to be only on the show-in-browser aspect and not the push ?

if you output_server and output_notebook then show has to push to the server sometime right

[havocp]

check over the docs on all these functions, whatever we do they need to be kept in sync ...

[havocp]

--notebook sounds like an ok approach