Skip to content

Conversation

cooperrc
Copy link
Member

@melissawm, I have added a short description in the README using Jupytext to convert or pair notebooks to the desired MyST-NB format.

This PR also includes 2 more gifs for pairing in the classic notebook and jupyterlab.

We should also update the "file upload" gif to a .md upload.

Copy link
Member

@melissawm melissawm left a comment

Choose a reason for hiding this comment

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

Thank you for this, @cooperrc ! I left some suggestions about MyST mainly noting that MyST markdown is the format and MyST-NB is a tool. I wonder if we should add a note about not using MyST directives in the notebook so we avoid the trouble that @a-elhag went through with his first tutorial - can you help here, Al?

@melissawm corrected my mistake: MyST-NB is a program while MyST Markdown is the file. 

also caught a few typos.

Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
@cooperrc
Copy link
Member Author

Thank you for this, @cooperrc ! I left some suggestions about MyST mainly noting that MyST markdown is the format and MyST-NB is a tool. I wonder if we should add a note about not using MyST directives in the notebook so we avoid the trouble that @a-elhag went through with his first tutorial - can you help here, Al?

It might help to update the "Learn to write a NumPy tutorial" with the best practices for pairing, common mark, and other general advice (like using docs/stable links). I think if we keep the README limited to just a few how-to steps it will be easier to reference for a new contributor.

@melissawm and @a-elhag, would updating Learn to write a NumPy tutorial fit within this PR? or should we keep all of this advice in the README?

@drskoolie
Copy link

drskoolie commented Dec 1, 2020

My first time writing a tutorial I thought that since you can go from Jupyter Notebook -----> Myst Markdown flawlessly, then it should logically work in the reverse direction too Myst Markdown -----> Jupyter Notebook. To add on to what @melissawm and @rossbar said, for the front page I think it would be best to not go into too much detail about the flavour of markdown, at least for the main README.md.

@cooperrc What do you think of making the front page one just for .ipynb to .md and/maybe as little details as possible about .md to ipynb.

And an additional How to write NumPy tutorials in the Command Line document linked where you can go into more details about the particulars of setting up Myst Markdown?

@melissawm
Copy link
Member

I like the approach of possibly leaving a more advanced workflow for users who wish to deal with that. This could be a separate document or live at the end of the readme (though personally I would prefer a shorter readme)

@rossbar
Copy link
Collaborator

rossbar commented Dec 1, 2020

I like the approach of possibly leaving a more advanced workflow for users who wish to deal with that. This could be a separate document or live at the end of the readme (though personally I would prefer a shorter readme)

I agree - I don't think we should be making any recommendations about tooling or workflow. There's a ton of flexibility (sphinx-based, nbconvert-based, jupytext-based, vscode plugins, etc.) and I think users need to find what's right for them.

@cooperrc
Copy link
Member Author

cooperrc commented Dec 1, 2020

I'll simplify the steps and remove the MyST-NB/MyST markdown/etc. discussions. I agree with the consensus here: keep it simple.

To @rossbar's point, there are a lot of different workflows that people can use, so its better to leave it up to the user.

One lingering question though: What is the final format we want to review?
options:

  • markdown: this is the default "Download as markdown" option in a Jupyter interface it does not execute when building the static site
  • MyST markdown: this is the format that myst-nb (and also jupyter-book) uses to build the static page

If we just want users to submit markdown, then I don't think we need much of a discussion. If we want users to submit MyST markdown, it helps to at least give a potential workflow (and mention that there are plenty of other methods).

Jupytext plugin recognizes both headers and will allow you to open both .md files "as notebooks".

@rossbar
Copy link
Collaborator

rossbar commented Dec 1, 2020

markdown: this is the default "Download as markdown" option in a Jupyter interface it does not execute when building the static site

Could you clarify what you mean here? We currently have the myst-nb extension configured to grab any file with the .md extension for conversion to html during the build process. Whether or not anything is executed is a question of whether there are {code-cell} directives present in the .md document. In other words, any .md file in the content/ folder is included in the sphinx build process.

@cooperrc
Copy link
Member Author

cooperrc commented Dec 1, 2020

markdown: this is the default "Download as markdown" option in a Jupyter interface it does not execute when building the static site

Could you clarify what you mean here? We currently have the myst-nb extension configured to grab any file with the .md extension for conversion to html during the build process. Whether or not anything is executed is a question of whether there are {code-cell} directives present in the .md document. In other words, any .md file in the content/ folder is included in the sphinx build process.

Yes, that was not clear.

In the markdown format, code cells are formatted as

```python
import numpy as np
```

These cells do not get executed by myst-nb.

The myst markdown uses

```{code-cell} ipython3
import numpy as np
```

@rossbar
Copy link
Collaborator

rossbar commented Dec 2, 2020

Gotcha, thanks for the clarification. For jupytext users, code cells in .ipynb format should always map to {code-cell} directive blocks automatically (instances of this failing should probably be raised in an issue to jupytext). For users writing in markdown directly, it relies on them understanding that the {code-cell} is not equivalent to the normal "fenced" code block, which does syntax highlighting but is not executed.

I would expect this not to be too much of a problem as jupytext handles it for authors who submit .ipynb files, and presumably those submitting markdown files directly are familiar with MyST and aware of the distinction.

@melissawm
Copy link
Member

Ok. How about this:

  • We will review code in myst markdown, with the {code-cell} directives;
  • If the user wants to, they can install jupytext and "download to myst markdown" to submit their PR.
  • If the user does not want to install jupytext, they can upload the ipynb and we'll do the conversion ourselves before starting the review.

The readme can mention uploading ipynb files and include a short optional step ("If you have jupytext installed, you can download your notebook as myst markdown and submit that in your PR").

The only problem with that is that notebooks and .md files live in separate folders so this might lead to user error, but I don't think those would be too much of a problem (I'm not expecting to see hundreds of PR's to this repo :))

@rossbar
Copy link
Collaborator

rossbar commented Dec 2, 2020

Sounds good to me! - I had always envisioned reviewers assuming the burden of converting to myst markdown since it's unlikely that authors would be familiar with it and it should be super easy for authors to submit .ipynb. If we do start hitting reviewer bandwidth limits, then I'd vote for adding a CI workflow to auto-convert with jupytext!

@cooperrc
Copy link
Member Author

cooperrc commented Dec 2, 2020

Sounds good to me! - I had always envisioned reviewers assuming the burden of converting to myst markdown since it's unlikely that authors would be familiar with it and it should be super easy for authors to submit .ipynb. If we do start hitting reviewer bandwidth limits, then I'd vote for adding a CI workflow to auto-convert with jupytext!

If you fork the repo, would the CI convert for you? The reason I ask is that the CI could do a

jupytext --set-formats ipynb,myst notebook.ipynb  # Turn notebook.ipynb into a paired ipynb/myst notebook
jupytext --sync notebook.ipynb                  # Update whichever of notebook.ipynb/notebook.md is outdated

That way the user/reviewer can update in either format. That's why I had added the discussion of pairing in the README.

@rossbar
Copy link
Collaborator

rossbar commented Dec 2, 2020

Ah, ok - I think I'm finally grasping the context I was missing!

The workflow I've been advocating for assumes that the .ipynb -> myst conversion happens only once (if necessary) on PR submission. I had assumed that all subsequent review/authoring would be done in md format. This was one of the main motivations for using the md-based notebook format rather than .ipynb (so that the diffs would be sensible, and things like the github suggestion feature work work). So to summarize, this workflow looks something like:

  1. User submits tutorial.ipynb
  2. Reviewer runs jupytext --from notebook --to myst tutorial.ipynb and pushes to the author's branch.
  3. Review now commences in the "standard" way via github - diffs/suggestions highlight changes and circleCI builds each push, executing the notebooks from the top and showing the rendered output in the build_artifact

This workflow makes an assumption that the original authors will be comfortable making edits in the md-based notebook format rather than .ipynb files in the browser. Maybe that's too strong an assumption, but the upside is that it avoids having to deal with bi-directional .ipynb <-> .md conversion at all (leaving users to setup a local workflow that works for them), and ensures that .ipynb files aren't involved in the review process.

@cooperrc
Copy link
Member Author

cooperrc commented Dec 2, 2020

Ah, ok - I think I'm finally grasping the context I was missing!

:) Probably on me for not being clear.

The workflow I've been advocating for assumes that the .ipynb -> myst conversion happens only once (if at all) on PR submission.

  1. User submits tutorial.ipynb
  2. Reviewer runs jupytext --from notebook --to myst tutorial.ipynb and pushes to the author's branch.
  3. Review now commences in the "standard" way via github - diffs/suggestions highlight changes and circleCI builds each push, executing the notebooks from the top and showing the rendered output in the build_artifact

This workflow makes sense. The only part is what happens to the tutorial.ipynb? We can keep it updated after further commits, but there's not much reason from Jupytext FAQ:

Which files should I version control?
Unless you want to version the outputs, you should version only the text representation. The paired .ipynb file can safely be deleted. It will be recreated locally the next time you > open the notebook (from the text file) and save it.

Note that if you version both the .md and .ipynb files, you can configure git diff to ignore the diffs on the .ipynb files.

We don't need the .ipynb. Is there any risk of confusion if we always review-store-update MyST files and delete .ipynb? It just reduces the number of files that need to be organized and kept up to date.

@rossbar
Copy link
Collaborator

rossbar commented Dec 2, 2020

We don't need the .ipynb. Is there any risk of confusion if we always review-store-update MyST files and delete .ipynb? It just reduces the number of files that need to be organized and kept up to date.

Yup, the original .ipynb file is deleted. So to expand on my bulleted list above, step 1 would actually look like:

jupytext --from notebook --to myst tutorial.ipynb
rm tutorial.ipynb

Thus (as soon as we're done with the PRs converting existing material to this workflow, see #44, #47, #49) there will be no .ipynb files stored in the content/ directory, only .md

@cooperrc
Copy link
Member Author

cooperrc commented Dec 2, 2020

Thus (as soon as we're done with the PRs converting existing material to this workflow, see #44, #47, #49) there will be no .ipynb files stored in the content/ directory, only .md

Nice, can we remove any failed checks due to outputs still being included? Is it possible to ignore the .ipynb's during circleCI? This would make the submission a little more user-friendly.

@cooperrc
Copy link
Member Author

cooperrc commented Dec 7, 2020

the last commit moved most of these changes to a new tutorial: content/pairing.md

I link to the tutorial in the README so a user can find the information if they are so inclined.

Now, the README has two messages: we review in .md and we accept .ipynb and myst .md.

Copy link
Member

@melissawm melissawm left a comment

Choose a reason for hiding this comment

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

Hi @cooperrc - thanks for the patience! Here are my comments, all minor - thanks for doing this, I think this makes it all much clearer, even for me!

Comment on lines +246 to +247
tutorials. Now you can work in either a simple text editor like VIM
or emacs or continue building notebooks in your browser. Jupytext can
Copy link
Member

Choose a reason for hiding this comment

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

Very nit: I think some people may object to your suggestion of vim or emacs as simple text editors :)

Copy link
Member Author

Choose a reason for hiding this comment

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

haha!
simple text editor as opposed to a simple text editor.

This was a conversation I first had in grad school: vim/emacs are editors for simple text i.e. ASCII characters. I would agree, they are not simple.

Maybe cut out the simple?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is the last change before we merge this; I'd suggest something like plain text editor.

Great catches from Melissa. I think its much clearer. Last _nit:_ is the reference to vim/emacs as "simple text editors" should we change to vim/emacs as text editors?

Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
Base automatically changed from master to main March 6, 2021 11:39
@melissawm
Copy link
Member

Let's merge this and if we think we need to do more, this can be done in a follow-up. Thanks again, @cooperrc !

@melissawm melissawm merged commit 4387ce8 into numpy:main Mar 15, 2021
@cooperrc
Copy link
Member Author

Let's merge this and if we think we need to do more, this can be done in a follow-up. Thanks again, @cooperrc !

Thanks @melissawm! This semester has not had much free time for my FOSS work. Looking forward to getting back to it come May timeframe.

This merge will definitely help users, we can fine tune it later.

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.

4 participants