Deprecated kernel_id param in GatewayKernelClient init() - #1603
Deprecated kernel_id param in GatewayKernelClient init()#1603vincentye38 wants to merge 18 commits into
Conversation
| GatewayClient.instance().kernels_endpoint, | ||
| url_escape(self.kernel_id), | ||
| "channels", | ||
| ) |
There was a problem hiding this comment.
Note: I proposed to move ws_url initialization to kernel manager, and make ws_url as a property of kernel client. Because kernel manager is responsible to start kernel process, instantiate a kernel client to connect o the process. So it knows information of how to connect to the kernel process.
Kernel manager will set ws_url of kernel client after it creates the kernel client.
There was a problem hiding this comment.
It will be on line 726 instead.
Zsailer
left a comment
There was a problem hiding this comment.
Thanks for the contribution @vincentye38 ! The reconnect logic is a nice addition. A few things to address before this can merge:
- Breaking API change — Removing
kernel_idfromGatewayKernelClient.__init__()will break downstream consumers. Please keep it as an optional kwarg with a deprecation warning and
auto-buildws_urlfrom it for backwards compat. asyncio.run()in_should_reconnect— This will fail inside a running event loop. Needs to be reworked.
add session_id to gateway url
throw dedicated exception
removed GatewayResponseRouterFinished
8731408 to
50bf889
Compare
|
@lresende @kevin-bates Any comment and concern for PR? |
|
I am resolving the failed check. All unit test passed. |
a0f88dc to
5faaca4
Compare
5faaca4 to
607cc2a
Compare
|
It looks like there are some conflicts which need to be resolved. |
| @property | ||
| def session_id(self): | ||
| """The session id.""" | ||
| return self.session.session |
There was a problem hiding this comment.
Is the definition of a property necessary? Is that to allow customizing it downstream in subclasses or just for convenience? If the latter, should it be private (prefixed with _)?
There was a problem hiding this comment.
Subclass can override this property to adapt to what the gateway side uses to key the buffered messages. Currently, Kernel Gateway use session id in the message as key, which is corresponding to self.session.session.
| self.response_router: Optional[Thread] = None | ||
| self._channels_stopped = False | ||
| self._channel_queues = {} | ||
| if kernel_id is not None: |
There was a problem hiding this comment.
The GatewayKernelManager class is still setting this keyword parameter inside of the client method (line 424 in the LHS of the diff).
If it is being deprecated, then we should also update the existing caller in this file to use the new ws_url traitlet instead.
There was a problem hiding this comment.
@vincentye38 I suppose the comment above still needs addressing?
There was a problem hiding this comment.
fixed in a new commit for GatewayKernelManager in line #425
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
…ient Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The kernel_id parameter caused the following exception when the ctor is called by
self.kernel_client = self.client(session=self.session)in KernelManager.init