Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(jupyter-kernel): accept nested objects from display calls #20537

Merged
merged 4 commits into from
Sep 17, 2023
Merged

feat(jupyter-kernel): accept nested objects from display calls #20537

merged 4 commits into from
Sep 17, 2023

Conversation

rgbkrk
Copy link
Contributor

@rgbkrk rgbkrk commented Sep 17, 2023 •
edited
Loading

Closes #20535.

Screenshots

JSON

image

Vegalite

image

Implementation

Instead of going the route of recursively getting all the objects under application/.*json keys, I went with JSON.stringifying in denospace then parsing it from rust. One of the key benefits of serializing and deserializing is that non-JSON-able entries will get stripped automatically. This also keeps the code pretty simple.

In the future we should only do this for application/.*json keys.

cc @mmastrac

@rgbkrk rgbkrk force-pushed the jupyter-display-accept-json-payloads branch from 2eb9464 to acadecd Compare September 17, 2023 03:16
@rgbkrk
Copy link
Contributor Author

rgbkrk commented Sep 17, 2023 •
edited
Loading

@domoritz -- jupyter kernel for deno pushing vegalite for rendering ^^

@rgbkrk rgbkrk force-pushed the jupyter-display-accept-json-payloads branch from acadecd to c2be89e Compare September 17, 2023 03:41
@mmastrac
Copy link
Contributor

I will have to take a closer look at this! It may actually fix the promise error reported in a different bug, but I'm not totally sure we want to go through JSON for everything.

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Sep 17, 2023 •
edited
Loading

Unless I'm mistaken this is only for when an object has the Symbol.for("Jupyter.display") property, which only contains media bundles ({ 'text/plain': ..., 'application/json': ..., 'text/html': ... }) that Jupyter consumes via the protocol for execute_result and display_data.

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Sep 17, 2023 •
edited
Loading

By the way I only started learning Rust because of this project so please feel free to treat my code as such. 🙃

@rgbkrk rgbkrk force-pushed the jupyter-display-accept-json-payloads branch 2 times, most recently from d2ddd4a to 8c879da Compare September 17, 2023 04:13
mmastrac
mmastrac suggested changes Sep 17, 2023
View reviewed changes
Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change, but I think this looks good.

cli/tools/jupyter/server.rs Outdated Show resolved Hide resolved
@rgbkrk
Copy link
Contributor Author

rgbkrk commented Sep 17, 2023

This doesn't handle the case when someone throws inside of their Symbol.for("Jupyter.display") function. I'm going to tackle that now.

@rgbkrk
Copy link
Contributor Author

rgbkrk commented Sep 17, 2023

Ok, exceptions will log to the jupyter server (with eprintln) and the user will at least see the object as if it went through a regular inspection:

image

mmastrac
mmastrac reviewed Sep 17, 2023
View reviewed changes
cli/tools/jupyter/server.rs Outdated
eprintln!("Unexpected response from Jupyter.display: {:?}", json_str);
eprintln!(
"Unexpected response from Jupyter.display: {:?}",
response.result.clone().value.unwrap_or_default()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skip the unwrap_or_default call here as the "debug"-style print (ie: :?) will show us the None.

@mmastrac
Copy link
Contributor

Looking good! One very small last change and we can merge this.

@rgbkrk rgbkrk force-pushed the jupyter-display-accept-json-payloads branch from 350ecdd to b1599ea Compare September 17, 2023 15:40
@mmastrac mmastrac enabled auto-merge (squash) September 17, 2023 16:38
mmastrac
mmastrac approved these changes Sep 17, 2023
View reviewed changes
Copy link
Contributor

@mmastrac mmastrac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mmastrac mmastrac merged commit 81d6ea8 into denoland:main Sep 17, 2023
12 checks passed
@mmastrac
Copy link
Contributor

Thanks for the quick fixes, @rgbkrk

@rgbkrk rgbkrk deleted the jupyter-display-accept-json-payloads branch September 17, 2023 17:05
@rgbkrk
Copy link
Contributor Author

rgbkrk commented Sep 17, 2023

Thanks for the review! We can do some really cool things now. 😄

@rgbkrk rgbkrk mentioned this pull request Sep 17, 2023
Merged
mmastrac added a commit that referenced this pull request Sep 18, 2023
@rgbkrk @mmastrac
fix(jupyter-kernel): don't log errors from objects without a `Symbol.…
ee38bbb 
…for("Jupyter.display")` (#20546)

Fast follow up to #20537.

Before:


![image](https://github.com/denoland/deno/assets/836375/8a12e83d-9008-419b-bd1f-24c0ac90afd3)

After:

<img width="235" alt="image"
src="https://github.com/denoland/deno/assets/836375/467bf381-278e-4577-a980-7b0ddb08d2af">

---------

Co-authored-by: Matt Mastracci <[email protected]>
@rgbkrk rgbkrk mentioned this pull request Oct 8, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmastrac mmastrac mmastrac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[jupyter-kernel] JSON output
2 participants
@rgbkrk @mmastrac