Jupyter Notebook Rich Diff Feedback

[gwincr11]

Hello and thank you for testing out the new Python Notebook Rich Diff feature. Please let us know if you see any issues, errors or room for improvement. ❤️

This is available as a feature preview

[krassowski]

This is really awesome, thank you for working on this! Some minor issues/suggestions for improvement:

• Notebooks added for the first time in a commit are showing in a rich diff view which takes more screen estate and may be slower than simple rich display. Is this worth a special case (not sure here)?
• I see a lot of "Metadata changed" messages even on newly added notebooks; maybe this could be less prominent, e.g. an icon on a side?
• A dark theme support would be a great enhancement
• I have many large notebooks and encountered "The notebook took too long to render." a few times. I would not mind having to click "wait a little longer" if it meant that I can see a diff for the larger notebooks, as usually those are the ones which I am most interested in diffing in the UI

[gwincr11]

Thanks for the feedback! We really appreciate it.

Notebooks added for the first time in a commit are showing in a rich diff view which takes more screen estate and may be slower than simple rich display. Is this worth a special case (not sure here)?

• We choose this on purpose for reasons :), we actually started by showing the notebook but it may hinder longer term plans.

I see a lot of "Metadata changed" messages even on newly added notebooks; maybe this could be less prominent, e.g. an icon on a side?

• We have discussed this as well, it is good to hear we are on the right track and will think about how to alleviate some of this in the layout.

A dark theme support would be a great enhancement

• We are on the same page here.

I have many large notebooks and encountered "The notebook took too long to render." a few times. I would not mind having to click "wait a little longer" if it meant that I can see a diff for the larger notebooks, as usually those are the ones which I am most interested in diffing in the UI

• This is a tricky balancing act for us. We have some ideas around how to speed up Notebook diff rendering and are currently working on performance, it would also be helpful to do some performance work on NbDime itself, @vidartf and I have discuss this issue in particular Support cell IDs jupyter/nbdime#553 I would love to see support for that land. I am also working on some threading/asyncio work on our side that could help with performance of certain fetching operations on our side. Performance work is always a preferred path to increasing timeouts for us, as timeout extension tends to cause issues at our scale.

[ShaklEl3tar]

https://amazoon-eg.com/#/pages/records/index?type=1

[HuangJunye]

Thanks for making this feature available! We have been using nbreview plugin to review jupyter notebooks but jumping between the two platforms is confusing. It's great to have the rich diff for jupyter notebook built-in on github. However, I have many notebooks that face "the notebook took too long to render" error which makes this feature unusable in most cases. I agree with the OP of this comment that longer notebooks are the ones that will benefits from the rich diff. I hope you can find a way to improve the performance.

[nicocanali]

Another small inconvenience is that the "expand all" button doesn't work at the moment

Expanding manually (by section), however, does work:

[nicocanali]

Also interesting is that the expand all button doesn't always show: for example, a PR with multiple files shows it, a PR with a single file doesn't 🤷

[nicolaskruchten]

I will second the comment about shrinking the "Metadata Changed" blocks, as I have tons of them and they're not very informative at the moment... all empty?

[nicolaskruchten]

For output blocks with multiple mime-types, maybe a different heuristic for choosing the default? Plotly plots output as huge empty areas and in order to see them I must manually choose the PNG type:

[gwincr11]

Also there is an interface for switching the displayed output type, but it only displays on hover. We hove to change this to make it more obvious.

[nicolaskruchten]

This is much better, thank you! Now just as a pie in the sky thought, it would be amazing to have the regular image-diffing tools appear here so I could do swipe/onion-skin/whatever comparisons between the charts :)

[gwincr11]

Yes that would be a nice touch. We could consider trying to upstream it to nbdime at some point.

[joelostblom]

We are exploring ways to dynamically execute plotly content, but due to security concerns this work is likely to be a ways off.

@gwincr11 Would there be similar concerns around rendering Vega-Lite / Altair charts? These require calling out to a CDN to render a JSON spec, but other than that does not contain much code that is needed to be executed in the file itself. You can see an example in this PR where the chart in the first cell is not rendered, but it is rendering when enabling the png backend in the second cell (as expected) https://github.com/joelostblom/test-notebook-pr/pull/2/files.

This is not just an issue with PRs and diffs, but a general issue view these charts on GitHub, see for example https://github.com/joelostblom/test-notebook-pr/blob/ae5969a5fd395c4f2ffb2301b4f0fa5983f12227/altair-test.ipynb. I think it would be quite powerful if we could see these charts directly on GitHub without including PNG representations of them. Do you know if this is worked on or where I could suggest it as a feature request? I tried here over a year ago but haven't gotten a response https://github.com/orgs/community/discussions/16963

[gwincr11]

Hi 👋

We cannot allow external calls on GitHub for security reasons.

[kennysong]

This is a great feature and our team would happily pay for it. (We use ReviewNB right now.)

Noticed a few UX bugs in the diff for a new cell:

[gwincr11]

Thanks for the feeback, glad your are finding the tool useful! Is this a public diff by chance? If so do you mind sharing a link?

[kennysong]

Sure, here's a public diff that's similar to the one above: https://github.com/kennysong/notebook-diff-example/pull/2/files

I think the "tooltip is partially occluded" problem is already fixed.

[mlucool]

Hi @gwincr11, it is possible to give me access try out this feature? Looking forward to using it well!

[gwincr11]

Hi @mlucool

We have a signup form available here: https://docs.google.com/forms/d/e/1FAIpQLSe7UL0qzaP_rRvnSNzGQgE4YmEJpMENWyDISDBleCz_IQLlFQ/viewform

[mlucool]

Thanks! Can you pin the form to the top of this in case others may want to see this as well?

[gwincr11]

Ok you are in.

[joelostblom]

Overall this is so great! One big issue when teaching how to work with notebooks is that students find it intimidating to version control them and this will help a lot to make that process friendlier.

1. Expand notebook metadata by default (cell metadata can remain collapsed as it is now). It is easy to miss this otherwise on top of the notebook and there can be significant changes here that are nice to see by default.

2. I really like that text is softwrapped for markdown cells just as it is for markdown documents, makes it much easier to read! I am not as fond of it for code cells, but if there this is an all or nothing situation then I think it is better to keep it as is and wrap both code and text.

3. It seems like I can't comment on individual lines in the rich diff? I don't see the blue plus sign show up when I hover

4. Sometimes there are big whitespaces on both the left and right hand side which I don't really know what they mean:
   

5. It's really nice to see visualizations show up in the diff when a chart is changed! I think it would be great to support this for libraries that create visualizations that rely on web technologies rather than images as well. I am mainly thinking about Vega/Vega-Lite/Altair myself, but there are many others such as Bokeh, Plotly, etc. It would be great if these are both rendered in notebooks by default and show up in rich diffs as well. Examples of a notebook where altair visualization do not show up https://github.com/joelostblom/altair_ally/blob/main/doc/examples.ipynb.

6. There seems to be an issue with marking output as changed when it is actually only output metadata such as execution count that has changed. In the example below you can see that there the entire output chunk is marked as changed in the rich diff, there is actually nothing changed in the actual output, just the output metadata as we can see in the source diff. I can't make this notebook public, but I have invited you to the repo @gwincr11

   

   

[gwincr11]

Thanks for the wonderful feedback, we are reviewing it all and considering some changes.

Here is a few notes from our review:

Expand notebook metadata by default (cell metadata can remain collapsed as it is now). It is easy to miss this otherwise on top of the notebook and there can be significant changes here that are nice to see by default.

Many new notebooks, diffing a brand new notebook, we have seen have thousands of lines of meta data at this level, showing it all the time is sometimes a very bad user experience. This is why we have chosen to toggle it. Is this useful to expand on changed notebooks?

I really like that text is softwrapped for markdown cells just as it is for markdown documents, makes it much easier to read! I am not as fond of it for code cells, but if there this is an all or nothing situation then I think it is better to keep it as is and wrap both code and text.

We will maintain softwrapping as this is how GitHub code diffs also behave.

It seems like I can't comment on individual lines in the rich diff? I don't see the blue plus sign show up when I hover

We would love to support commenting, but do not at this time.

