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

Use nbsphinx to build examples for documentation #1349

Merged
merged 6 commits into from
May 9, 2018
Merged

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Aug 16, 2017

Motivation and context:
As noted in #1308 it is not possible to build the examples for inclusion in the documentation with current versions of Jupyter. While the solution so far was to install older versions, this should eventually be fixed and as I have the same issue with the documention of Nengo SPA, I decided to spend some time on this.

@tbekolay pointed me towards nbsphinx which allows to include Jupyter notebooks into Sphinx documentation with good integration with the Sphinx theme. However, Sphinx does not allow to have source files outside of the documentation folder (for security reasons I believe), but our examples folder lives outside the documentation folder. I am opposed to changing that, and I think @tbekolay agrees. So other potential solutions are:

  • The PR Add notebook directive spatialaudio/nbsphinx#33 which adds a RST directive that allows to include notebooks from arbitrary paths. Unfortunately it doesn't seem like this PR will get merged any time soon (if ever).
  • A symlink, but this does not work on Windows (before 10 without elevated privileges). Also, this leads to problems with images (Symbolic link causes problems with images in output cells spatialaudio/nbsphinx#49).
  • Copy all the notebooks which can be done automatically by sphinx through conf.py. This is the solution I chose in this branch because it was pretty much the path of least resistance. It seems like sphinx manages, despite the copied notebooks being overwritten in each run, to just re-excute notebooks that have been changed.

Another issue (which might also exist in the current system, but I haven't verifiied it): The Jupyter notebooks are executed by a Jupyter kernel that is independent from whatever virtualenv is active. It seems like a bad idea to just use the default kernel that might have the wrong version of Nengo installed (in particular true for my environment). Thus I added some code to conf.py to ask for which kernel to use (and the possibility to fix it via an environment variable).

This PR is still work-in-progress (see to do list below), but at this stage it would be good to get an idea if the general direction seems good.

Interactions with other PRs:

ctn-waterloo/devscripts#6 changes the clearoutput script to add metadata to the notebooks that is required to make syntax highlighting with nbsphinx work. I ran the script on all existing examples to add that metadata.

How has this been tested?

build the docs and looked at through them

How long should this take to review?

  • Average (neither quick nor lengthy)

Types of changes:

  • Non-code change (touches things like tests, documentation, build scripts)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • [n/a] I have added tests to cover my changes.
  • All new and existing tests passed.

Still to do:

  • There are some warnings about non-included files (examples/REAME.rst, examples/usage/exceptions.ipynb, examples/usage/strings.ipynb). Figure out whether these should indeed be skipped in the documentation (and silence these warnings) or include them in documenation in an appropriate way.
  • In some notebooks input part of code cells has been collapsed. This does not work in nbsphinx. nbsphinx does support hiding complete cells, i.e. input and output, but in most cases we want to preserve the output. While a commonly requested feature, it doesn't seem likely that separate hiding of input and output will get implemented any time soon. Not sure what exactly to do about this or whether this rules out nbpshinx completely. It might also be possible to adapt our hide_input function to work in Jupyter notebook and the Sphinx output (at least the Javascript gets included).
  • The progress bar widget doesn't work and instead a warning will be included. It seems that this is because we are not properly installing the widget Javascript in the recommended way. This is something that we probably should fix (Rethinking the Jupyter progress bar. #1087), but involves some other issues. Also, I think it isn't very useful to include the progress bar in the documentation (what does it matter how long that example took on someone else's computer?). But currently I don't see a way to disable it when building the notebooks for the documentation.
  • Disable cache as it might introduce warnings into the documentation that should not be there.

@tbekolay
Copy link
Member

Sphinx does not allow to have source files outside of the documentation folder (for security reasons I believe), but our examples folder lives outside the documentation folder. I am opposed to changing that, and I think @tbekolay agrees.

This was my original reason to investigate these types of options and make the PR I made for nbsphinx, but as time's passed I have to admit that I'm not as opposed to moving the notebooks to the docs directory as I used to be. I think the main argument to have an examples folder at the root level was to make it easily discoverable when people go to https://github.com/nengo/nengo/; however, I think it's going to become increasingly common for users to go to https://www.nengo.ai/nengo/ for help and not GitHub. For that reason, I think it might be a good time to 1) upgrade the notebooks to v4 and 2) move them to the docs folder. I added that to the next dev meeting agenda as something to discuss.

The Jupyter notebooks are executed by a Jupyter kernel that is independent from whatever virtualenv is active.

Hm, I've never experienced that behavior before... when I'm in my Python 2 environment, I get:

$ jupyter kernelspec list
Available kernels:
  python2    /home/tbekolay/.virtualenvs/nengo/share/jupyter/kernels/python2

and in my Python 3 environment:

$ jupyter kernelspec list
Available kernels:
  python3    /home/tbekolay/.virtualenvs/nengo3/share/jupyter/kernels/python3

What do you get?

In some notebooks input part of code cells has been collapsed.

I'm sure that it can be adapted, but also I'm not sure how strongly I feel about these input cells being collapsed in the first place... perhaps we should remove the hide_input function?

The progress bar widget doesn't work and instead a warning will be included.

Indeed, we should get rid of the progress bar in the docs... I have a few thoughts about how this might be done but would have to play around with a bit...

@jgosmann
Copy link
Collaborator Author

I think the main argument to have an examples folder at the root level was to make it easily discoverable when people go to https://github.com/nengo/nengo/;

In Nengo SPA I moved the examples folder into nengo_spa which allows to install them as package date and extract them to a user-defined location with python -m nengo_spa extract-examples <dst>. This doesn't work will with moving them to the docs folder. :/

My jupyter kernelspec list output (independent of virtualenv, all my virtualenvs are using Python 3 at the moment):

Available kernels:
  bleeding-edge          /home/jgosmann/.local/share/jupyter/kernels/bleeding-edge
  cogscie17-decide       /home/jgosmann/.local/share/jupyter/kernels/cogscie17-decide
  imem                   /home/jgosmann/.local/share/jupyter/kernels/imem
  kajic-frontiers2016    /home/jgosmann/.local/share/jupyter/kernels/kajic-frontiers2016
  nengodev               /home/jgosmann/.local/share/jupyter/kernels/nengodev
  python3                /home/jgosmann/.local/share/jupyter/kernels/python3

(and I can switch the virtualenv in a notebook by changing the kernel)

perhaps we should remove the hide_input function?

I'm probably ok with that.

Indeed, we should get rid of the progress bar in the docs... I have a few thoughts about how this might be done but would have to play around with a bit...

Can you roughly describe your thoughts?

@tbekolay
Copy link
Member

moved the examples folder into nengo_spa which allows to install them as package date

Makes sense!

jupyter kernelspec list output

Hmm, do you not install jupyter in your virtualenvs? Seems like your virtualenv / jupyter setup is very different from mine... not sure if these kinds of environmental differences should end up in a shared repo.

Can you roughly describe your thoughts?

I'm not sure how Sphinx works exactly, but the first thing I'd try is manipulating nengo.rc to turn off progressbars in the conf.py script. But maybe Sphinx or nbsphinx uses subprocesses for some part of the build, which would skirt the nengo.rc settings in that Python process, so if that's the case, then It might be possible to spit out a nengorc file in whatever directory the docs are being built from, and then remove it after the build (if there's already a nengorc file in that folder, then probably warn about the progress bars, leave it as is, and continue).

@jgosmann
Copy link
Collaborator Author

My Jupyter kernels reside in the user location which is not related to the virtualenv, whereas yours are in the “environment” location. Whether jupyter is installed in the virtualenv affects only the process managing the kernels (usually I start it without an active virtualenv if I don't want to test something with a specific Jupyter version). I think we can't rely on the default kernel being the correct one.

If we don't want the user to manually select the kernel, it might be possible to automatically install a kernel with the virtualenv's Python.

But maybe Sphinx or nbsphinx uses subprocesses for some part of the build

I think Sphinx launches independent Jupyter kernels which are unaffected by config manipulations (but I might be wrong).

, then It might be possible to spit out a nengorc file in whatever directory the docs are being built from, and then remove it after the build

I think this might actually work. Especially if we copy the examples, the file doesn't need to be removed.

@jgosmann
Copy link
Collaborator Author

then It might be possible to spit out a nengorc file in whatever directory the docs are being built from

That works indeed, I added a commit. :)

@jgosmann
Copy link
Collaborator Author

I think this should now be ready for review. I made hide_input work in both the documentation and Jupyter. But I'm also still open to removing it either as part of this PR or a different PR.

In Nengo SPA I had some incompatiblitly with the numfig feature and numpydoc (nengo/nengo-spa@b61b770) and replaced numpydoc with sphinx.ext.napoleon (which ships with Sphinx and supports both numpydoc and Google style docstrings). It could be nice to do the same here for consistency. However, in the Nengo documentation it introduces some other issues:

  • It doesn't play nice with the defaults. It changes the formatting a little bit and puts the argument data types into parenthesis, so that we always get weird looking parenthesis within parenthesis. This is somewhat easy to fix with a regex search and replace. (Though one could argue that the data type field isn't the right place for that information. According to the NumPy docs it is supposed to go into the description string if not taken from the function signature.)
  • The formatting of the attribute list takes up way too much space ([napoleon] rendering of google-style class attributes sphinx-doc/sphinx#2115). This is mostly fixed by the napoleon_use_ivar = True config option, but introduces a new problem:
  • With napoleon_use_ivar = True sphinx tries to link the attribute names to random stuff it shouldn't be linked to (PythonDomain.resolve_xref incorrectly matches ivars to class methods sphinx-doc/sphinx#2549). This won't be fixed before Sphinx 1.7.

For those reasons it seems best to stick with numpydoc in core Nengo for now.

@jgosmann
Copy link
Collaborator Author

One way to properly show the default values in the function signature would be to add docstrings like this one to the __init__ methods:

class Node(NengoObject):
    # ....
    def __init__(self, output=Default, size_in=Default, size_out=Default,
                 label=Default, seed=Default):
        """__init__(output=None, size_in=None, size_out=None, label=None, seed=None)"""
        # ...

But I'm not sure if we want to do that:

  • It adds a little bit more boilerplate to the code.
  • Having the defaults in the function signature might not be the most convenient location.
  • It might be misleading because the actual values will depend on the config (but not sure if it is really more misleading then the current notation of defaults).

@tbekolay
Copy link
Member

We discussed this at the dev meeting today and were generally in favor of moving the examples to the docs directory, and generally discourage people from viewing the notebooks through Github (as they're unevaluated). The main downsides to this are that:

  1. It may not be obvious how to edit the notebooks, and
  2. it may not be clear how to get the notebooks locally to evaluate them live.

To deal with these issues, we will 1) either manually add some text in the examples folder to say where all the examples live in the repo, and 2) make sure that the docs still include a link to download the .ipynb file. That might require a change in nbsphinx, but I've touched that code before and can likely make the PR if it's not already possible.

@Seanny123
Copy link
Contributor

To deal with these issues, we will

  1. either manually add some text in the examples folder to say where all the examples live in the repo, and

So, just adding a README.md into the examples folder?

  1. make sure that the docs still include a link to download the .ipynb file.

As in, adding this as a header to the rendered/uploaded docs?

Also, why not do both of these things?

@tbekolay
Copy link
Member

So, just adding a README.md into the examples folder?

No, the text would have to be in the rendered docs, as in the ideal case users would never need to know about the organization of the source tree and mostly interact with Nengo by pip installing and looking through the docs.

As in, adding this as a header to the rendered/uploaded docs?

Ideally this would be done automatically in the conversion from notebook to doc page. With nbsphinx, the only way to add this as a header would be to include it in the notebook itself, which feels weird.

@Seanny123
Copy link
Contributor

Given this new information, I vote for the second option of adding a header to the rendered page, which would understandably require a modification to nbsphinx.

@jgosmann
Copy link
Collaborator Author

I added a commit that changes the “View page source” link at the top right of normal documetation pages to a “Download example as Jupyter notebook” link for the examples. This is achieved by two things:

  • Overwriting the relevant part of the template. This unfortunately involved copying a ~30 line block from the original template. The templates support inheritance, but are not fine grained enough to just change that one link with overwriting a bunch of other links (e.g. to GitHub) that could be included (tough we don't include any of those links).
  • A hook into the end of the sphinx build process that copies the examples (they are actually already copied for the page source link, but a .txt extension is added. Copying them again seems to be the only way to get rid of that extension.

Let me know what you think of these changes.

@jgosmann jgosmann self-assigned this Oct 20, 2017
@jgosmann jgosmann force-pushed the nbsphinx2 branch 2 times, most recently from 387924a to 68fe598 Compare October 23, 2017 20:36
@jgosmann
Copy link
Collaborator Author

Rebased, apparently the documentation theme has changed. With the guzzle theme the template modification actually requires less boilerplate, so that is nice. :)

I think the only remaining thing is to add some documentation pointing to the new location of the examples folder. However, I am not sure where that should go and what it should say, @tbekolay. To me it doesn't seem particularly obscure to have the examples in the docs folder and I don't think we have any other parts of the folder structure documented (which doesn't mean that it isn't a good idea).

@jgosmann
Copy link
Collaborator Author

I mentioned that it would be nice to automatically extract parameter defaults from the source code and I think it might be possible with a small Sphinx extension, but that is probably something for a separate PR.

@Seanny123
Copy link
Contributor

Note: TravisCI is failing for reasons unrelated to this PR.

@Seanny123 Seanny123 mentioned this pull request Nov 27, 2017
2 tasks
@tbekolay
Copy link
Member

tbekolay commented May 9, 2018

OK I've rebased this now and fixed issues from the rebase; if there are no objections to the current state of this PR I'll compress the history a bit and merge after lunch.

@tbekolay tbekolay force-pushed the nbsphinx2 branch 5 times, most recently from b689662 to 45b090a Compare May 9, 2018 19:10
In order to do this, examples were moved to the docs folder.
When including them in the docs with nbsphinx, headings are
real reST headings, meaning they can be included in toctrees.
For that reason, we:

- Ensure there is only one level 1 heading
- Ensure headings have sentence case (not capitalized)
- Add metadata for syntax highlighting
Obsolete as it did the same job as nbshpinx.
@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 7, 2018

There was code for selecting the appropriate kernel when building the notebooks with nbsphinx in the PR (see discussion above), but that seems to have completely vanished? 😮

I still think it should be included, though I don't care as much about Nengo core as I usually don't have to build the documentation. But the same code seems to have gone missing from Nengo SPA too (or was it never in there?) which right now prevents me from building the docs with the correct kernel/virtualenv. 😞

@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 7, 2018

Luckily I found a local copy of the code.

@tbekolay
Copy link
Member

tbekolay commented Jun 7, 2018

Yeah, I removed that from this core PR, but I didn't touch anything in Nengo SPA.

@tbekolay
Copy link
Member

tbekolay commented Jun 7, 2018

As for why I removed it, it just seemed out of place in conf.py. It's a local environment thing, so it seems as though it should be handled in the local environment, not in conf.py. I am able to build these docs without that snippet, as is Travis CI.

@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 7, 2018

But it cannot be handled without modifying conf.py (I believe) because nbsphinx_kernel_name needs to be set.

@tbekolay
Copy link
Member

tbekolay commented Jun 7, 2018

You can pass that in to sphinx-build with the -D flag

@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 7, 2018

Does that somehow work with python setup.py build_sphinx (which is the command in our documentation)?

@tbekolay
Copy link
Member

tbekolay commented Jun 7, 2018

I'm not sure, but it's definitely easier to update the documentation than have that in conf.py

@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 7, 2018

Makes sense.

I rescued the code into a Gist in case I or someone else wants to refer to it at a later point.

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

Successfully merging this pull request may close these issues.

None yet

3 participants