deno
deno copied to clipboard
feat(jupyter): support `confirm` and `prompt` in notebooks
Solves: https://github.com/denoland/deno/issues/22633
Supports confirm
and prompt
with custom versions used when inside a Jupyter Notebook with Deno kernel.
The desired behavior (per python reference and docs):
- confirm or prompt will trigger kernel to request the frontend to get user's input and return it to backend for processing. If stdin not supported, skip the request.
We accomplish this by creating custom versions of confirm and prompt that call into an op_jupyter_input rust function with access to the stdin_socket.
confirm
and prompt
are instantiated in the jupyter specific TS interface, so they only override the standard functions in jupyter context.
Jupyter requires us to clone zmq_identities for this "input_request" message as documented in comments:
* Using with_identities() because of jupyter client docs instruction
* Requires cloning identities per :
* https://jupyter-client.readthedocs.io/en/latest/messaging.html#messages-on-the-stdin-router-dealer-channel
* The stdin socket of the client is required to have the
* same zmq IDENTITY as the client’s shell socket.
* Because of this, the input_request must be sent with the same IDENTITY
* routing prefix as the execute_reply in order for the frontend to receive the message.
TODO
- [x] Get review on Jupyter mechanics to confirm correctness
- [x] Get review on Rust implementation / code style
- [x] Add JSDoc to confirm/prompt for jupyter
PR guidelines
- [x] Before submitting a PR, please read https://docs.deno.com/runtime/manual/references/contributing
- [x] Comply with title guidelines
- [x] Ensure there is a related issue and it is referenced in the PR text.
- [x] Ensure there are tests that cover the changes.
- [x] Ensure
cargo test
passes. - [x] Ensure
./tools/format.js
passes without changing files. - [x] Ensure
./tools/lint.js
passes. - [x] Open as a draft PR if your work is still in progress. The CI won't run all steps, but you can add '[ci]' to a commit message to force it to. If you would like to run the benchmarks on the CI, add the 'ci-bench' label.
@rgbkrk 👋 I see you've been advising on the Jupyter side for Deno's kernel. If you have a moment to sanity check this for the approach I took re: jupyter protocols, I'd appreciate it.
This PR enables confirm
and prompt
behavior in deno based Jupyter notebooks (ala input prompts in Python notebooks).
Specifically, the way I did zmq_identities copying works, but is it the right way?
Otherwise, I've taken what I saw as python's approach for input, which looks to be overriding input
with a custom builtin in the context of the ipython notebook.
Thanks for checking out this draft if you have availability 🙏 .
I'm trying to bring feature parity for Deno notebooks with Python as the reference and I find myself relying on this functionality in python notebooks when used as operational runbooks.
☹ leading whitespace on PR title. Pushing fix :)
Thanks for bumping this to kick off CI @dsherret! I've been enjoying using dax also as part of these notebooks.
@dsherret I'll treat this as blocked pending: https://github.com/denoland/deno/issues/23617
Great! I'll look at it this weekend and see if I can get it working ☺️
Ooops, I actually meant https://github.com/denoland/deno/pull/23622 and @dsherret told me that it's still not enough to address https://github.com/denoland/deno/issues/23617.
Ooops, I actually meant #23622 and @dsherret told me that it's still not enough to address #23617.
No problem, I subscribed to #23617 and will keep an eye out for it landing!
I've done some deeper typing of the jupyter structures into a library we can share across projects and integrated it with deno here: https://github.com/denoland/deno/pull/23826
It'll be great to bring your work directly into it.
I've done some deeper typing of the jupyter structures into a library we can share across projects and integrated it with deno here: #23826
It'll be great to bring your work directly into it.
Awesome! I'll wait a bit for yours to land :) and then happy to incorporate/build-on.
Landed #23826! You can either rebase/merge main or kick off a new PR. Feel free to ping me, I'm happy to help.
Awesome, thanks @rgbkrk
@zph I got this working using #24373. Do you want me to push to your PR or open a new one based on your code?
You're welcome to push on to this PR! 😁
On Sun, Jun 30, 2024, at 17:24, Bartek Iwańczuk wrote:
@zph https://github.com/zph I got this working using #24373 https://github.com/denoland/deno/pull/24373. Do you want me to push to your PR or open a new one based on your code?
— Reply to this email directly, view it on GitHub https://github.com/denoland/deno/pull/23592#issuecomment-2198826323, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2UGEZI5ZVAKLYCZHIHMDZKCOSZAVCNFSM6AAAAABG5FCNT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJYHAZDMMZSGM. You are receiving this because you were mentioned.Message ID: @.***>
--
Zander GH: @ZPH || https://blog.xargs.io
@zph I got this working using #24373. Do you want me to push to your PR or open a new one based on your code?
@bartlomieju I'm excited you got it working! I took a few hours on it but didn't make much progress and think I need more rust under my belt to solve it after the earlier refactor.
This will be awesome to see in the language and it's cool that some base of my code will make it into deno
You're welcome to push on to this PR! 😁
Great, I will do just that.
@bartlomieju I'm excited you got it working! I took a few hours on it but didn't make much progress and think I need more rust under my belt to solve it after the earlier refactor.
No worries, you tackled a really tough one which looked easy on the surface. Kudos for prompt
ing us (pun intended) to address this one.
This will be awesome to see in the language and it's cool that some base of my code will make it into deno
Keep it coming!
This is now working:
I'm gonna land it as is and add tests in a follow up PR, because they require to rewrite a bunch of testing infra and I want to land changes from #24250 first.
LGTM, exciting work @zph! Thanks for the PR
Awesome! Thank you for carrying this along 😺. I'll pull it down and take it for a spin.
Confirmed looks good in:
- vscode's jupyter notebook environment
- web ui for jupyter notebooks