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

Draft of text-based notebooks #116

Merged
merged 21 commits into from
Apr 1, 2020
Merged

Draft of text-based notebooks #116

merged 21 commits into from
Apr 1, 2020

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Mar 31, 2020

Builds on top of #107. In this implementation myst-nb replaces myst-parser as the markdown parser and tests all files for a "notebook signature".

Documented in https://306-241268229-gh.circle-artifacts.com/0/html/use/markdown.html

@chrisjsewell
Copy link
Member Author

Some thought needs to be put into how people might use this with jupytext, i.e. if there is both a notebook.ipynb and notebook.md in the source folder. At the moment its rather undetermined which one will be used to actually build the document with. I raised this in sphinx-doc/sphinx#7324, but they only went half-way with my suggestion; such that a warning will now be raised, but really you should be able to select a preferential extension.

@choldgraf
Copy link
Member

Fantastic - I think this is one of the final pieces in the puzzle!

re: extension preference, this is something that we could also control at the jupyter-book level. Basically we could do a quick check for any files with duplicate filenames, and for any that are found, we could load one of them into exclude_patterns before passing the whole thing to Sphinx

@jstac
Copy link
Member

jstac commented Mar 31, 2020

Great work @chrisjsewell.

(Regarding the source-for-build policy at the jupyter-book level, it is a potential source of confusion isn't it? Given that most people are pretty careless when it comes to reading the documentation. Perhaps it's worth having a clear statement of the default (either md or ipynb), although still allowing the user to customize. A natural default would be to build from the md whenever they exist. That will never trouble people who use only ipynb.)

@chrisjsewell
Copy link
Member Author

The correct line numbers are now reported for these text-based notebooks 😄

Shall I also convert some more of the notebooks in the documentation to markdown files? Obviously the markdown ones are a lot easy to edit (and better for git diffs), but probably also want some ipynb in their for example.

@choldgraf
Copy link
Member

I'm +1 for the idea. Let's leave at least one ipynb file in there, but I'm fine with almost all of them being .md.

@choldgraf
Copy link
Member

@jstac I agree - my intuition is that "markdown should take precedence by default". That way, we won't run into issues if, for example, somebody edits content directly on a github repository, which isn't really possible to do with the ipynb files

@jstac
Copy link
Member

jstac commented Mar 31, 2020

Yes, "markdown takes precedence whenever it exists" is an easy rule for the user to remember.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Mar 31, 2020

Yes, "markdown takes precedence whenever it exists" is an easy rule for the user to remember.

Yep that would be the preference. However, as mentioned above sphinx doesn't handle this very well (it wasn't really written with multiple file types in mind). I've added the .md suffix before the .ipynb one, in the sphinx setup(), which should make that happen. But yeh something like the CI might be able to handle this better.

A few edge cases:

  • If you remove name.ipynb and add name.md (i.e. same name, different extension) before running sphinx; this is "confusing" sphinx and breaking the cache (by not removing the name.ipynb).

  • If a text-based notebook fails to execute, and so doesn't get updated with exec metadata, then down the line you get:

image

This is only because jupyter_sphinx is trying to retrieve it to output a name.py file. This output should be made optional if language_info doesn't exist.

@chrisjsewell
Copy link
Member Author

A few edge cases:

  • If you remove name.ipynb and add name.md (i.e. same name, different extension) before running sphinx; this is "confusing" sphinx and breaking the cache (by not removing the name.ipynb).

  • If a text-based notebook fails to execute, and so doesn't get updated with exec metadata,

Added 'fixes' for these cases

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Apr 1, 2020

All the documentation notebooks are now markdown files, and I have added a footnote with download links for the files as both markdown and (executed) notebooks, e.g. https://318-241268229-gh.circle-artifacts.com/0/html/use/basic.html#download
As discussed in executablebooks/cli#59, this may be a stop-gap for adding these in a drop-down menu as part of the theme. Plus there is also the whole binder/thebelab links to consider

@chrisjsewell chrisjsewell marked this pull request as ready for review April 1, 2020 09:56
@chrisjsewell
Copy link
Member Author

This can be merged if people are happy

choldgraf
choldgraf previously approved these changes Apr 1, 2020
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

I have a few minor comments but this looks great in general. I think one small round of tweaks and then we should merge!

Sphinx using the MyST parser.[^download]

[^download]: This notebook can be downloaded as
**{jupyter-download:notebook}`basic`** and {download}`basic.md`
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason to use jupyter-download here and not just download?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the file is taken from the jupyter_execute folder, i.e. the post-execution notebook, not the source folder as download looks at. I guess see jupyter-sphinx for the actual logic.

Copy link
Member

Choose a reason for hiding this comment

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

right - I remember now, thanks

docs/use/markdown.md Show resolved Hide resolved
myst_nb/__init__.py Show resolved Hide resolved

:raises MystMetadataParsingError if the metadata block is not valid JSON/YAML

NOTE: we assume here that all of these directives are at the top-level,
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty big assumption, no? So myst-nb will not support nested directives?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not all directives, just {code-cell}. This is necessary for compliance with round trip conversion with notebooks, as obviously you can't have a code cell nested in a markdown cell. When I say 'assume`, I mean we just ignore any nested code cells, which will, if present, then raise an error on the sphinx parse, because it won't have a directive associated with it.

Copy link
Member

@choldgraf choldgraf Apr 1, 2020

Choose a reason for hiding this comment

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

Ah - that makes sense then, since contents of {code-cell}s don't support myst-markdown

Copy link
Member Author

@chrisjsewell chrisjsewell Apr 1, 2020

Choose a reason for hiding this comment

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

Yep thats the one 'compromise' we make, in terms of treating the whole file as a notebook (rather than 'extracting' notebooks from files, like jupyter-sphinx does): you can't do something like

````{note}
```{code-cell}
a = 1
```
````

with the benefit of being abled to treat them exactly the same a standard notebooks; using top-matter for metadata and integrating them with jupyter-cache etc.

myst_nb/parser.py Outdated Show resolved Hide resolved
Co-Authored-By: Chris Holdgraf <choldgraf@berkeley.edu>
@choldgraf choldgraf merged commit 0919b71 into master Apr 1, 2020
@choldgraf choldgraf deleted the text-based-exec branch April 1, 2020 16:46
@choldgraf
Copy link
Member

boom! I am very excited, will test out today

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