Sometimes there are big whitespaces on both the left and right hand side which I don't really know what they mean:

Can you provide an example diff so we can see this?

It's really nice to see visualizations show up in the diff when a chart is changed! I think it would be great to support this for libraries that create visualizations that rely on web technologies rather than images as well. I am mainly thinking about Vega/Vega-Lite/Altair myself, but there are many others such as Bokeh, Plotly, etc. It would be great if these are both rendered in notebooks by default and show up in rich diffs as well. Examples of a notebook where altair visualization do not show up

We would love to get something working with this. Our current priority is ensure that all Notebook renders/diffs are as secure as possible. Rendering user submitted javascript is an attack vector we have to take very seriously, so we have chosen to remove this content at this time.

There seems to be an issue with marking output as changed when it is actually only output metadata such as execution count that has changed. In the example below you can see that there the entire output chunk is marked as changed in the rich diff, there is actually nothing changed in the actual output, just the output metadata as we can see in the source diff. I can't make this notebook public, but I have invited you to the repo

This is an issue that should be opened with nbdiff. I can try to get around to it some time soon.

[joelostblom]

Is this useful to expand on changed notebooks?

I agree that seeing thousands of lines of metadata is not useful in notebooks. I don't know what all this metadata is about and was primarily thinking that it would be useful to see things such as if the kernel (and maybe the Python version was changed), since this could lead to how the notebook runs on your own system if you don't pay careful attention to it and change it back locally.

Can you provide an example diff so we can see this?

The source diff looks like this (it is in the PR of the repo I invited you to). It seems like in this case there is actually a markdown cell with two <br> tags in it which is what generates the spacing. That is probably pretty uncommon to be used in notebooks, so probably no action required here.

This is an issue that should be opened with nbdiff. I can try to get around to it some time soon.

That would be great, thank you!

[joelostblom]

Also, I do think providing a commenting feature is a big one as I can see it being a not so great user experience to toggle between the rich diff for viewing the changes and then the source diffs for adding comments. I think I would have two windows open side to side to deal with this effectively myself.

[joelostblom]

It also seems like I can't edit notebooks directly in the web editor any longer which I believe I could before:

Without the rich diff feedback enabled (our enterprise server)

github.com after opting in to enable the rich diff feedback:

[serghiou]

It's very exciting to be able to render notebooks appropriately! My thoughts so far:

1. As noted before, being able to provide comments in a notebook sent for PR is essential. Without such functionality, whether we can render a notebook appropriately or not is almost useless. Even if it weren't useless, we could render it as a notebook before in VS Code.

2. It's a shame that we cannot differentiate between a Markdown vs. a Code cell. Both look identical to us:

3. As you can see in the picture above:
   a. It's difficult to know where one cell ends and another begins - there's a dark green line but that's very faint.
   b. For some reason, this is a much stronger green than the beautiful green seen when reviewing a non-notebook file.
   c. There are black lines between the numbers on the left - these do not look appealing and are not there in non-notebook files.

Really looking forward to further support for notebooks!

[tcapelle]

I want to be able to make comments on cells when reviewing notebooks

[timatim]

Is this sign-up form still valid? I signed up two days ago and still don't see rich diffs

[therzka]

you're in!

[timatim]

thank you! will post feedback soon

[rodrigoalmeida94]

I would also really like to try this out! Thank you :)

[timatim]

Thank you for letting us test this functionality! I work with notebooks a lot and my old company uses phabricator which has a pretty nice support for jupyter rich diff, glad to see that github is also introducing this!

Some issues I noticed + functionalities I think would be beneficial (apologies if some have been mentioned already, I wrote these before reading others comments):

• incompatibility with dark theme on github
• line count is based on per cell. not sure if this is an issue or intended, but personally i find it easier to reason with global line (code only) count as opposed to local cell line count, as i usually write my notebooks in a way that it can be executed as a script from top to bottom. tho the current local cell line count seems more aligned with line counts in jupyterlab.
• don't know why this is but sometimes selecting code with cursor does not work in rich diff
• this is probably in the works already but would be really nice to be able to comment in-line!
• "metadata changed" seems unclear to me what was changed (or was it designed to show the change?), but i'm glad you're including this! I remember when i was using phabricator, they would show a rich diff of a notebook with no lines highlighted when metadata changed, and that was really counterintuitive.

