-
Notifications
You must be signed in to change notification settings - Fork 315
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
[DISCUSS] RESTful Asynchronous Kernel Starts #197
Comments
Thank you for starting this Steve, I think this could be helpful to clients. Had a couple thoughts, questions...
Let's add this to the server's weekly meeting agenda if you can make it on Thursday. |
Thanks for the write-up, Steve. I agree with the general approach. Some thoughts come to mind:
Ideally, whatever status is used to represent a kernel/session that is being launched will be separate from the "starting" status that kernels currently advertise. This will help disambiguate the different states for the end-user and avoid overloading an existing state.
|
Thanks for the feedback @kevin-bates, @captainsafia!
Not necessarily, you can have multiple sessions with different types or names, for instance. Interestingly, the classic notebook tears down the session instead of changing kernels, but does so synchronously. In jupyterlab/jupyterlab#8174 I implemented an asynchronous version of this so you can switch kernels while a slow kernel is launching.
It is possible to trigger by adding a
Agreed!
We call this "initializing" in JupyterLab currently. |
Thank you for making this issue! I've been working on a kernel that proxies requests while waiting for an underlying kernel to take a long time to start up, all while accepting parameters on boot. Delighted to see the conversation going here. |
Hi Steve, thank you for your response. It reflects my naivete with the front-end, so I apologize. I was more caught up on the race condition thing and how to reproduce that than the issue at hand. Mostly for my benefit, the idea is that these long-running calls would be mitigated by...
I'm hoping we could make this the normal sequence of events within the server and simply make the response conditional based on whether the client has requested async behavior or not. |
I agree that behind the scenes the server should be handling all requests as async, since it may be servicing both types of clients. One thing we have to determine is whether we should have an "exists" check at all, and what it should mean. If I POST to |
In // to the discussion on #592, I wonder what are the needed changes to the services packages to support the Pending/Slow staring kernel proposal. My feeling after having worked a bit on some restarting/terminating/... edge case is that a State Machine (https://en.wikipedia.org/wiki/State_pattern) would really help to deal with the various kernel state. For now, the code is really ad-hoc and does not leverage the power of the well-know state pattern. This would mean a deep refactoring of the services, which maybe out of scope of this requirement, but worth at least getting feedback on this idea. |
Closing this, since we have implemented the changes in |
Jupyter Notebook has had an implicit assumption that starting a kernel process is instantaneous. This results in a race condition when two requests try and start a session at the same time. The second started session will check to see if one exists, see that it does not because the previous one has not saved, and create a new one. You will then end up with two sessions for the same path.
Additionally, if the kernel takes a very long time to start, there is no feedback to the client. In JupyterLab 1.2, this results in No Kernel! and a busy indicator until the kernel is started. In JupyterLab 2.0, you see "Kernel" and an idle indication until the response is received, at which point you see "Python 3". You can see this behavior by adding a
yield gen.sleep(10)
inMappingKernelManager.start_kernel
. Admittedly, we can hack the display to show the pending name, but a better fix is to have a better server/client contract.I propose that we upgrade the server to respond POST to
/api/kernels
,/api/sessions
, or/api/terminals/
that has an "async" flag (or equivalent name) in the body with a response of the following form, following the restcookbook:Text inline from the cookbook:
Instead of creating the actual resources, create a temporary one. Instead of returning a 201 (Created) HTTP response, you can issue a 202 (Accepted) response code. This informs the client that the request has been accepted and understood by the server, but the resource is not (yet) created. Send the temporary resource inside the Location header."
This location can store information about the status of the actual resource: an ETA on when it will be created, what is currently being done or processed.
When the actual resource has been created, the temporary resources can return a 303 (See other) response. The location header returns the URI to the definitive resource. A client can either DELETE the temporary resource, or the server can expire this resource and return a 410 (Gone) later on.
This way, the client immediately gets feedback that a kernel or a session is being created without having to wait for it to be created. By making it optional, clients can opt-in to this behavior and migrate to it. At some later date we could require that all clients handle 202 responses.
The server would need to be upgraded to have new database entries for pending kernel and sessions, and use them when determining if one exists. It would need to implement the protocol defined above. Even if a client does not request async, we can still use this new database table to fix the race condition.
The text was updated successfully, but these errors were encountered: