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 documentation of myst-nb format #458

Merged
merged 22 commits into from
Mar 18, 2020
Merged

Conversation

chrisjsewell
Copy link
Contributor

@chrisjsewell chrisjsewell commented Mar 17, 2020

Also added MyST jupyter labextension

cc @choldgraf @jstac @mmcky

@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #458 into master will decrease coverage by 0.02%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
- Coverage   98.91%   98.88%   -0.03%     
==========================================
  Files          79       79              
  Lines        7762     7823      +61     
==========================================
+ Hits         7678     7736      +58     
- Misses         84       87       +3     
Impacted Files Coverage Δ
jupytext/formats.py 98.38% <60.00%> (-0.31%) ⬇️
jupytext/myst.py 98.33% <96.55%> (-0.99%) ⬇️
jupytext/jupytext.py 98.67% <100.00%> (ø)
tests/test_ipynb_to_myst.py 100.00% <100.00%> (ø)
tests/test_mirror.py 100.00% <100.00%> (ø)

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 75449a5...c7384a8. Read the comment docs.

@mwouts mwouts marked this pull request as ready for review March 17, 2020 06:56
@mwouts
Copy link
Owner

mwouts commented Mar 17, 2020

Hello @chrisjsewell , thanks for this PR, which looks good to me!

I will let @choldgraf have a look at the proposed documentation, and confirm the latest changes, namely:

  • The default extension is now .mnb
  • Code cells are encoded with code-cell rather than nb-cell, etc

Also, in the previous PR (#456 ) I think that @choldgraf suggested allowing the .md extension for the MyST format. This should be possible. I think you'd have to

  • add '.md' (and maybe .markdown?) to the list of MyST extensions
  • patch formats.guess_format so that it can differenciate MyST-md files from Jupytext md files. Or maybe you could directly adapt read_metadata to the MyST-way of representing the notebook metadata - I mean, the jupytext section is at the top level in that format, and that's where you could be looking for the jupytext.text_representation.format_name metadata...

If you do that, then there will be the question of which extension should be the default: .mnb or .md? I think I'd recommend .md, but obviously that's up to you! (Note that it would be a bit harder to adapt the notebook/lab extensions, still you could seek inspiration from the light vs percent formats for scripts, which both share the .py extension).

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 17, 2020

Thanks @mwouts yes in 316bc2e, I tried adding .md to the extensions, but you'll see that it failed a number of the tests. Perhaps patching formats.guess_format will fix this.

I think I'd recommend .md, but obviously that's up to you!

IMO ideally it would be something like .myst.md. But I'm not sure how possible it is to have such a "double-barrel" extension in jupytext, without changing too much code.

Note that it would be a bit harder to adapt the notebook/lab extensions

Indeed. I did have a quick try at this but couldn't get it to working straight away. I might need some help in that respect.

@chrisjsewell
Copy link
Contributor Author

Note-to-self: /usr/local/share/jupyter/nbextensions/jupytext/index.js

@mwouts
Copy link
Owner

mwouts commented Mar 17, 2020

Thanks @mwouts yes in 316bc2e, I tried adding .md to the extensions, but you'll see that it failed a number of the tests. Perhaps patching formats.guess_format will fix this.

Yes, I think so.

I think I'd recommend .md, but obviously that's up to you!

IMO ideally it would be something like .myst.md. But I'm not sure how possible it is to have such a "double-barrel" extension in jupytext, without changing too much code.

With the current code Jupytext will see this as a file with .md. I think indeed it would be too much work to change this. Still: 1) if you fix guess_format, then this .md file will be recognized as a MyST file, and 2) you can pair a notebook to double-barrel extensions, cf. the World Population example in which we pair the .ipynb file to a .pandoc.md file, using the format description .pandoc.md:pandoc.

Note that it would be a bit harder to adapt the notebook/lab extensions

Indeed. I did have a quick try at this but couldn't get it to working straight away. I might need some help in that respect.

Sure! First, let's see what @choldgraf thinks about having .md (or .myst.md) as the default extension, and I'll show you how to use the explicit format name (.md:mystnb) to achieve this.

Note-to-self: /usr/local/share/jupyter/nbextensions/jupytext/index.js

Yes, I saw you had issues with the README, didn't the symlink install work? I'd be happy to know how to fix the README...

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 17, 2020

Yeh I don't know if its as easy as just changing guess_format. For example, in
tests/test_active_cells.py

---
jupyter:
  jupytext:
    main_language: python
---
```python active="ipynb,py,R,Rmd"
# This cell is active in all extensions
```

There's no real way to differentiate between MyST-md and Jupytext md?
So all these are failing

The only real way to discriminate, is to parse the whole file (with myst) and check if any code cells have been discovered, which is far from ideal.
The quick and dirty way is using the same approach as for pandoc (that looks for lines starting with :::), by looking for any lines starting with ```{code-cell}, I would not that this means that a myst notebook would now require a code cell to be recognised, and there's a bunch of edge cases where this fails (for pandoc as well). But yeh, that's this is the only way. (This is why I think it would be much better to use .myst.md)

@chrisjsewell
Copy link
Contributor Author

Ok I've added .md to the myst formats. @choldgraf can work out how to make that the default for nbextension and labextension

Yes, I saw you had issues with the README, didn't the symlink install work? I'd be happy to know how to fix the README...

Nope, jupyter nbextension enable jupytext --user fails, so I just used jupyter nbextension install index.js --py --user (or something like that) and edited the index.js at the path that I pasted. No idea why I'm afraid, and its why I'm now leaving it to @choldgraf, because its just way too much hassle trying to do interactive development with these extensions 😒

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 17, 2020

@mwouts if/when you are happy with this PR (do I need to work out tests to add, to cover the -0.05% decrease!?) I would suggest merging, then @choldgraf can work on the notebook/lab extensions separately

@choldgraf
Copy link
Contributor

choldgraf commented Mar 17, 2020

I'd be OK with having .mnb as the default extension. After a few weeks/months then we can revisit the question before too many users have solidified mnb in their heads. 👍

(just a note, I probably will not be working on the lab/notebook extensions, I don't know anything about developing for lab/notebook...but we don't need that to block this PR)

@mwouts
Copy link
Owner

mwouts commented Mar 17, 2020

Hello @chrisjsewell , @choldgraf , well this is going fast! 🚀

do I need to work out tests to add, to cover the -0.05% decrease!?

Oh no, surely not!! Actually a few months I tried to fix that with .codecov.yml, but it looks like it didn't work... 😄

I would suggest merging, then @choldgraf can work on the notebook/lab extensions separately

Well, as I user I think I'd prefer to use .md. Maybe it's not that hard to adapt the extensions, let me give it a try...

@mwouts
Copy link
Owner

mwouts commented Mar 17, 2020

@chrisjsewell , what do you think of this: 6bec0bc ? This gives the option to pair the document to a md:myst file, and it seems to work as expected. Would you like to give it a try ? Maybe we can also add this to the Lab extension ?

@chrisjsewell
Copy link
Contributor Author

chrisjsewell commented Mar 17, 2020

@chrisjsewell , what do you think of this: 6bec0bc ? This gives the option to pair the document to a md:myst file, and it seems to work as expected. Would you like to give it a try ? Maybe we can also add this to the Lab extension ?

Yeh cheers, if you say it works, then that's good enough for me 😄
A minor thing is that I think we can now maybe change the name from MyST-NB to MyST Markdown
(I changed it to this in formats.md),
and yes if you could also add it to the Lab extension, that would be great 👍

@chrisjsewell
Copy link
Contributor Author

FYI I just added the nbextension, copied from your branch. Its working great 👍

@chrisjsewell
Copy link
Contributor Author

One minor improvement that comes to mind, is to set the common sphinx metadata key orphan, and maybe tocdepth (see sphinx field-lists) to sync by default, i.e. initially setting notebook_metadata_filter: orphan.
Is this possible/advisable?

@chrisjsewell
Copy link
Contributor Author

In 9735b58 I have added the option to store the original source line numbers in the cell metadata,
when converting from a text document to a notebook.
This is not directly applicable to jupytext, but is part of the fix for executablebooks/MyST-NB#83, whereby we would like to report the original source line numbers, e.g. for missing references or other sphinx errors/warnings.

@mwouts
Copy link
Owner

mwouts commented Mar 18, 2020

Hello @chrisjsewell , great job! I see you've changed the pairing to md:myst in the lab extension as well, so we're good to go! I should be able to find some time later on tonight to release this.

One minor improvement that comes to mind, is to set the common sphinx metadata key orphan, and maybe tocdepth (see sphinx field-lists) to sync by default, i.e. initially setting notebook_metadata_filter: orphan. Is this possible/advisable?

Yes, this is possible. I think it should have very little impact, because the filter ignores metadata when it is missing. All you need to do is to modify metadata_filter._DEFAULT_NOTEBOOK_METADATA below. As long as it has little impact on the tests (i.e. the mirror files), I am ok with any change there.

_DEFAULT_NOTEBOOK_METADATA = ','.join([
# Preserve Jupytext section
'jupytext',
# Preserve kernel specs
'kernelspec',
# Kernel_info found in Nteract notebooks
'kernel_info'])

@mwouts
Copy link
Owner

mwouts commented Mar 18, 2020

Thanks @chrisjsewell , I'll merge this, test and release a new version of Jupytext. Thank you for this great contribution !

Just to mention:

@mwouts mwouts merged commit 8ac346d into mwouts:master Mar 18, 2020
@chrisjsewell
Copy link
Contributor Author

Thanks @mwouts!

(Yeh I'll look into the notebook_metadata_filter for orphan at a later date.)

@choldgraf
Copy link
Contributor

BOOM - way to go everybody 🎉 thanks @chrisjsewell for the hard work on this, and thanks @mwouts for the quick responses and turnaround :-)

@mwouts
Copy link
Owner

mwouts commented Mar 18, 2020

(Yeh I'll look into the notebook_metadata_filter for orphan at a later date.)

If you like I can take it (and tocdepth) - there's no visible impact here.

@chrisjsewell
Copy link
Contributor Author

Dont merge

(Yeh I'll look into the notebook_metadata_filter for orphan at a later date.)

If you like I can take it (and tocdepth) - there's no visible impact here.

Yep that would be great thanks 👍

@mwouts
Copy link
Owner

mwouts commented Mar 18, 2020

Yep that would be great thanks 👍

Done! And jupytext-1.4.1 is now available on pip (soon on conda-forge). Let me know how it works for you... Thanks!

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.

3 participants