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.

[p5fs5pk8q7-jpg]

37376

[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

[Jemmawarren1993]

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

Want my access recover with email and app

[Ezechukwuebuka11]

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. ❤️

[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

[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! 🙏 ❤️

[amit1rrr]

the major missing feature for productivity is the ability to make in-line comments in a PR review

ReviewNB offers in-line commenting for notebook diffs on GitHub PRs. It's a GitHub marketplace app.

It would be ideal to have native support for this in GitHub, meanwhile, ReviewNB has been a reliable alternative for the last 6+ years. We're completely free for open source & academic use—here are some popular OSS repos using ReviewNB.

Disclaimer: I'm the founder of ReviewNB. I've been tracking this community discussion since inception and see many familiar faces of our users here (hello 👋). Since inline commenting has come up often here, I just wanted to share an existing alternative—shared in the spirit of being more helpful than promotional.

[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):

[ethompsy]

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):

This just takes us back to the near unreadable text view which was already rejected by my team because it makes PRs miserable.

[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

[ethompsy]

My team just rejected the use of .ipynb files in our codebase after trying this preview BECAUSE there is no in-line commenting feature.

[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. ⭐

[sonja-hiltunen-ivadolabs]

This is a great feature! The main missing part for me to start using is the "comment" feature, that's crucial for code reviews.

[AlexJBisping]

I second this. We’d use the feature only if commenting on individual lines is possible; otherwise, it’s a non-starter.

Basic syntax highlighting would be nice too.

Thanks for adding this!

[daithi-walker]

This doesn't seem to work for me or others in my org. We've enabled the feature and I've toggled between source and rich view but nothing.

[ignacio-amaya-rovio]

Is this still in preview? Are there plans to make this part of standard offering any time soon?

[tariqshihadah]

I agree with others that in-line comments are an essential feature I'm looking for, though I'd add that cell-level comments would also be a major step in the right direction that would make a big difference for me and my team.

[cgravill]

We remove a lot of metadata from our notebooks to avoid churn between people. We recently found folks who are using this preview were having issues as we'd removed 'display_name'. This apparently leads to failure in diffing:

might you be able to make that optional as it doesn't seem essential for a diff?

[krassowski]

display_name is currently required in the notebook format, see here. You may wish to propose making this optional over in nbformat, but there may be more tools and notebook clients which assume that this is always available, so it could be a breaking change.

[cgravill]

Thanks for that, we should probably stop removing it then. So far we've not run into any issues with other tools.

[Gharalast]

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

[Gharalast]