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

zph · 2024-04-28T18:00:18Z

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

Get review on Jupyter mechanics to confirm correctness
Get review on Rust implementation / code style
Add JSDoc to confirm/prompt for jupyter

PR guidelines

Before submitting a PR, please read https://docs.deno.com/runtime/manual/references/contributing
Comply with title guidelines
Ensure there is a related issue and it is referenced in the PR text.
Ensure there are tests that cover the changes.
Ensure cargo test passes.
Ensure ./tools/format.js passes without changing files.
Ensure ./tools/lint.js passes.
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.

CLAassistant · 2024-04-28T18:00:23Z

All committers have signed the CLA.

zph · 2024-04-28T18:08:50Z

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

rgbkrk · 2024-04-29T23:53:56Z

cli/ops/jupyter.rs

+     *  routing prefix as the execute_reply in order for the frontend to receive the message.
+     * """
+     */
+    last_request


AFAIK, I think this is correct.

Thank you 👏

Confirmed this by the way and incorporated it directly into the new library to make it easier to do.

cli/js/40_jupyter.js

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

zph · 2024-04-30T18:21:06Z

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

cli/js/40_jupyter.js

zph · 2024-04-30T23:17:23Z

@dsherret I'll treat this as blocked pending: #23617

bartlomieju · 2024-05-05T00:24:51Z

cli/tools/jupyter/jupyter_msg.rs

+  pub(crate) fn allow_stdin(&self) -> bool {
+    self.content["allow_stdin"].as_bool().unwrap_or(false)
+  }
+
+  pub(crate) fn value(&self) -> &str {
+    self.content["value"].as_str().unwrap_or("")
+  }


Are you sure that these values are always present in the Jupyter messages? If not, we should use self.content.get(...) instead and provide default values that are "falsy".

bartlomieju

Really nice work @zph! This is a great improvement to our Jupyter kernel support 👍

Now that #23592 has landed I think we are ready to rewrite op_jupyter_input to be a synchronous op and finalize this PR so it can land in Deno v1.44.

Let me know if you need any help with rebasing and/or turning that op into a sync code.

zph · 2024-05-05T01:57:42Z

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

bartlomieju · 2024-05-05T13:03:47Z

Ooops, I actually meant #23622 and @dsherret told me that it's still not enough to address #23617.

zph · 2024-05-06T15:22:09Z

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!

rgbkrk · 2024-05-16T17:37:33Z

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.

zph · 2024-05-16T23:04:22Z

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.

rgbkrk · 2024-05-22T12:44:49Z

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

zph · 2024-05-22T20:14:01Z

Awesome, thanks @rgbkrk

bartlomieju · 2024-07-01T00:23:49Z

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

zph · 2024-07-01T01:44:38Z

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 <#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 <#23592 (comment)>, 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 · 2024-07-01T04:36:46Z

@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

Moves the ZeroMQ messaging server to a separate thread. This will allow to run blocking JS code and maintain communication with the notebook frontend. Towards #23592 Towards #24250 Closes #23617

bartlomieju · 2024-07-03T00:06:57Z

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 · 2024-07-04T21:36:48Z

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.

bartlomieju

LGTM, exciting work @zph! Thanks for the PR

zph · 2024-07-04T21:54:17Z

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 · 2024-07-04T22:13:15Z

Confirmed looks good in:

vscode's jupyter notebook environment
web ui for jupyter notebooks

…24373) Moves the ZeroMQ messaging server to a separate thread. This will allow to run blocking JS code and maintain communication with the notebook frontend. Towards denoland#23592 Towards denoland#24250 Closes denoland#23617

…23592) Closes: denoland#22633 This commit adds support for `confirm` and `prompt` APIs, that instead of reading from stdin are using notebook frontend to show modal boxes and wait for answers. --------- Co-authored-by: Bartek Iwańczuk <[email protected]>

rgbkrk reviewed Apr 29, 2024

View reviewed changes

cli/js/40_jupyter.js Show resolved Hide resolved

zph marked this pull request as ready for review April 30, 2024 02:03

zph added 3 commits April 29, 2024 19:05

Formatting

f9a7a3e

JSDoc

49c60d7

zph force-pushed the jupyter-add-confirm-and-prompt-support branch from 9d07554 to 1cd77e2 Compare April 30, 2024 02:05

[ci] bump

2162e33

zph force-pushed the jupyter-add-confirm-and-prompt-support branch from 1cd77e2 to 2162e33 Compare April 30, 2024 02:07

zph changed the title ~~feat(cli/tools/jupyter) support confirm and prompt in notebooks~~ feat(cli/tools/jupyter) support confirm and prompt in notebooks Apr 30, 2024

[ci] After removing leading whitespace in PR

eb7ed61

dsherret changed the title ~~feat(cli/tools/jupyter) support confirm and prompt in notebooks~~ feat(jupyter): support confirm and prompt in notebooks Apr 30, 2024

dsherret reviewed Apr 30, 2024

View reviewed changes

cli/js/40_jupyter.js Show resolved Hide resolved

dsherret reviewed Apr 30, 2024

View reviewed changes

cli/js/40_jupyter.js Show resolved Hide resolved

When defaultValue is empty don't show prompts empty square brackets

f4faf22

bartlomieju added this to the 1.44 milestone May 5, 2024

bartlomieju reviewed May 5, 2024

View reviewed changes

factor out ReplSessionProxy

6600ddd

bartlomieju added 3 commits June 29, 2024 01:29

hoist out replsessionproxy

310a793

oooh, it's compiling :>

dba8b73

fix an op

ac4c0c0

bartlomieju mentioned this pull request Jun 30, 2024

refactor(jupyter): move ZeroMQ server to a separate thread #24373

Merged

bartlomieju added 2 commits July 1, 2024 01:26

Merge branch 'jupyter_zeromq_thread' into jupyter_confirm

e5d1965

prompt and confirm are actually working now

16e5e2f

bartlomieju added 5 commits July 3, 2024 03:36

Merge branch 'main' into jupyter_confirm

1cc0ccd

lint

5517f93

Merge branch 'main' into jupyter-add-confirm-and-prompt-support

ba2c242

Merge branch 'main' into jupyter-add-confirm-and-prompt-support

29339f8

add a comment

31ea718

bartlomieju added 2 commits July 4, 2024 23:37

revert unrelated change

1394e7f

remove unneeded comment

8fcc263

bartlomieju approved these changes Jul 4, 2024

View reviewed changes

bartlomieju enabled auto-merge (squash) July 4, 2024 21:44

bartlomieju merged commit f00f0f9 into denoland:main Jul 4, 2024
17 checks passed

zph deleted the jupyter-add-confirm-and-prompt-support branch July 4, 2024 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zph commented Apr 28, 2024 •

edited by bartlomieju

Loading

CLAassistant commented Apr 28, 2024 •

edited

Loading

zph commented Apr 28, 2024

rgbkrk Apr 29, 2024

zph Apr 30, 2024

rgbkrk May 17, 2024

zph commented Apr 30, 2024

zph commented Apr 30, 2024

bartlomieju May 5, 2024

bartlomieju left a comment

zph commented May 5, 2024

bartlomieju commented May 5, 2024

zph commented May 6, 2024

rgbkrk commented May 16, 2024

zph commented May 16, 2024

rgbkrk commented May 22, 2024 •

edited

Loading

zph commented May 22, 2024

bartlomieju commented Jul 1, 2024

zph commented Jul 1, 2024 via email

zph commented Jul 1, 2024 •

edited

Loading

bartlomieju commented Jul 3, 2024

bartlomieju commented Jul 4, 2024

bartlomieju left a comment

zph commented Jul 4, 2024

zph commented Jul 4, 2024

feat(jupyter): support confirm and prompt in notebooks #23592

feat(jupyter): support confirm and prompt in notebooks #23592

Conversation

zph commented Apr 28, 2024 • edited by bartlomieju Loading

TODO

PR guidelines

CLAassistant commented Apr 28, 2024 • edited Loading

zph commented Apr 28, 2024

rgbkrk Apr 29, 2024

Choose a reason for hiding this comment

zph Apr 30, 2024

Choose a reason for hiding this comment

rgbkrk May 17, 2024

Choose a reason for hiding this comment

zph commented Apr 30, 2024

zph commented Apr 30, 2024

bartlomieju May 5, 2024

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

zph commented May 5, 2024

bartlomieju commented May 5, 2024

zph commented May 6, 2024

rgbkrk commented May 16, 2024

zph commented May 16, 2024

rgbkrk commented May 22, 2024 • edited Loading

zph commented May 22, 2024

bartlomieju commented Jul 1, 2024

zph commented Jul 1, 2024 via email

zph commented Jul 1, 2024 • edited Loading

bartlomieju commented Jul 3, 2024

bartlomieju commented Jul 4, 2024

bartlomieju left a comment

Choose a reason for hiding this comment

zph commented Jul 4, 2024

zph commented Jul 4, 2024

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

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

zph commented Apr 28, 2024 •

edited by bartlomieju

Loading

CLAassistant commented Apr 28, 2024 •

edited

Loading

rgbkrk commented May 22, 2024 •

edited

Loading

zph commented Jul 1, 2024 •

edited

Loading