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

Tutorials infrastructure: switch to sphinx+text-based notebook workflow #36

Merged
merged 11 commits into from
Nov 9, 2020

Conversation

rossbar
Copy link
Collaborator

@rossbar rossbar commented Oct 27, 2020

Overview

Changes the workflow for the numpy-tutorials repo to use a text-based format for Jupyter notebooks and sphinx+myst for running & testing the notebooks, and generating a static tutorials site.

The text-based notebook format expresses notebooks in markdown rather than the typical json format associated with .ipynb files. This enables a workflow for submission and review that maps onto the standard GitHub reviewing workflow.

Authors can still submit content in .ipynb format - it will be converted to .md for review using jupytext. Both html and downloadable notebooks (.ipynb) are generated as part of the build process. CircleCI has been configured to handle this on GitHub. A standard sphinx workflow can be used to run/evaluate changes locally (though users are not required to use the sphinx for authoring/reviewing!)

The sphinx-book-theme is based on the pydata-sphinx-theme used in the main NumPy docs, and provides convenient buttons both for launching notebooks on cloud services (e.g. binder & jupyterhub) as well as downloading notebooks to run locally.

Todo

Post merge

 * Add skeleton page from sphinx-quickstart.

 * Convert source files from ipynb to md:
   jupytext -k python3 --from notebook --to myst *.ipynb

 * Add md files to sphinx site.

 * Change index to mystmd and add binder link.
Provides more options for buttons in theme. Enable download and github
interaction buttons.

 * Add launch button w/ prelim configuration.
   Configuration will likely need to be adjusted after merge.

 * Add logos, titles, favicon to theme.
Build sphinx site.
 * Remove cloud-service-based workflow (e.g. reviewnb)
 * Add circleCI artifact redirect badge
Build directly from markdown and take advantage of theme
options for enabling downloaded nb's
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Landing page should now look like the README.

Also moved gifs to make them accessible both in the repo and
the site.
 * Handle formatting complaint in style guide.
 * Turn off execution of cell that raises exception.
@melissawm
Copy link
Member

Thanks, @rossbar ! I'll give it a try locally and see how that goes.

Could the static site be served through github pages?

I can do the README update with new gifs.

@rossbar
Copy link
Collaborator Author

rossbar commented Oct 28, 2020

Could the static site be served through github pages?

That would be my preference!

I can do the README update with new gifs.

Great :) I didn't intend for the TODO's to be blockers or anything, just a running list of ideas that I had while putting this together, so no pressure!

@melissawm
Copy link
Member

I tried this locally and this looks really good. Now for the plan:

  • I think hosting on github pages is probably the way to go, although I don't have a lot of experience there. Any advice is appreciated.
  • I'll update the README in a follow-up, so the workflow is decided and I can redo the gifs and update instructions.
  • This - Incorporate 🐛 FIX: Fix linking to autogenerated ipynb files from md. executablebooks/sphinx-book-theme#241 (or related) to get downloads working for both .ipynb and .md formats - would be very nice, but not a blocker. If people really want the ipynb files before that is done we can always include a link to the raw github files in the .md. Let's wait and see if that's merged before we get our own things sorted.
  • I'll trust you with the CI, but the workflow in Demo workflow with ma-tutorial rossbar/numpy-tutorials#2 seems pretty reasonable.

Thank you so much, @rossbar ! I think this is an improvement over ReviewNB which, unfortunately, doesn't look ready to use.

@rossbar
Copy link
Collaborator Author

rossbar commented Nov 2, 2020

I think hosting on github pages is probably the way to go, although I don't have a lot of experience there. Any advice is appreciated.

I agree - I'll set up the CI/CD to deploy to github pages on merge (similar to numpy devdocs). Down the road, I think we can still use github pages to host but redirect somewhere else (e.g. somewhere on numpy.org) pretty easily if that's what the community decides to do.

I'll trust you with the CI, but the workflow in rossbar#2 seems pretty reasonable.

I expect there will be plenty to tweak as we try this out. Fortunately the set of tools is pretty flexible so I think we can accommodate a lot of different approaches.

I'll update the CI config this week for deployment. LMK if anything else crops up that needs attention!

@mattip
Copy link
Member

mattip commented Nov 2, 2020

The numpy devdocs is a bit strange in that there is a dedicated repo just for the rendered docs. Another option, and one that projects seem to be adpoting more, is to have a dummy branch inside the repo, like site or gh-pages. Then you do something like this on merges

push doc
make html
popd
git checkout gh-pages
rm -rf docs/*
cp -r doc/build/html/* docs
git add docs       # both adds and removes content
git commit -m"automatic build"
git push

For a sample github-action based workflow that pushes built documents to a branch in the current repo see here

This example has a few other things on the site branch:

  • there is a directory for each version of the repo under docs. You probably don't need that here
  • since there directories are versioned, there is a stable symlink to the last release, and an index.html page that redirects to stable. Again, you probably don't need that here.

We did have a bit of a problem getting it going, since the PR tries to do git push, and the PR was from a fork of the repo, so during the PR CI runs it kept failing. Once we did the PR from a branch in the repo itself, rather than from a fork, it all JustWorks.

If you are concerned about the size of the repo, you can do destructive commits to site instead of additive ones using force-push. It will mess up people who check out the site branch, but 🤷

@mattip
Copy link
Member

mattip commented Nov 2, 2020

Maybe split the PRs: only build in this PR and then in another one deploy?

@melissawm
Copy link
Member

I agree with @mattip . As far as I'm concerned, this is good for merging. But it would be useful to have the opinion of others more experienced in CI and this kind of workflow.

@mattip
Copy link
Member

mattip commented Nov 4, 2020

Someone with admin permissions and a circleci account needs to add the needed integrations to get this to work

@charris
Copy link
Member

charris commented Nov 4, 2020

@mattip What needs to be done?

@rossbar
Copy link
Collaborator Author

rossbar commented Nov 4, 2020

I think I have the necessary permissions for the CircleCI. I also have to disable the CircleCI on my own fork for it to take effect. I'm happy to do so after merging.

@melissawm
Copy link
Member

melissawm commented Nov 9, 2020

Hello folks! Finally merging this - thanks @rossbar and all who contributed to the discussions. This may mean that our README is outdated for a while, so I'll add a note there explaining we're still figuring this out.

Edit: I'll also organize the remaining todo items into issues so we can keep track of what's left to do.

@melissawm melissawm merged commit 97df601 into numpy:master Nov 9, 2020
melissawm added a commit to melissawm/numpy-tutorials that referenced this pull request Nov 9, 2020
melissawm added a commit to melissawm/numpy-tutorials that referenced this pull request Nov 9, 2020
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