Hope this is helpful and very much looking forward to further iteration on this crucial functionality!

[gwincr11]

Thanks for the feedback!

• incompatibility with dark theme on github
  • Can you explain what you mean by this, you should be seeing the notebook diff in dark mode as dark mode. Some of the colors may slightly differ based on what version of dark mode you have as we do not have 1:1 support for all themes but you should at least get a dark mode.
• line count is based on per cell. not sure if this is an issue or intended, but personally i find it easier to reason with global line (code only) count as opposed to local cell line count, as i usually write my notebooks in a way that it can be executed as a script from top to bottom. tho the current local cell line count seems more aligned with line counts in jupyterlab.
  • The underlying diff is built on top of the JupyterLab widget tools, this was intentional to keep the experience as close to the JupyterLab enviroment as possible and help the diff expereince stay inline with changes in Jupyter. We are using Jupyters official Diff tool NbDime to accomplish this.
• don't know why this is but sometimes selecting code with cursor does not work in rich diff
  • I have not seen this, but it is interesting. Do you have an example we could see?
• this is probably in the works already but would be really nice to be able to comment in-line!
  • Yes this would be really nice, we agree. Our current thinking on our side is this is way better than what we currently show so it is progress in the right direction and when we have the opportunity we can add comments. At this time giving the community an improved way to view a diff is our priority.
• "metadata changed" seems unclear to me what was changed (or was it designed to show the change?), but i'm glad you're including this! I remember when i was using phabricator, they would show a rich diff of a notebook with no lines highlighted when metadata changed, and that was really counterintuitive.
  • You should be able to toggle that cell open to see what changed.

Thanks for the feedback!

[timatim]

Thanks for the feedback!

• incompatibility with dark theme on github

  • Can you explain what you mean by this, you should be seeing the notebook diff in dark mode as dark mode. Some of the colors may slightly differ based on what version of dark mode you have as we do not have 1:1 support for all themes but you should at least get a dark mode.

I'm not currently seeing the behavior of "noteobok diff in dark mode as dark mode", screenshot attached below

• line count is based on per cell. not sure if this is an issue or intended, but personally i find it easier to reason with global line (code only) count as opposed to local cell line count, as i usually write my notebooks in a way that it can be executed as a script from top to bottom. tho the current local cell line count seems more aligned with line counts in jupyterlab.

  • The underlying diff is built on top of the JupyterLab widget tools, this was intentional to keep the experience as close to the JupyterLab enviroment as possible and help the diff expereince stay inline with changes in Jupyter. We are using Jupyters official Diff tool NbDime to accomplish this.

Got it! that makes sense if you're trying to go for alignment with jupyterlab.

• don't know why this is but sometimes selecting code with cursor does not work in rich diff

  • I have not seen this, but it is interesting. Do you have an example we could see?

example: in https://github.com/timatim/GRU_D/pull/3/files, the cell that defines def test_model_gru_mean. seeing this on both chrome and safari.

• this is probably in the works already but would be really nice to be able to comment in-line!

  • Yes this would be really nice, we agree. Our current thinking on our side is this is way better than what we currently show so it is progress in the right direction and when we have the opportunity we can add comments. At this time giving the community an improved way to view a diff is our priority.

yea agreed this is much better than what we have and a right direction, thank you for working on this!

• "metadata changed" seems unclear to me what was changed (or was it designed to show the change?), but i'm glad you're including this! I remember when i was using phabricator, they would show a rich diff of a notebook with no lines highlighted when metadata changed, and that was really counterintuitive.

  • You should be able to toggle that cell open to see what changed.

I see, I think i ran this on a weird cell where there aren't any metadata and i didn't expect that, but it does seem to be functioning as intended on other cells i've tried.

[gwincr11]

I'm not currently seeing the behavior of "noteobok diff in dark mode as dark mode", screenshot attached below

Can you share what you have your theme setting as? https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-personal-account-settings/managing-your-theme-settings Thanks!

[timatim]

[dantegates]

