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 mystnb format #456

Merged
merged 18 commits into from
Mar 16, 2020
Merged

Add mystnb format #456

merged 18 commits into from
Mar 16, 2020

Conversation

chrisjsewell
Copy link
Contributor

@chrisjsewell chrisjsewell commented Mar 16, 2020

Heya @mwouts the new format we have discussed for you to look at 😄

closes #447, originally implemented in executablebooks/MyST-NB#82

See https://myst-parser.readthedocs.io and https://myst-nb.readthedocs.io for further details on the format. The file extension is still up for discussion. Also, at present, I copy nbformat and nbformat_minor to the front matter, but that's probably not necessary?

All tests pass, apart from 3 that are skipped due to diffs in newline characters at the end of markdown blocks, which don't actually affect the consistency of the round-trip.

cc'ing @choldgraf @AakashGfude @mmcky @jstac

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 16, 2020

Hmm, all tests are passing for the required python versions, but conda create -q -n jupytext-env --file requirements-dev.txt --file requirements.txt;
doesn't like myst-parser>=0.7.1,<0.8; python_version >= '3.6':

InvalidVersionSpec: Invalid version '0.8;python_version>='3.6'': invalid character(s)

Sure you don't want to just drop python 2.7 & 3.4 in this PR 😉

@codecov
Copy link

codecov bot commented Mar 16, 2020

Codecov Report

Merging #456 into master will increase coverage by 0.00%.
The diff coverage is 99.45%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #456    +/-   ##
========================================
  Coverage   98.91%   98.91%            
========================================
  Files          77       79     +2     
  Lines        7563     7762   +199     
========================================
+ Hits         7481     7678   +197     
- Misses         82       84     +2     
Impacted Files Coverage Δ
jupytext/myst.py 99.31% <99.31%> (ø)
jupytext/formats.py 98.69% <100.00%> (+0.01%) ⬆️
jupytext/jupytext.py 98.67% <100.00%> (+0.08%) ⬆️
tests/test_ipynb_to_myst.py 100.00% <100.00%> (ø)
tests/test_mirror.py 100.00% <100.00%> (ø)
tests/utils.py 100.00% <100.00%> (ø)
jupytext/cli.py 95.44% <0.00%> (-0.20%) ⬇️
tests/test_cli.py 100.00% <0.00%> (ø)
jupytext/languages.py 100.00% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a393afd...43624c8. Read the comment docs.

@@ -39,7 +39,7 @@ install:
pip install -r requirements.txt;
fi
# install is required for testing the pre-commit mode
- pip install . || true
- pip install .[myst] || true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the install to an extra, if thats ok

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 16, 2020

@choldgraf you'll like this one; in d627398 I have introduced logic to compact the metadata blocks.

For blocks with no nested dicts, the block is denoted by starting colons::

```{nb-code}
:other: true
:tags: [hide-output, show-input]

content
```

For blocks with nesting the block is enlosed by ---::

```{nb-code}
---
other:
  more: true
tags: [hide-output, show-input]
---

content
```

(these two representations are synonymous in the MyST spec: https://myst-parser.readthedocs.io/en/latest/using/syntax.html#parameterizing-directives)

@mwouts
Copy link
Owner

mwouts commented Mar 16, 2020

Hello @chrisjsewell , that's awesome!

Sorry to have caused you trouble with Python 2.7, I prefer to keep testing it, but I agree, it's hard to make the CI work with conditional dependencies. Adding the extra dependencies in the CI script itself is perfectly OK. Anyway, maybe I should rewrite the part of travis.yaml that tests conda environment to match the recommendations from CONTRIBUTING.md...

Tonight I'll take the time to review this with more detail. We could ship that soon, at least if it's OK on your side. Maybe we could

  • confirm the mystnb extension
  • add a word/link on the myst format in the documentation (in formats.md)
  • extend the Notebook and Lab extension with the myst format

Do you see anything else that you'd like to add?

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 16, 2020

to match the recommendations from CONTRIBUTING.md.

Oh yeh, should have probably read that lol!

confirm the mystnb extension

I've added .mnb now as well as .mystnb. I think I'm happy with these, but @choldgraf, @mmcky can interject if they wish.

  • add a word/link on the myst format in the documentation (in formats.md)

  • extend the Notebook and Lab extension with the myst format

    Do you see anything else that you'd like to add?

No, I think that should just about cover it 👍

A markdown cell
And below, the cell for function f has non trivial cell metadata. And the next cell as well.

```{nb-code} ipython3
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this planned to be {execute}?

@choldgraf
Copy link
Contributor

choldgraf commented Mar 16, 2020

This looks great to me - with one main question, which is that I see a few discrepancies between the .myst format and the format we've laid out in:

executablebooks/MyST-NB#12 (comment)

In particular, using {nb-code} instead of {execute}, same for {nb-raw} etc (where I assumed we would just use {raw}.

Is there a reason for the change?

re: mnb or mystnb, I think those are OK with me. Another option just be to use .md like many of the other parsers do, since that's such an overloaded extension anyway...

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 16, 2020

In particular, using {nb-code} instead of {execute}, same for {nb-raw} etc (where I assumed we would just use {raw}.

This is the 'tricky' part; currently {execute} is a synonym for {jupyter-execute} and implies that the directive will be able to be run by the jupyter_sphinx.JupyterCell directive, if you just have the myst-parser and jupyter-sphinx extensions in your sphinx conf.py.
The following will not work in this way, because the metadata/options are invalid for JupyterCell.

```{execute}
:other: true
:tags: [hide-output, show-input]

content
```

If you wanted this to be directly used by JupyterCell you would have to introduce a specific conversion of the metadata to comply with the options dictated by that directive. This would negate true round-trip conversion, because you would have to 'throw away' any metadata that JupyterCell doesn't have an option for.

Another option would be to just use .md like many of the other parsers do, since that's such an overloaded extension anyway...

This then follows on from the above; with this ".mystnb" file format, we are saying that it will not be treated the same as simple .md in the sphinx conversion; it will be specifically converted to a notebook first (either using a pre-conversion step in NotebookParser or a separate sub-class of it as I outlined here). So in your sphinx project you may have three distinct types of file:

  • .md which are read directly by the myst-parser package's MystParser. These could contain jupyter-execute directives, but would not be involved in any jupyter-cache like execution caching, invoked by myst-nb.
  • .ipynb which are read by the myst-nb package's NotebookParser
  • .mystnb which are read by NotebookParser or a sub-class like MystNbParser.

In this context, .ipynb and .mystnb are interchangeble.

@chrisjsewell
Copy link
Contributor Author

In 4c206ec I have added MyST-NB to the Notebook extension. Basically, anywhere there was an Rmd I also added an mnd. From manual testing this looks to be working great, but obviously @mwouts may want to check over this

@chrisjsewell
Copy link
Contributor Author

FYI I had a few difficulties following the jupytext/nbextension/README.md, for which this issue helped widgetti/ipyvolume#85

@chrisjsewell
Copy link
Contributor Author

I added MyST to the jupyterlab extension, but I'm having trouble installing it to test. So I've put the commit it in a separate branch for now a6a95e8 might need you to add that for me @mwouts

FYI I note that in the package.json, the jupyterlab dependancy is set to version 1, but now version 2 is released. I'm guessing that's on the TODO list to update

@choldgraf
Copy link
Contributor

That all sounds good to me @chrisjsewell - @mwouts if we end up changing the names of some things then we are happy to upstream a PR to keep jupytext up-to-date with things.

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 16, 2020

Added mystnb to benchmarking (4c206ec) and no problems there (faster than md 😉 )

image

image

@mwouts mwouts marked this pull request as ready for review March 16, 2020 22:33
@mwouts
Copy link
Owner

mwouts commented Mar 16, 2020

Impressive work @chrisjsewell !

I'll merge this now, and then maybe we can do a bit more testing, and ship this in a few days...

A few comments:

  • This PR seems to be based on version 1.3.4, now we are at 1.4.0+dev, but I expect no issue with the merge.
  • I can take care of adding the mnb extension to the JupyterLab extension, sure no pb
  • Maybe I'll regenerate the World population.mystnb example as a paired notebook (e.g. jupytext --set-formats ipynb,.pct.py:percent,.lgt.py:light,.spx.py:sphinx,md,Rmd,.pandoc.md:pandoc,mnb,mystnb '.\World population.ipynb') - I'll do that after the merge
  • We will also have to add a mention of the mnb and mystnb extensions in the documentation

@mwouts mwouts merged commit 75449a5 into mwouts:master Mar 16, 2020
@chrisjsewell
Copy link
Contributor Author

We will also have to add a mention of the mnb and mystnb extensions in the documentation

In a few more minutes I would of committed that 😆

This PR seems to be based on version 1.3.4, now we are at 1.4.0+dev, but I expect no issue with the merge

Ah oops, that must have been down to way I fork it off of the ExecutableBookProject:master

I can take care of adding the mnb extension to the JupyterLab extension, sure no pb
Maybe I'll regenerate the World population.mystnb example as a paired notebook (e.g. jupytext --set-formats ipynb,.pct.py:percent,.lgt.py:light,.spx.py:sphinx,md,Rmd,.pandoc.md:pandoc,mnb,mystnb '.\World population.ipynb') - I'll do that after the merge

Cheers 😄

@mwouts
Copy link
Owner

mwouts commented Mar 16, 2020

I added MyST to the jupyterlab extension, but I'm having trouble installing it to test. So I've put the commit it in a separate branch for now a6a95e8 might need you to add that for me @mwouts

FYI I note that in the package.json, the jupyterlab dependancy is set to version 1, but now version 2 is released. I'm guessing that's on the TODO list to update

@chrisjsewell , if you like you're actually very close to having a PR for this... your change is correct, but should be rebased to match the latest version of the Lab extension (and version number could thus be 1.2.1). Just one detail - could you also update packages/labextension/CHANGELOG.md before compiling the .tar.gz package?

And if you want to try the lab extension locally, I recommend building (python setup.py sdist bdist_wheel) and then pip install the jupytext-1.4.0+dev.tar.gz package, cf. the extension README.md.

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.

A book publishing project interested in extending jupytext code
3 participants