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 instead of converting .ipynb -> .rst #212

Merged
merged 3 commits into from
Jan 27, 2016

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Jan 22, 2016

This is a proof-of-concept for using the nbsphinx extension to use notebooks as Sphinx sources instead of manually converting them to .rst files beforehand.

It still has a few rough edges regarding the CSS, but in general I think it works quite well.

@Carreau
Copy link
Member

Carreau commented Jan 22, 2016

Cool !

"metadata": {
"collapsed": false
},
"outputs": [
Copy link
Member

Choose a reason for hiding this comment

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

I take it nbsphinx executes the notebooks as part of the build, then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if there are no outputs, it does.
If there is at least one output, the notebook is not executed (see http://nbsphinx.readthedocs.org/en/latest/pre-executed.html).

@takluyver
Copy link
Member

I think this looks like a good idea.

@minrk
Copy link
Member

minrk commented Jan 25, 2016

I tested this locally, and it seems to work rather well. There seem to be merge conflicts, but I'd be happy to merge this once it's mergeable. Any further work to explore?

@minrk minrk added this to the 4.2 milestone Jan 25, 2016
@mgeier
Copy link
Contributor Author

mgeier commented Jan 25, 2016

I did a rebase, now it should be mergeable.

Note that the CSS is incomplete and it might not look nice on narrow browser windows and on old browsers, see spatialaudio/nbsphinx#11.

@willingc
Copy link
Member

@mgeier Thank you. This is really cool.

I only tried on Chrome but it looks pretty good as is on a narrow window.

I did notice a few things when I tested building the nbconvert documentation locally in a virtualenv with jupyter installed, the dev version of nbconvert with your PR, and the nbconvert/docs/requirements.txt:

  • First run of make html threw an error because one of the notebooks imported requests but that was not yet installed in the virtualenv. Installing requests resolved the error as expected. Something that we will need to document for nbconvert users in the nbconvert documentation.

Error
screen shot 2016-01-25 at 11 30 21 am

Source Notebook
screen shot 2016-01-25 at 11 45 05 am

  • Running make html sent warnings WARNING: using "math" markup without a Sphinx math extension active, please use one of the math extensions described at http://sphinx-doc.org/ext/math.html when a notebook with math in it was processed (example.ipynb). This is resolved by adding sphinx.ext.mathjax as an extension in conf.py.
  • Due to the notebooks being processed, the built documents will display errors and warnings underneath the source cell. Some examples:

Unable to execute
screen shot 2016-01-25 at 11 44 15 am

Deprecation warning
screen shot 2016-01-25 at 11 45 50 am

At minimum, this will be need to be documented. I'm a little concerned about what information might be displayed on an error or warning. It's not a big deal for an individual user but could be an issue when used for documentation or folks running batches of notebooks without close review of the output. If potentially sensitive or secure information were displayed within the error/warning messages, that would be a problem. Not sure of the likelihood of this. Thoughts from all?

Overall, this is a big improvement over the current way we have been using nbconvert in documentation. Thanks ☀️

@mgeier
Copy link
Contributor Author

mgeier commented Jan 26, 2016

A different solution to the requests dependency is to just get rid of it, which I did in f950abe.

Both WARNING: document isn't included in any toctree and WARNING: using "math" markup ... go away if example.ipynb is added to the exclude_patterns, like I did in 9eb45c9.

The further errors and warnings @willingc is describing would also appear when executing the notebooks manually beforehand. Exceptions will cause the build process to fail (unless they are ignored), but warnings on stderr won't.
I could change nbsphinx to raise a Sphinx error/warning whenever a stderr cell is present in a notebook, but I think that would be too much as a message on stderr might be completely harmless, like in the customizing.ipynb example where stdout is used for the actual output and an inconspicuous status message is printed on stderr:

[NbConvertApp] Converting notebook example.ipynb to python

The Bash error at the bottom is legitimate because the command isn't valid as-is.
There should be a separate notebook to illustrate using metadata in templates, or probably this could be incorporated into example.ipynb?
It would be no problem to run the command with nbsphinx, which would generate an HTML file which could be linked from a Markdown cell below the command.
Anyway, I think this can be done after this PR.

I don't know what caused the deprecation warning, i'm not getting that on my local installation.

Finally, I have no clue if any sensitive or secure information might be displayed in the output of Sphinx, but I think if anything, this is likely to be less of a problem if the notebooks are stored in the repository without output cells. The build process on readthedocs.org has only access to the files of the public repository, so I think it cannot produce anything more private/sensitive as is already available in the (public) repository.

@willingc
Copy link
Member

@mgeier My apologies, wrong notebook on the exclude. For a one off example, it may be okay to change the requests import but that's inefficient as a general solution. I don't want to hold up the PR since I would like to use this for other Jupyter docs. At minimum, this needs to be documented which I am happy to do.

The use case that I am most concerned about is someone who has been using nbconvert in their documentation, switches to nbsphinx expecting that the output would not include warnings, and then is surprised that the warnings are displayed in their production docs on RTD.

I'm +1 on merging this PR yet I would like f950abe to be removed and the example notebook left as is. I would rather add a section to the bottom of the docs/requirements.txt to include required packages in documentation notebooks (i.e. add requests to the end of the list).

@mgeier
Copy link
Contributor Author

mgeier commented Jan 27, 2016

I just thought it's strange to add an external dependency just to download a single file, which the standard library can perfectly do on its own. But anyway, I don't really care that much.

Shall I remove f950abe and instead add requests to the requirements.txt?

I still don't understand your concerns about the warnings.
If a user has notebooks with stored outputs, nothing changes, nbconvert does the same thing in both cases. All warnings that are stored in the notebook file will be shown in the HTML output.
If a notebook doesn't have any stored outputs, it will be executed by nbsphinx, is that what you think might be confusing?
Notebooks are executed by nbsphinx using the exact same mechanism as jupyter nbconvert --execute, so the resulting output will be the same again in both cases.

@takluyver
Copy link
Member

It looks like the bash cell is not supposed to be run as is. Maybe it should be a code block in a markdown cell rather than a code cell.

@takluyver
Copy link
Member

I mean the one with <your chosen notebook.ipynb> in the command.

@willingc
Copy link
Member

@takluyver Good point.

@mgeier I'm going to merge this PR as is. I'll fix up the notebooks and requirements.txt so that you can get back to working on more good stuff for nbsphinx. Thanks for creating nbsphinx. I really like it 😄 🌻 🍰

willingc added a commit that referenced this pull request Jan 27, 2016
Use nbsphinx instead of converting .ipynb -> .rst
@willingc willingc merged commit 174bfde into jupyter:master Jan 27, 2016
@mgeier mgeier deleted the try-nbsphinx branch January 27, 2016 16:27
@mgeier
Copy link
Contributor Author

mgeier commented Jan 27, 2016

Great, thanks a lot!

I wanted to look at the glorious result on readthedocs, but there was a build error:
http://readthedocs.org/projects/nbconvert/builds/3674982/

This exception was raised while executing nbconvert_library.ipynb:

ImportError: markdown2html requires mistune: No module named 'mistune'

Isn't that a dependency of nbconvert itself?

Is the option "Install your project inside a virtualenv using setup.py install" switched on in the "Advanced Settings" on readthedocs?

But even without that, nbsphinx should be in the requirements.txt and it depends on nbconvert which depends on mistune ... so what's wrong here?

Hmm, yet another possibility would be that the notebook wants to run python3 but the readthedocs build (and installation) is using Python 2.x ...?

@takluyver
Copy link
Member

I think we just need to add mistune to docs/requirements.txt. I've just done that, let's see if it works.

@willingc
Copy link
Member

@takluyver If it doesn't work (which I hope it does), it may be the recent change to jupyter_client. I noticed on jupyter/notebook that the doc build was failing for having setuptools 18.2 vs 18.5.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 27, 2016

Now it works, thanks!

I still don't understand why it's necessary to add this to the requirements.txt because it's supposed to be in the install_requires anyway.
Same for traitlets, jupyter_core, nbformat, BTW.

@willingc
Copy link
Member

@mgeier Cool. I agree that its weird that RTD is needing this.

@takluyver
Copy link
Member

I think it's because RTD invokes setup.py directly rather than doing pip install ., and we avoid using setuptools because of the utter putrid mess produced if that tool gets involved without pip shepherding it. Vanilla distutils doesn't do anything with install_requires.

@willingc
Copy link
Member

@takluyver Ugh! Thanks for the explanation. setuptools == utter putrid mess Well said.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 27, 2016

Ah, that explains it!
I wasn't aware that you're not using setuptools (and that distutils doesn't care about install_requires).

But if you don't mind my asking, why are you even bothering to pass install_requires to setup()?

@takluyver
Copy link
Member

When setup.py is run by pip, or when it runs a command like bdist_whl, it does use setuptools, so install_requires is significant.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 27, 2016

Thanks a lot for the explanation!

OK, first I thought: readthedocs uses pip to install nbsphinx, which depends on nbconvert, which would depend on mistune, but since nbconvert was already installed before via python setup.py install, its dependencies are not installed at this point, either.

But, according to the build log, pip install -r requirements is called before nbconvert is installed (again) with python setup.py install --force.

So I guess the problem only occured because the nbconvert installation was cached on readthedocs.

If I'm not mistaken, I think apart from caching, those other dependencies are not strictly necessary anymore for readthedocs since nbsphinx got into requirements.txt.

But if anyone else installs nbconvert first with python setup.py install, they'll have the same problem, so I think it's good to have those dependencies in the requirements.txt.

The reason why I myself didn't have this problem initially, is because I used python setup.py develop, which is caught as a special case in setup.py and it imports setuptools after all!

Sorry for all the confusion, but I just wanted to see if I can understand this stuff ...

Please allow me one final question: How are you installing the development version of nbconvert locally if you dislike setuptools so much?

@willingc
Copy link
Member

The readme outlines how to install the dev version locally. "-e" option on pip is the key.

Install nbconvert for development using:

git clone https://github.com/jupyter/nbconvert.git
cd nbconvert
pip install -e .

@willingc
Copy link
Member

pip install -e . is essentially http://pythonhosted.org/setuptools/setuptools.html#development-mode

To do this, use the setup.py develop command. It works very similarly to setup.py install or the EasyInstall tool, except that it doesn’t actually install anything. Instead, it creates a special .egg-link file in the deployment directory, that links to your project’s source code.

@willingc
Copy link
Member

So pretty much like you. Just under the hood of pip.

@takluyver
Copy link
Member

Please allow me one final question: How are you installing the development version of nbconvert locally if you dislike setuptools so much?

No problem :-). I symlink packages I'm working on into ~/.local/lib/python3.4/site-packages/ (and copy/create scripts as necessary). I wouldn't particularly recommend this workflow for everyone, but I like it: I know exactly what's going on, and it doesn't involve certain packaging libraries which shall not be named ;-).

I actually made my own packaging tool, flit, which works like this for development installs.

@mgeier
Copy link
Contributor Author

mgeier commented Jan 27, 2016

Thanks a lot for the explanations, that's good to know!

@Carreau
Copy link
Member

Carreau commented Jan 27, 2016

Ugh! Thanks for the explanation. setuptools == utter putrid mess Well said.

I think I'm going to register utterputridmess on PyPI and make is an alias of setuptools, like numpee and spicy.

@takluyver
Copy link
Member

Are you going for a 'most packages on PyPI' record?

Next step, create a script so you can do it all with pypi-alias spicy scipy. ;-)

@Carreau
Copy link
Member

Carreau commented Jan 27, 2016

Are you going for a 'most packages on PyPI' record?

Nah, I know the owner of egg and chicken and dot though. Who for whatever reason added me as owner on some packages.

I also have some other packages for fun.

@mgeier mgeier mentioned this pull request Feb 24, 2016
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.

5 participants