Is this still active? I signed up two days ago but still see the JSON based diffs.

[MichaelCaplanSeccl]

Mine went active about a week after I signed up!

[dantegates]

just checked again, mine is also active now

[rankarni]

This feature is amazing, thank you for creating it!

It would be nice if markdown cells would be rendered (as you get when reviewing '.md' files).
Also, code should be highlighted in the same way it is for python files (color keyword and stuff like that)

[anirudh-k]

Agree that syntax highlighting should be a feature too. I saw it in their feature preview, so I imagine it's coming?

[gwincr11]

That would be best as a feature request against NbDime, which is the underlying Jupyter Diff tool https://github.com/jupyter/nbdime/

[alexscott-ff]

I'm not able to highlight and select (to copy) the text in the rich diff. Is that expected? On latest Chrome OS X.

[asmeurer]

Had to completely disable this and switch over to just reviewing JSON because of this + no line-by-line comments. It's basically unusable for doing actual reviewing.

[matthewfeickert]

I 100% agree with @asmeurer here. It totally defeats the purpose of review if I can't make comments

• without rich Jupyter notebook diffs
  

• with rich Jupyter notebook diffs
  

@gwincr11 given your comments here https://github.com/orgs/community/discussions/37376#discussioncomment-4788256 it seems like it would be possible for this to be released as a feature without having commenting enabled. If so, this would totally break review for many of us, so will there be a way to disable this as a feature if this decision is made?

[gwincr11]

You can toggle between code view and the rich diff view, and comments will continue to work on the source view.

[gwincr11]

File level commenting is also an option if you are ok with less granular comments https://github.blog/changelog/2023-03-14-comment-on-files-in-a-pull-request-public-beta/

[matthewfeickert]

You can toggle between code view and the rich diff view, and comments will continue to work on the source view.

Ah okay.

I had missed that, but that's a decent workaround for now. Thanks for pointing that out.

File level commenting is also an option if you are ok with less granular comments https://github.blog/changelog/2023-03-14-comment-on-files-in-a-pull-request-public-beta/

While having the ability to give file level comments is useful, it is not a suitable replacement for review.

[j-gooding]

Thank you for starting to integrate nbdime; I've wanted something like this for years.

I have two main pieces of feedback:

1. I don't really care about metadata differences. I'm not sure if I ever have or will. I can imagine it is very important for some people, though. It would be good if there were a user-configurable preference for whether metadata should be collapsed by default.
2. I'm an academic researcher. Most of our notebooks are used for data analysis on large physics datasets. Our notebooks can be 10,000+ lines long to generate highly detailed walk-through reports for a given experiment. I'm unable to view the diffs for nearly all of my notebooks because they are too big and getting timeout errors. Obviously, faster is better, but I don't care how long the diff takes to load so long as it eventually loads. Otherwise, I have to use offline tools, which defeats the purpose and removes the convenience.

Again, thank you so much for working on this. I can't wait to see where it goes.

[gwincr11]

Thanks for the feedback.

As for Metadata be closed by default I think this is an NbDime request.

Large notebooks are very hard for us to handle due to scaling needs of GitHub and ensuring long running notebook renders do not overtake shorter ones. There is some work on the NbDime side to improve the diffing algorithm which may help us support larger notebooks so that maybe worth following with, the pr is currently in progress last I checked.

jupyter/nbdime#639 and jupyter/nbdime#402

[krassowski]

FYI, I added results for benchmarks on cell ID PR in jupyter/nbdime#639 (comment) if you are interested. The speedup for a large notebook (1000 cells) goes from x2.5 speedup for naive case of no changes to as much as x150 when all cells are changed (from over 6 minutes to about 2.5 seconds). I also opened a PR to to expose the cell ID to frontend (vidartf/nbdime#3) to help in finishing that work. @gwincr11 if you have any comments on that I am happy to help with moving it forward.

[krassowski]

jupyter/nbdime#639 was merged and released in nbdime 4.0.0. Due to packaging issues the version to install is 4.0.1 (4.0.0 was yanked).

[krassowski]

Just out of curiosity, did you have a chance to look at the new version? Any blockers for upgrade?

[gwincr11]

