-
Notifications
You must be signed in to change notification settings - Fork 435
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
Switch Binder and Colab links in notebooks to markdown block quote #1398
Conversation
Check out this pull request on You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB. |
Code Climate has analyzed commit 747764f and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
When I added the original Binder and Colab links, I couldn't get markdown tables to render inside Jupyter notebooks, and in this PR they're not rendering either? eg https://github.com/stellargraph/stellargraph/blob/d1bcf9153e7b235b13bd06064afbffb6e56ed2ab/demos/basics/loading-networkx.ipynb |
Yeah, it's not working on Github or nbviewer, despite looking reasonable in Jupyter lab: and also on Read the Docs: https://stellargraph--1398.org.readthedocs.build/en/1398/demos/basics/loading-networkx.html I'm thinking I may switch this to something clunky like a block quote: We can potentially post-process the block quote to look better on Read the Docs, with a technique along the lines of #1401. |
This reverts commit aae2ed7.
This causes problems with some of the notebooks that have a block quote as the first element of the markdown cell after the image:
The text of the quote disappears on Read the Docs due to spatialaudio/nbsphinx#450 and/or jupyter/nbconvert#1245. I think we can work around this by moving that quote to be immediately after the title, e.g.: for https://github.com/stellargraph/stellargraph/blob/aae2ed75612a4504b74f856c65106c45af07d0da/demos/node-classification/gcn/gcn-cora-node-classification-example.ipynb
I'm going to do this as a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frustrating that the formatting can't work well in all targets, but I think this is a good compromise
Pull request #1319 (DGCNN demo) added a new demo, and merged just before pull request #1398 (switching cloud runner cells to use a markdown quote). This hit some "merge skew" (https://bors.tech/): the latter changed the expected format of the notebooks, but the update wasn't applied to the DGCNN demo. Thus CI is failing: https://buildkite.com/stellar/stellargraph-public/builds/3625 This does the requisite formatting.
Pull requests #1398 (using pure markdown for colab links) and #1394 (no nested formatting in notebooks) merged at about the same time, but they conflicted. The syntax used in #1398 failed the check of #1394, and indeed revealed a missing piece: it's ok to have a link that contains an image in notebook markdown, because this passes through the reStructuredText conversion ok. (This was yet more "merge skew".) This also has to replace an instance of ***bold-italic*** with plain **bold** in the GCN-LSTM notebook, because the CI requires that this doesn't appear (as it doesn't render correctly on Read the Docs; #1394 discusses this in more detail).
…1413) This works around problems in the Markdown to reStructuredText conversion that nbsphinx does when rendering notebooks. In particular, spatialaudio/nbsphinx#450 and/or jupyter/nbconvert#1245. These issues mean that if a cell starts with a block quote, it will merge with a reStructuredText directive at the end of the previous cell, because an appropriate separating comment is not inserted. Pull request #1398 gives an example of this; the `loading-networkx.ipynb` notebook's rST from `nbconvert` ends up looking like: ```rst Run the master version of this notebook: |Open In Binder| |Open In Colab| .. |Open In Binder| image:: https://mybinder.org/badge_logo.svg :target: https://mybinder.org/v2/gh/stellargraph/stellargraph/master?urlpath=lab/tree/demos/basics/loading-networkx.ipynb .. |Open In Colab| image:: https://colab.research.google.com/assets/colab-badge.svg :target: https://colab.research.google.com/github/stellargraph/stellargraph/blob/master/demos/basics/loading-networkx.ipynb This demo explains how to load data from NetworkX into a form that can be used by the StellarGraph library. `See all other demos <../README.md>`__. ``` The "This demo ..." text gets included as part of the `|Open In Colab|` substitution definition. This notebook thus isn't displayed correct, and potentially has warnings/errors when rendering. If this was all in a single cell, the rST would look like: ```rst Run the master version of this notebook: |Open In Binder| |Open In Colab| .. |Open In Binder| image:: https://mybinder.org/badge_logo.svg :target: https://mybinder.org/v2/gh/stellargraph/stellargraph/master?urlpath=lab/tree/demos/basics/loading-networkx.ipynb .. |Open In Colab| image:: https://colab.research.google.com/assets/colab-badge.svg :target: https://colab.research.google.com/github/stellargraph/stellargraph/blob/master/demos/basics/loading-networkx.ipynb .. This demo explains how to load data from NetworkX into a form that can be used by the StellarGraph library. `See all other demos <../README.md>`__. ``` Where the extra `..` is an empty comment, which is the recommended way to separate a quote from an earlier construct (https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#block-quotes). This pull request fixes this in two ways: - flagging any markdown cells that have a block quote at the start, since they may hit these problems (except for the cloud-runner ones, which are entirely a quote now #1398) - allowing the first cell to also include a summary after title, where the summary is denoted by a `> ...` quote element Examples of the failures (on the notebooks that were fixed here): https://buildkite.com/stellar/stellargraph-public/builds/3613 Comparison: (see #1413 for images) - Before: The notebook summary is missing. https://stellargraph--1398.org.readthedocs.build/en/1398/demos/basics/loading-pandas.html - After: The notebook summary is included. https://stellargraph--1413.org.readthedocs.build/en/1413/demos/basics/loading-pandas.html Also see: #1398 (comment) See: #1293
The raw HTML table was not converting to reStructuredText through
nbsphinx
(which, I think, usesjupyter nbconvert --to=rst
, which uses pandoc). The output lost the links + images (see more details aboutpandoc
directly at the end of this PR).This PR updates the links to be normal markdown links + images, which ensures they do get converted correctly. To distinguish these links from normal body text, these are formatted as a block-quote (
> ...
). This gives acceptable (not great) formatting on each of the major platforms one might be using to look at this:Note that both Github and nbviewer have CSS that forces images to be formatted as block elements, and so they're positioned separately (instead of inline elements as is the default).
We can potentially improve the Read the Docs formatting with custom CSS or some post-processing (similar to #1401).
See: #1293
Reproducing pandoc's behaviour
The output is just
Y
by itself, the link and image both completely disappear.From their docs
That doesn't include reStructuredText.
Strangely enough (and in contradiction), it does seem to pass through the various
<table>
tags. The converted rST for the original table is contains the text and the table-related tags, but nothing else: