jupyter_tools: fix out of order display of multiple shapes in static html

[victorpoughon]

Hello! I know we talked on discord about how you're thinking of removing the jupyter display support, but I found a bug in it and fixed it 😁

The problem

When using IPython display from jupyter_tools, there is currently an issue when exporting a notebook to static html. If the notebook contains multiple calls to display(part) displaying multiple parts, the parts will be all displayed at the end of the notebook, instead of after the cell that calls the code.

This is because the javascript code that displays the shape is only executed after the full html is loaded, and it dynamically appends a new div to the end of the notebook container when it runs. Therefore, when viewing the static html, all parts 3D models are at the end of the document. This does not happen interactively because the javascript is executed right away.

Here is a jupyter notebook that show the issue: https://gist.github.com/fouronnes/c6bee77d9895003935a83ff5db11d8ca

Interactively, it works fine. But if you convert the notebook to html with:

jupyter nbconvert test_notebooks/fix_async_display.ipynb --execute --to html

and open the html in firefox, all 3D widgets are at the end, which is not the intended order.

The fix

First I moved the template JS code to its own .js file for lisibility. I also switch to using string.Template instead for f-strings, because escaping { makes the JS code very un-canny. Subsitution with $ is more suited in this case, I think.

Then in the second commit, I fix the issue by immediately returning a div with a unique ID. This creates a HTML DOM element that the javascript code can them fill with its data later.

Here is a screenshot of a side by side compare of 0.7.0 (the left) and this branch (the right). Each html page is rendered with jupyter nbconvert ~/Downloads/build123d_fix_async_display.ipynb --execute --to html

The unknown

The only thing I'm not sure about is this piece of code (currently in dev branch):

else{
        container = parent_element.append("<div/>").children("div:last-child").get(0);
        dims = parent_element.get(0).getBoundingClientRect();
};

I don't understand what this else branch is for. As far as I can tell it should never be executed, and it's producing invalid html anyway. I removed it, but I'm not sure why it was there.

[jdegenstein]

Here is a jupyter notebook that show the issue: https://gist.github.com/fouronnes/c6bee77d9895003935a83ff5db11d8ca

Can you explain how to view this issue? All I see is just the notebook cells with no 3D view. Perhaps a screenshot would be more appropriate?

I don't understand what this else branch is for. As far as I can tell it should never be executed, and it's producing invalid html anyway. I removed it, but I'm not sure why it was there.

As noted in jupyter_tools.py it was based on the same file in CadQuery/cadquery -- we have made very few changes to it. https://github.com/CadQuery/cadquery/blob/master/cadquery/occ_impl/jupyter_tools.py

It might be a few days or longer until we get back to this, the build123d codebase is in the middle of a major refactor.

[victorpoughon]

• Updated the PR with a screenshot
• Interesting, perhaps I should contribute that same patch to CadQuery but I don't use that project at all 🤔
• Yeah no worries, I'm using my own fork right now so there is no rush. I'm happy to rebase the PR when the refactor is finished if that's easier.

[victorpoughon]

I've also pushed a new commit to update the test in test_direct_api.py, should be good now.

[raspberrypisig]

GitHub won’t show interactive output cells. Need to use nbviewer.org

To see the problem more clearly, here is an example (any notebook on gist or github can be viewed on nbviewer.org, including
ones with interactive elements, the below is interactive you can move the models around like you do in ocp_vscode). On some devices, you may need a refresh before seeing the model.

https://nbviewer.org/github/mohankumargupta/learnbuild123d/blob/d6fd1aab5765f4b7f4e61059f649ef2438e1af76/notebooks/testing_multiple_interactive.ipynb

The source code for the example

https://github.com/mohankumargupta/learnbuild123d/blob/d6fd1aab5765f4b7f4e61059f649ef2438e1af76/notebooks/testing_multiple_interactive.py

[raspberrypisig]

Here is the desired result

https://nbviewer.org/github/mohankumargupta/learnbuild123d/blob/980520e42baca7d5013237c7ca83350e66a4775a/pull_request/1.ipynb

The source code used to produce the above thanks to the pull request is in a slightly different form to pull request because I'm not using the _repr_javascript/_repr_html, but using the shape_to_html function directly.

https://github.com/mohankumargupta/learnbuild123d/tree/980520e42baca7d5013237c7ca83350e66a4775a/pull_request

[jdegenstein]

@victorpoughon can you rebase this PR? I believe the major refactor is now complete. Also FYI, we have static type checking (mypy) passing on the codebase, so this PR will need to keep that working as well.

FYI topology.py was split up into multiple files, I think you will find _repr_javascript here: src/build123d/topology/shape_core.py

[victorpoughon]

@jdegenstein Sure! I'll take a look, give me a few days.

[victorpoughon]

I've rebased the branch. Nothing special besides reporting the old topology.py changes to shape_core.py, as you said.

If you want to test it with the test notebook on your side:

• Download this jupyter notebook: https://gist.github.com/victorpoughon/c6bee77d9895003935a83ff5db11d8ca#file-build123d_fix_async_display-ipynb. You can also use any notebook that does display() multiple times in different cells.
• Setup a venv with this branch of build123d and jupyter
• Run the notebook interactively: this should work as usual and display the 3D widgets as before (no changes there)
• Convert the notebook to static html with jupyter nbconvert ~/Downloads/build123d_fix_async_display.ipynb --execute --to html
• Open the created html file in firefox, shape widgets should be visible and in the correct cell order. If you use the dev branch you will see they are all at the end of the notebook (as in the screenshot in my first post).

[victorpoughon]

After some testing, I found a bug in my code :) Using id(shape) for a unique id was a bad idea. I found out that sometimes the id wasn't unique, I guess because python is reusing memory addresses on successive function calls, which makes sense.

I've pushed a new commit that should fix it. The unique id is now made with uuid.uuid4().hex[:8].

[jdegenstein]

I've pushed a new commit that should fix it. The unique id is now made with uuid.uuid4().hex[:8].

Can you instead try hash(shape) ? If it works it should have a nearly zero chance of collisions now that build123d uses cadquery-ocp>7.8

[victorpoughon]

That would work too if shape is hashable, except in the edge case where the same object is displayed more than once in the same notebook. Then div id's aren't unique.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.65%. Comparing base (1740f38) to head (edf1dbd).
Report is 9 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #829      +/-   ##
==========================================
+ Coverage   96.62%   96.65%   +0.03%     
==========================================
  Files          31       31              
  Lines        9142     9177      +35     
==========================================
+ Hits         8833     8870      +37     
+ Misses        309      307       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gumyr]

Unfortunately this change has been messed up by yet another build123d change - this time the test_direct_api.py massive test file has been split into 32 separate tests. There is a test_jupyter.py file which will be where this test needs to go now although if you have suggestions on how the tests might be better structured I'm open to that.

[victorpoughon]

No problem, I just rebased it moving the test to test_jupyter.py. Let me know if that looks good to you.

I haven't changed the div id generation as discussed by @jdegenstein though. It's still using uuid4. I think that's a better solution than hash but I'm open to changing it if you prefer.

[jdegenstein]

I haven't changed the div id generation as discussed by @jdegenstein though. It's still using uuid4. I think that's a better solution than hash but I'm open to changing it if you prefer.

That is fine with me, it can always be changed later if needed.,

[gumyr]

Thank you!