deno icon indicating copy to clipboard operation
deno copied to clipboard

feat(jupyter): support `confirm` and `prompt` in notebooks

Open zph opened this issue 9 months ago • 11 comments

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.

zph avatar Apr 28 '24 18:04 zph

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 28 '24 18:04 CLAassistant

@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.

zph avatar Apr 28 '24 18:04 zph

☹ leading whitespace on PR title. Pushing fix :)

image

Thanks for bumping this to kick off CI @dsherret! I've been enjoying using dax also as part of these notebooks.

zph avatar Apr 30 '24 18:04 zph

@dsherret I'll treat this as blocked pending: https://github.com/denoland/deno/issues/23617

zph avatar Apr 30 '24 23:04 zph

Great! I'll look at it this weekend and see if I can get it working ☺️

zph avatar May 05 '24 01:05 zph

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.

bartlomieju avatar May 05 '24 13:05 bartlomieju

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!

zph avatar May 06 '24 15:05 zph

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.

rgbkrk avatar May 16 '24 17:05 rgbkrk

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.

zph avatar May 16 '24 23:05 zph

Landed #23826! You can either rebase/merge main or kick off a new PR. Feel free to ping me, I'm happy to help.

rgbkrk avatar May 22 '24 12:05 rgbkrk

Awesome, thanks @rgbkrk

zph avatar May 22 '24 20:05 zph

@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 avatar Jul 01 '24 00:07 bartlomieju

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 avatar Jul 01 '24 01:07 zph

@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

zph avatar Jul 01 '24 04:07 zph

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 prompting 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!

bartlomieju avatar Jul 03 '24 00:07 bartlomieju

This is now working: Screenshot 2024-07-01 at 02 19 45

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.

bartlomieju avatar Jul 04 '24 21:07 bartlomieju

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.

zph avatar Jul 04 '24 21:07 zph

Confirmed looks good in:

  • vscode's jupyter notebook environment
  • web ui for jupyter notebooks

zph avatar Jul 04 '24 22:07 zph