Fix: insert variables error + default protocol #37
Conversation
echarles
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adjusts the WebSocket client defaults to use Jupyter Server’s default websocket protocol by default, and improves error handling when setting variables by surfacing a clearer serialization failure message.
Changes:
- Switch default
subprotocolfromJupyterSubprotocol.V1toJupyterSubprotocol.DEFAULTin WebSocket session/client constructors. - Wrap
serialize_object(value)inKernelClient.set_variable()with aValueErrorthat includes additional context about non-serializable objects.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
jupyter_kernel_client/wsclient.py |
Changes default websocket subprotocol selection to DEFAULT. |
jupyter_kernel_client/client.py |
Improves error reporting when variable serialization fails during set_variable(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ping_interval: float = 60, | ||
| reconnect_interval: int = 0, | ||
| subprotocol: JupyterSubprotocol | None = JupyterSubprotocol.V1, | ||
| subprotocol: JupyterSubprotocol | None = JupyterSubprotocol.DEFAULT, |
There was a problem hiding this comment.
The subprotocol parameter is typed as optional (JupyterSubprotocol | None), but KernelWebSocketClient._on_message only handles DEFAULT and V1 and will raise ValueError("unsupported protocol.") for None. Consider normalizing None to JupyterSubprotocol.DEFAULT in __init__ (and/or removing | None from the type) so passing None doesn’t break message handling.
| def __init__(self, | ||
| log: logging.Logger | None = None, | ||
| subprotocol: JupyterSubprotocol | None = JupyterSubprotocol.V1, | ||
| subprotocol: JupyterSubprotocol | None = JupyterSubprotocol.DEFAULT, |
There was a problem hiding this comment.
subprotocol is declared as JupyterSubprotocol | None, but downstream code (e.g., KernelWebSocketClient._on_message) doesn’t accept None. Either remove | None here or coerce None to JupyterSubprotocol.DEFAULT to keep the type contract consistent.
| try: | ||
| data, metadata = serialize_object(value) | ||
| except ValueError as e: | ||
| raise ValueError( | ||
| f"Cannot serialize variable '{name}': {e}. " |
There was a problem hiding this comment.
New behavior: set_variable now wraps serialization failures and raises a ValueError. Since the repo has pytest coverage for successful set_variable, please add a negative test that passes a clearly non-serializable value (e.g., an asyncio.Future() or a threading.Lock()) and asserts a ValueError mentioning the variable name, so this error path stays stable.
1 participant
No description provided.