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.

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

Add nbdiff-web-export to export HTML diff using just command line #552

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

trams
Copy link

@trams trams commented Nov 22, 2020

This is Proof-of-concept but it should be good enough to actually try to use it

How to try it?
nbdiff ./nbdime/webapp/testnotebooks/remote.ipynb ./nbdime/webapp/testnotebooks/base.ipynb --out diff_with_base.json --include-base
nbdiff-web-export diff_with_base.json ./nbdime/webapp/static/nbdime.js > ./diff.html

Open diff.html in your browser of choice

What has been tested?
Only the commands above

I am thinking about actually combining two command and teaching nbdiff-web-export to build diff and then html. I do not see the reason why I won't be able to import appropriate packages and call some kind of diff method. What do you think about this?

I have no idea how this would work with several changed notebooks in some git commit. I honestly did not try it. If someone can try it I would appreciate

As you can see in code change I am trying to reuse as much as possible existing components: nbdime.js to render the diff. I copy pasted and modified a template (would be nice to just reuse it but yet to figure this out). Added python code is mostly trivial

What is missing?

  1. Any kind of tests
  2. Proper error processing

I would really appreciate your feedback

nbdiff-web-export --ouput-dir output ./nbdime/webapp/testnotebooks/remote.ipynb ./nbdime/webapp/testnotebooks/base.ipynb

or

nbdiff-web-export --output-dir output HEAD~1 HEAD

These would generate inside output directory
diff1.html
diff2.html
....
diffN.html
depending how many files has been changed

it would also drop nbdime.js in the output directory
Alternatively one can use --nbdime_url optional parameter to specify where to get nbdime.js
@trams
Copy link
Author

trams commented Nov 24, 2020

I figured out how to combine two commands i.e. do the diff and then generate html to render it so I will update this pull request

Copy link
Collaborator

@vidartf vidartf left a comment

Choose a reason for hiding this comment

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

This mostly looks good. I'm sure there is some tidying up that I might want to do, but I can do that iteratively after merging. The main concepts look good, so thanks! I have one conceptual question inline (keeping JS separate vs embedding fully).

@@ -286,6 +286,16 @@ def test_nbdiff_app_no_colors(filespath, capsys):
assert 0 == main_diff(args)


def test_nbdiff_app_fail_if_file_is_missing(filespath):
# Simply check that the --color-words argument is accepted, not behavior
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. I just copy pasted the test with a comment

nbdime/webapp/templates/diffembedded.html Outdated Show resolved Hide resolved
@trams
Copy link
Author

trams commented Nov 25, 2020

So things which are not working

  1. Export button does not work (do we need it?)
  2. Hide changes button does not work
  3. The title of the page is generic and should contain the names of notebooks changed.

But I do believe it is already usable. I will definitely try to integrate this tool into our ML CI/CD so I will get a better sense how to make it more user friendly

@trams
Copy link
Author

trams commented Dec 15, 2020

@vidartf I addressed your concerns. Let me know if there is anything else preventing this from merging? Thank you

@vidartf
Copy link
Collaborator

vidartf commented Dec 15, 2020

Oh, I thought your previous comment meant you wanted to go away and try it out some more before coming back and making adjustments. I'll try to have a look this week 👍

Important points:
- Allow for configuration of mathjax and hide unchanged defaults
- Fix read/write encodings so it works on Windows
- Add some error handling code if we end up with broken json for the diff view

Remaining TODO:
- Ensure we build and use a single bundle. Existing nbdime.js does not bundle all the things (fonts, and some chunked JS).
- Find and check a decent default for MathJax: Use CDN as default, or switch to using jupyter-renderer's mathjax3 bundled ?
@vidartf
Copy link
Collaborator

vidartf commented Dec 18, 2020

Hi again, I did some cleanup to align it more with some internal conventions, fixing some issues while doing that. There are a few remaining issues:

  • Ensure we build and use a single static bundle. Existing nbdime.js does not bundle all the things (fonts, and some other chunked JS). This probably means we need another webpack config, and so we need to decide if it should it be an additional one, or if we should modify the existing one. If we add another, it means the PyPI package will become larger, if we modify the existing one, it means the base bundle that always gets loaded will increase in size by quite a bit.
  • Find and check a decent default for MathJax: Use CDN as default, or switch to using jupyter-renderer's mathjax3 bundled ? If we use a CDN as default, it means we cannot get MathJax rendering when offline. If we bundle MathJax, the bundle size will increase as well. This question ties in both to the point above (separate bundle or additional one), as well as the upgrade in Update to Jupyterlab 3.0 #551 to switch to jupyter_server: since jupyter server does not include mathjax, we will have to make a plan for how do get mathjax onto the page for typesetting latex anyways.

@vidartf
Copy link
Collaborator

vidartf commented Dec 18, 2020

Oh, and we will at some points need to add test coverage for the new CLI entry point.

@vidartf vidartf mentioned this pull request Jan 4, 2021
@vidartf
Copy link
Collaborator

vidartf commented Jan 7, 2021

An option to solve the MathJax for #551 is outline here: #551 (comment), but that won't solve the issue for the embedded case, so we will probably need to use a separate JS entrypoint / bundle that includes the MathJax code (making the embed option even heavier). That should still probably be ok for now, and then we can look at trimming that bundle later on.

@trams
Copy link
Author

trams commented Jan 8, 2021

Thank you for all the great feedback. I still did not have a chance to fully go though comments and I do not fully understand all the comments yet.

Meanwhile I had some success integrating this version in our CI/CD and it seems it works fine but to be fair in the last few weeks we did not have a lot of changes to our notebooks so we probably did not hit any edge cases

As for whether to extend the current bundle or to create a separate I do not have specific opinion.

@icankeep
Copy link
Contributor

hello, when the code can be merged? I really need this feature!!

@wenindoubt
Copy link

@trams Thanks so much for starting the work on this. This is exactly what we need as well. Are there any updates on getting this merged?

@trams
Copy link
Author

trams commented Nov 18, 2021

@wenindoubt I did not have a chance to address the maintainer's comments mostly due to lack of expertise (I am a backend developer) and lack of time.
Currently I am trying to get some "buy in" on such a feature inside my company and hopefully we will allocate some time on merging this pull request. But I can't make any promises as of now

I am open on accepting anyone's help! I am open on allocating some time on zoom calls for XP sessions or consultations if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants