Fix connect mode with jupyter-collaboration#95
Open
andrii-i wants to merge 8 commits into
Open
Conversation
11f06fe to
e297c4c
Compare
|
Was hoping the change would be as simple as duplicating our endpoint in JSD to also cover the Hmm, but it seems that JSD's |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #92
Problem
Connect mode only works with
jupyter-server-documents(JSD) and fails againstjupyter-collaborationin four ways:File ID retrieval calls
POST /api/fileid/index, which only exists in JSD. jupyter-collaboration exposesPUT /api/collaboration/session/{path}instead. Connect mode fails with 404 on any jupyter-collaboration server.The Y.js WebSocket handshake is rejected with
1003 unknown_session. jupyter-collaboration requires asessionIdquery parameter on the room WebSocket URL. nb-cli does not pass it.Kernel WebSocket messages are not parsed correctly. jupyter-collaboration does not hardcode the v1 kernel protocol the way JSD does. Without negotiating the
v1.kernel.websocket.jupyter.orgsubprotocol, the server falls back to the legacy 4-byte big-endian frame format, which the v1 parser (8-byte little-endian offsets) silently fails to decode.Cell outputs are never written back to the notebook. JSD writes outputs to the Y.js document server-side (
server_side_execution=True). jupyter-collaboration does not. nb-cli relies entirely on reading outputs from the Y.js document after the server writes them, sonb executereturns empty outputs.Changes
get_file_id()inydoc.rstriesPOST /api/fileid/indexfirst (JSD). On 404, falls back toPUT /api/collaboration/session/{path}(jupyter-collaboration), which returns bothfileIdandsessionId. The return type is now(String, Option<String>)to carry the optional session ID.build_room_ws_url()usesUrl::parse+query_pairs_mutfor proper encoding and appendssessionIdwhen present. Aserver_writes_outputsflag tracks which output path to use (true for JSD where session ID is absent, false for jupyter-collaboration).KernelWebSocket::connect()inwebsocket.rssendsSec-WebSocket-Protocol: v1.kernel.websocket.jupyter.orgduring the handshake. This makes the server select the v1 binary frame format regardless of which Y.js backend is installed.When
server_writes_outputsis false (jupyter-collaboration path),execute_codeinmod.rscollects outputs from kernel iopub messages (stream, execute_result, display_data, error) into a local vec. After the kernel goes idle, it writes them to the Y.js document viaupdate_cell_outputsandupdate_cell_execution_count, syncs, and the existing read loop picks them up. A fallback path returns collected kernel outputs directly if the Y.js write/sync times out.Removed
#[allow(dead_code)]fromupdate_cell_outputsandupdate_cell_execution_countinoutput_conversion.rssince they are now called in the client-write path.Four files changed, no new dependencies.
Testing
Tested end-to-end against real Jupyter servers in clean venvs:
nb connectnb readnb execute(2 cells, stream + execute_result)nb output clearnb statusMerge order
Needs to be merged after #91.
continue-on-errorfrom thejupyter-collaborationmatrix entry inrust.yml, ensure both CI flows pass