-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
feat(jupyter): support confirm
and prompt
in notebooks
#23592
base: main
Are you sure you want to change the base?
Conversation
@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 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 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. |
* routing prefix as the execute_reply in order for the frontend to receive the message. | ||
* """ | ||
*/ | ||
last_request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, I think this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed this by the way and incorporated it directly into the new library to make it easier to do.
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. ```
9d07554
to
1cd77e2
Compare
1cd77e2
to
2162e33
Compare
☹ 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. |
confirm
and prompt
in notebooksconfirm
and prompt
in notebooks
confirm
and prompt
in notebooksconfirm
and prompt
in notebooks
async function confirm(message = "Confirm") { | ||
const answer = await input(`${message} [y/N] `, false); | ||
return answer === "Y" || answer === "y"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! I was unsure how to make the rust work for sync and will try that out :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #23617
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! 👍 . Thanks for opening up the other feature request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zph it seems this doesn't work in vscode. I'm guessing vscode doesn't support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But viewing it when not passing any defaultValue
looks bad b/c of the surrounding brackets. I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, when defaultValue == ""
we hide the empty square brackets in f4faf22
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("") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Great! I'll look at it this weekend and see if I can get it working |
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 |
Solves: #22633
Supports
confirm
andprompt
with custom versions used when inside a Jupyter Notebook with Deno kernel.The desired behavior (per python reference and docs):
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
andprompt
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:
TODO
PR guidelines
cargo test
passes../tools/format.js
passes without changing files../tools/lint.js
passes.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.