-
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): add handling for comms #24250
base: main
Are you sure you want to change the base?
Conversation
Co-Authored-By: Trevor Manz <[email protected]> Co-Authored-By: Kyle Kelley <[email protected]>
typeof df.head === "function" && | ||
df.toRecords !== void 0 && | ||
typeof df.toRecords === "function" | ||
); |
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 reformatting must have been caused by defaults in Zed. 😅
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'll revert that before merging
.send(StreamContent::stdout( | ||
String::from_utf8_lossy(buf).into_owned(), | ||
)) | ||
.send(StreamContent::stdout(&String::from_utf8_lossy(buf))) |
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 see why you were hoping to pass in a String
here. Lossy creates an owned string whereas std::str::from_utf8
returns a Result<&str>
. We could have runtimelib
's StreamContent
have a method to accept from &[u8]
and create the owned string directly. Alternatively, this could just be:
StreamContent {
name: Stdio::Stdout,
text: String::from_utf8_lossy(buf),
}
@@ -20,7 +21,7 @@ use tokio::sync::mpsc; | |||
use tokio::sync::Mutex; | |||
|
|||
use jupyter_runtime::messaging; | |||
use jupyter_runtime::AsChildOf; | |||
// use jupyter_runtime::AsChildOf; |
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.
Good catch that this got dropped to simplify.
|
||
return Ok(()); | ||
} | ||
JupyterMessageContent::CommClose(_) => { |
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.
We'll want to make sure to delete the comm container for this comm id here.
status: ReplyStatus::Error, | ||
error: Some(ReplyError { | ||
error: Some(Box::new(ReplyError { |
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.
Good catch on this. By the way I did this boxing to pack the enum for jupyter message content more tightly.
...data, | ||
buffers, | ||
close() { | ||
if (closed) { |
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.
We also need to use an op_jupyter_
I assume, to clean up the mapping on the Rust side.
Co-authored-by: Trevor Manz <[email protected]> Signed-off-by: Bartek Iwańczuk <[email protected]>
msgCallback?.({ | ||
...data, | ||
buffers, | ||
}); |
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.
Follow up on comment below, to mirror the send API
msgCallback?.({ | |
...data, | |
buffers, | |
}); | |
msgCallback?.(data, buffers); |
Bikeshedding the API... right now you still need to open the comm with broadcast separately ("comm_open") message. let comm = await Deno.jupyter.commOpen(commId, {
targetName: "jupyter.widgets",
data: { /* anywidget stuff */ },
metadata: { version: "2.1.0" },
});
// either only allow one callback or manage a set of them
comm.on((data, buffers) => {
});
comm.close();
comm.send(data, buffers); or something more webby (e.g., EventTarget, let comm = new Deno.jupyter.Comm(commId, {
targetName: "jupyter.widgets",
data: { /* anywidget stuff */ },
metadata: { version: "2.1.0" },
});
comm.addEventListener("message", (event) => {
let { data, buffers } = event.detail;
});
comm.removeEventListener("message", /* function */ );
comm.dispatchEvent(
new CustomEvent("message", { detail: { data, buffers } })
// or something custom - new Deno.jupyter.MessageEvent(data, { buffers });
);
comm.close(); the latter would be easy to extend for other messages https://jupyter-client.readthedocs.io/en/latest/messaging.html#tearing-down-comms: comm.addEventListener("close", () => {
// front-end closed cleanup stuff
}); |
let Some(comm) = maybe_comm else { | ||
return (serde_json::Value::Null, vec![]); | ||
}; | ||
comm.receiver.resubscribe() |
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.
@manzt @rgbkrk if we go with EventListener
approach for a comm then we can expect more than one consumer of the "comm". That means we need to use tokio::sync::broadcast
channel which is buffered. We can go with a rather big number for the buffer like 1024 or 65536. If we expect only a single consumer then we could use an mpsc
channel that is unbounded. Which one should we go with?
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.
For widgets, I think it will basically only be one comm listener. There is new comm created for each new widget instance.
No description provided.