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 👏

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.

dsherret · 2024-04-30T19:07:31Z

cli/js/40_jupyter.js

+  async function confirm(message = "Confirm") {
+    const answer = await input(`${message} [y/N] `, false);
+    return answer === "Y" || answer === "y";
+  }


This PR looks great, but prompt and confirm are not async functions. Probably we should make op_jupyter_input sync, then block on the future in rust.

I believe the way to do this is to do the following inside the op:

tokio::runtime::Handle::current().block_on(async { // async code goes here })

Makes sense! I was unsure how to make the rust work for sync and will try that out :)

I tried converting to block_on and hit the following error:

thread 'main' panicked at cli/ops/jupyter.rs:53:46: Cannot start a runtime from within a runtime. This happens because a function (like `block_on`) attempted to block the current thread while the thread is being used to drive asynchronous tasks.

I'll dig into that further and if you have advice from already having more context let me know!

So apparently the answer is this isn't possible to do with a single threaded runtime. We'll have to move a bunch of the messaging off into a separate thread then make this other code have the ability to send and receive message from that thread in a synchronous manner.

I opened #23617

Ok! 👍 . Thanks for opening up the other feature request.

dsherret · 2024-04-30T19:20:45Z

cli/js/40_jupyter.js

@zph it seems this doesn't work in vscode. I'm guessing vscode doesn't support it?

Hmm, I validated in the web based jupyter notebook but by architecture I expect it should similarly work in both. VSCode does support the python version of input popups.

I'll report back.

It does work in vscode (screenshot)

But to see it yourself, you'll need to update your deno jupyter kernelspec to point to the debug built binary either by updateing the "argv" location or by overwriting your deno binary with the debug binary. On my mac the location is here and I'm attaching the file content:

❯ cat ~/Library/Jupyter/kernels/deno/kernel.json { "argv": [ "/Users/zph/.local/bin/deno", "jupyter", "--kernel", "--conn", "{connection_file}" ], "display_name": "Deno", "language": "typescript" }

What I'm doing locally to validate this is building the debug binary and installing it into that location, overriding my normal deno. 😸

But viewing it when not passing any defaultValue looks bad b/c of the surrounding brackets. I'll fix it.

Ok, when defaultValue == "" we hide the empty square brackets in f4faf22

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.

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

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

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

CLAassistant commented Apr 28, 2024 •

edited

zph commented Apr 28, 2024

rgbkrk Apr 29, 2024

zph Apr 30, 2024

zph commented Apr 30, 2024

dsherret Apr 30, 2024

dsherret Apr 30, 2024

zph Apr 30, 2024

zph Apr 30, 2024 •

edited

dsherret Apr 30, 2024 •

edited

dsherret Apr 30, 2024

zph Apr 30, 2024

dsherret Apr 30, 2024

zph Apr 30, 2024 •

edited

zph Apr 30, 2024

zph Apr 30, 2024

zph 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

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

Are you sure you want to change the base?

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

Conversation

zph commented Apr 28, 2024 • edited

TODO

PR guidelines

CLAassistant commented Apr 28, 2024 • edited

zph commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zph commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zph Apr 30, 2024 • edited

Choose a reason for hiding this comment

dsherret Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zph Apr 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zph commented Apr 30, 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

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

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

zph commented Apr 28, 2024 •

edited

CLAassistant commented Apr 28, 2024 •

edited

zph Apr 30, 2024 •

edited

dsherret Apr 30, 2024 •

edited

zph Apr 30, 2024 •

edited