Thanks for checking in @krassowski I was able to updated us late last week and everything went smoothly. Thank you and the team for the smooth update! ❤️

[Dadiselef20]

Pull request

[Hengyi-Zhu]

Thank you for your work on this! It's been a while since this has been in feature preview. Wondering if this is an ETA for when this can become a standard feature / if there's a typical amount of time to expect for something from a feature preview to become standard?

[zmbc]

This is a cool feature!

Two issues I've run into:

• Maybe this only happens when running notebooks in VS Code, but for me if I insert cells and re-run, the execution count updates both on the later cells and on their outputs. This is invisible metadata but it still causes the rich diff to mark it as "Outputs changed." See an example here: https://github.com/zmbc/reviewnb-test/pull/3/files
• Maybe related, but for very short outputs, when I hover over them, the "show source" tooltip covers the added output itself like so:

[bicepjai]

Is this available on GitHub enterprise ?

[Ryan-Draves]

I am also interested in this being released for GHES; it'd be a huge help for reviewing notebook changes

[AlexanderLanin]

Any plans for extensions / API to use this UI for other custom diffs? e.g. PlantUML is sometimes difficult to review in text form.

[scottamain]

Thank you for building this functionality! Jupyter notebooks are increasingly a common documentation and programming format and your support for it is much appreciated!

As others have said, the major missing feature for productivity is the ability to make in-line comments in a PR review. I can appreciate how this is a non-trivial feature because you need to match the comments to the exact lines in the "source code," so I wish you god speed on this one. Thank you! 🙏 ❤️

[matthewbbone]

I've requested access to this last week but still don't have access, could I be added?

[gwincr11]

This is now a feature preview you can enable your self, enjoy. https://docs.github.com/en/get-started/using-github/exploring-early-access-releases-with-feature-preview

[vlad-regal]

While this is nice for viewing diffs you don't have access to line comment when reviewing PR's that have Jupyter notebooks so you end up switching between the 2 views. If adding inline review comments is hard to implement at least add a toggle between the 2 views on the PR page.

[scottamain]

If adding inline review comments is hard to implement at least add a toggle between the 2 views on the PR page.

@vlad-regal It is there, at the top corner of each file (I also didn't discover this easily):

[robert-g-green]

Thanks for your work on this, great progress.

I've read several comments & responses about in-line comments and whether they are a worth implementing.

I'd like to add to previous comments that the ability to comment in-line is an essential feature. I would even go so far as to bluntly say (but with no rudeness intended) that if in-line comments were not added then the notebook rich diff rendering will have no purpose. The only reason I would use the diff feature is for PR review.

Adding in-line comments is the next priority.

[smith-kyle]

Shameless plug but GitNotebooks has inline notebook comments

[bdcowman]

Good

[github-actions[bot]]

💬 Your Product Feedback Has Been Submitted 🎉

Thank you for taking the time to share your insights with us! Your feedback is invaluable as we build a better GitHub experience for all our users.

Here's what you can expect moving forward ⏩

• Your input will be carefully reviewed and cataloged by members of our product teams. 
  • Due to the high volume of submissions, we may not always be able to provide individual responses.
  • Rest assured, your feedback will help chart our course for product improvements.
• Other users may engage with your post, sharing their own perspectives or experiences. 
• GitHub staff may reach out for further clarification or insight. 
  • We may 'Answer' your discussion if there is a current solution, workaround, or roadmap/changelog post related to the feedback.

Where to look to see what's shipping 👀

• Read the Changelog for real-time updates on the latest GitHub features, enhancements, and calls for feedback.
• Explore our Product Roadmap, which details upcoming major releases and initiatives.

What you can do in the meantime 💻

• Upvote and comment on other user feedback Discussions that resonate with you.
• Add more information at any point! Useful details include: use cases, relevant labels, desired outcomes, and any accompanying screenshots.

As a member of the GitHub community, your participation is essential. While we can't promise that every suggestion will be implemented, we want to emphasize that your feedback is instrumental in guiding our decisions and priorities.

Thank you once again for your contribution to making GitHub even better! We're grateful for your ongoing support and collaboration in shaping the future of our platform. ⭐