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

DOC: Use nbsphinx #15581

Merged
merged 1 commit into from
Apr 8, 2017
Merged

DOC: Use nbsphinx #15581

merged 1 commit into from
Apr 8, 2017

Conversation

TomAugspurger
Copy link
Contributor

@TomAugspurger TomAugspurger commented Mar 5, 2017

Update header levels for nbsphinx

Link to nb, nicer default table

Closes #15539

@TomAugspurger
Copy link
Contributor Author

Here's a preview

screen shot 2017-03-05 at 12 28 50 pm

The doc build may fail to convert the notebook if we don't have pandoc on the CI server. It's available through conda-forge if we need, but wanted to test without first.

@jorisvandenbossche
Copy link
Member

Why does it need pandoc?

You also included two versions of the notebook, was that on purpose?
I also don't see where you included it in any toctree.

But seems to look much nicer!

@codecov-io
Copy link

codecov-io commented Mar 5, 2017

Codecov Report

Merging #15581 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #15581   +/-   ##
=======================================
  Coverage   90.99%   90.99%           
=======================================
  Files         145      145           
  Lines       49565    49565           
=======================================
  Hits        45101    45101           
  Misses       4464     4464
Flag Coverage Δ
#multiple 88.76% <ø> (ø) ⬆️
#single 40.56% <ø> (ø) ⬆️

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 d60f490...a2f3056. Read the comment docs.

@TomAugspurger
Copy link
Contributor Author

Why does it need pandoc?

Oh, I think it's the same as nbconvert, so nothing new.

You also included two versions of the notebook, was that on purpose?

Nope, fixed.

I also don't see where you included it in any toctree.

We had the dummy style.rst file that just included the converted html in a raw block. I removed style.rst and renamed the notebook to style.ipynb, so it gets picked up by the style already in the index.rst toctree.

@jreback
Copy link
Contributor

jreback commented Mar 5, 2017

@TomAugspurger you need to add the install of nbsphinx in ci/requirements-3.5_DOC_BUILD.sh (you can add as same line as feather-format). That's the only place we explicitly install from conda-forge.

@@ -114,6 +114,10 @@ if [ "$COVERAGE" ]; then
pip install coverage pytest-cov
fi

if [ "$DOC_BUILD" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

move instead to ci/requirements-3.5_DOC_BUILD.sh (and I would install install via conda-forge channel (just like feather-format)

@@ -81,7 +81,9 @@ have ``sphinx`` and ``ipython`` installed. `numpydoc
<https://github.com/numpy/numpydoc>`_ is used to parse the docstrings that
follow the Numpy Docstring Standard (see above), but you don't need to install
this because a local copy of ``numpydoc`` is included in the pandas source
code.
code. `nbsphinx <https://nbsphinx.readthedocs.io/>`_ is used to convert
Jupyter notebooks. You will need to install it if you intend to modify any of
Copy link
Contributor

Choose a reason for hiding this comment

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

should prob add this to the requirements_docs (which should also have conda-forge as a channel in the instructions)

@TomAugspurger
Copy link
Contributor Author

Fixed, thanks. Also added seaborn to the doc build (used in the notebook). I set the option to allow errors when executing the notebook, as that seems more consistent with the rest of the doc build.

@jorisvandenbossche
Copy link
Member

@TomAugspurger doc build is failing on travis due to missing pandoc. So apparently this was previously not needed.

@@ -321,3 +321,27 @@ div.seealso dd {
margin-top: 0;
margin-bottom: 0;
}

table {
Copy link
Member

Choose a reason for hiding this comment

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

There is some styling for rendered_html tables above. Should that then be removed? (I think that was added for the converted styling notebook)

Also, would this also influence the tables in the other docs? So maybe this should have a 'dataframe' selector?

@@ -3,6 +3,7 @@ pytest-cov
pytest-xdist
flake8
sphinx
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, is this our catch all for what people should install to get a dev env? should add seaborn 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.

We also have requirements_dev.txt, which I think is more appropriate. I read requirements_all.txt as all the optional deps for using pandas (xlrd, xlwt, tables, etc.). Maybe sphinx and the test libs should go in requirements_dev.txt?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I think these should be more clear what the purpose of these are. You can add a comment (or 2) at the top of the file I think (# is ignored)

@TomAugspurger
Copy link
Contributor Author

@TomAugspurger doc build is failing on travis due to missing pandoc. So apparently this was previously not needed.

I'd rather not have pandoc as a required dependency to build the docs. I'll see what I can do to make this fail gracefully without pandoc installed.

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Mar 6, 2017

Previously, we were using nbconvert to just convert it to html, which did not needed pandoc. But with nbsphinx I suppose this converts it to rst under the hood and therefore uses pandoc? (not sure if that is a correct assessment, the docs of nbsphinx are not very clear on the pandoc requirement in their installation notes)
But, if that is correct, and if we want to include this in our dev doc build, then we just need to install pandoc? (you can add it to the conda-forge packages to install)

But indeed, it would be nice that this would not fail the doc build if you do that locally without having pandoc installed. EDIT: ah, this last thing was the point you were making I suppose :-)

@TomAugspurger TomAugspurger force-pushed the nbsphinx branch 2 times, most recently from 999f77b to 0033aa7 Compare March 9, 2017 15:20
@jreback
Copy link
Contributor

jreback commented Mar 9, 2017

this lgtm. @jorisvandenbossche had some comments though.

@TomAugspurger
Copy link
Contributor Author

Oh, whoops, forgot I had already made a merge request. I'll let you know when this is ready for review.

Quick update though: I raised an issue on nbsphinx about gracefully failing if there's no pandoc. I think they're open, but 1.) they have a stalled PR removing the need for pandoc entirely, and 2.) it's a bit tricky since the errors are raised on each markdown cell (do you skip the cell, or the entire nb?)

So I think it's probably best we just workaround it for now by removing the ipynb file(s) before calling out to sphinx. Also, should we do something like my maybe_exclude_notebooks for the regular .rst files? It can be a bit strange for a new contributor to have a bunch of files removed when you tell them to use --single=.

@TomAugspurger
Copy link
Contributor Author

Are people OK with using requirements_all.txt to contain all the optional runtime deps to use pandas, and requirements_dev.txt to contain all the optional deps to build docs and run tests?

Pip let's you source additional requirements files from another like so

# ci/requirements_dev.txt
# All additional dependencies required for running tests, building docs
# Main requirements are in ci/requirements_all.txt
-r requirements_all.txt
# Building
cython
# Testing
flake8
pytest
pytest-cov
pytest-xdist
# Doc building
sphinx
nbsphinx
nbconvert
ipython
seaborn

Does anyone know if this is possible with conda? If not I'll open an issue requesting it.

@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Mar 11, 2017

Reading a bit more, it seems like requirements_dev.txt was intended to be a minimal set of dependencies to develop pandas. I think it's a bit strange the doc dependencies (at least sphinx) aren't in there, but the test dependencies are.

One additional option is

  • ci/requirements_base.txt: Required deps only (plus cython?)
  • ci/requirements_dev.txt: Test and doc deps (and cython if it's not in base)
  • ci/requirements_all.txt: all optional deps

And then the conda install instructions are

conda env create -n pandas_dev python=3 -f ci/requirements_base.txt
conda install -n pandas_dev --file=ci/requirements_all.txt
conda install -n pandas_dev --file=ci/requirements_dev.txt
[source] activate pandas_dev

@jreback
Copy link
Contributor

jreback commented Mar 11, 2017

ci/requirements_dev.txt: Test and doc deps (and cython if it's not in base)

should also have a requirements_docs.txt I think (with only the doc-stuff).

@TomAugspurger
Copy link
Contributor Author

Looks like conda is close to allowing -r syntax in a requirements.txt. I'm going to just add nbsphinx to requirements_all.txt for now, and open another issue for reorganizing the requirements_*.txt files.

@@ -6,6 +6,6 @@ echo "[install DOC_BUILD deps]"

pip install pandas-gbq

conda install -n pandas -c conda-forge feather-format
conda install -n pandas -c conda-forge feather-format nbsphinx pypandoc
Copy link

Choose a reason for hiding this comment

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

What do you need pypandoc for?
Wouldn't pandoc be more appropriate instead?
https://anaconda.org/conda-forge/pandoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, your correct. I forgot they were packaged separately. Will update.

yield
for nb, content in contents.items():
with open(nb, 'wt') as f:
f.write(content)
Copy link

Choose a reason for hiding this comment

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

Am I seeing correctly that you are writing the file(s) each time after Sphinx is run?
This will force re-parsing each time you run it again ...
Otherwise, Sphinx would simply skip over the already built (and up-to-date) notebooks.

Also, if somebody is editing the notebook(s) at the same time, this might cause problems, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I seeing correctly that you are writing the file(s) each time after Sphinx is run?

If nbsphinx is installed then contents will be empty. So we should only be writing stuff if nbsphinx is not installed, and sphinx isn't parsing these files anyway.

For editing the notebooks at the same time, I don't think there are any additional problems with this approach.

Copy link

Choose a reason for hiding this comment

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

OK, I see now. It still seems a bit brutal to me and I'm generally skeptical about writing to the source directory, but I guess it's OK.

@mgeier
Copy link

mgeier commented Mar 12, 2017

@jorisvandenbossche

But with nbsphinx I suppose this converts it to rst under the hood and therefore uses pandoc? (not sure if that is a correct assessment, the docs of nbsphinx are not very clear on the pandoc requirement in their installation notes).

Yes, this is correct. At least until spatialaudio/nbsphinx#36 is done (which is a biggie).

You are also right that the documentation doesn't mention that, but we had at least an issue for that: spatialaudio/nbsphinx#72.
I just created a PR which hopefully closes this issue: spatialaudio/nbsphinx#102.

Looking forward to Jupyter notebooks in the pandas docs!


div.seealso dd {
margin-top: 0;
margin-bottom: 0;
Copy link
Member

Choose a reason for hiding this comment

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

Should this have been removed? (not sure if it does something useful)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm probably an accident on my part. I'll push a fix (the rendered_html removal was intentional).

@@ -52,14 +52,16 @@
'numpydoc', # used to parse numpy-style docstrings for autodoc
'ipython_sphinxext.ipython_directive',
'ipython_sphinxext.ipython_console_highlighting',
'IPython.sphinxext.ipython_console_highlighting', # lowercase didn't work
Copy link
Member

Choose a reason for hiding this comment

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

Note there is a difference here in _ vs .. The one already present with the _ is our vendored version (and the directory is with lower case, so that works). What was the reason you needed this?
(although we should maybe migrate to just use IPython itself instead of our own version, but should look again at why we exactly were doing this)

@jreback jreback added this to the 0.20.0 milestone Mar 27, 2017
@jreback
Copy link
Contributor

jreback commented Mar 29, 2017

can you rebase (happy to push this in as well if you are ready)

@TomAugspurger
Copy link
Contributor Author

TomAugspurger commented Mar 29, 2017 via email

the Jupyter notebooks included in the documentation.

If you have a conda environment named ``pandas_dev``, you can install the extra
requirements with::

conda install -n pandas_dev sphinx ipython nbconvert nbformat
conda install -n pandas_dev -c conda-forge nbsphinx
Copy link
Contributor

Choose a reason for hiding this comment

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

we should just always add -c conda-forge (as defaults is always added), and things just work

@jreback
Copy link
Contributor

jreback commented Mar 29, 2017

@TomAugspurger merge? (minor comment on the installation of libs).

@jreback
Copy link
Contributor

jreback commented Apr 3, 2017

ready for merge?

@TomAugspurger
Copy link
Contributor Author

Going to wait a day or two to see if #14757 is updated, since there will be a notebook conflict.

@jreback
Copy link
Contributor

jreback commented Apr 3, 2017

sure

Adds a new doc-dependency nbsphinx for converting jupyter notebooks
to ReST, which works better with the sphinx conversion process.
Remvoes the hacky notebook -> HTML -> raw include we had before.
@jreback
Copy link
Contributor

jreback commented Apr 8, 2017

nice @TomAugspurger couple of comments on the built docs: http://pandas-docs.github.io/pandas-docs-travis/style.html

  • maybe need some :okwarning: to eliminate some of the displayed warnings (or slight code changes)
  • can you align the numbers in the tables (maybe make this the first table)

TomAugspurger added a commit to TomAugspurger/pandas that referenced this pull request Apr 8, 2017
Followup to pandas-dev#15581

Using the `nbsphinx: hidden` metadata to hide the ouptut, so
readers don't see matplotlib's fc-list warning.

Make the tables monospaced in CSS.
TomAugspurger added a commit to TomAugspurger/pandas that referenced this pull request Apr 8, 2017
Followup to pandas-dev#15581

Using the `nbsphinx: hidden` metadata to hide the ouptut, so
readers don't see matplotlib's fc-list warning.

Make the tables monospaced in CSS.
jreback pushed a commit that referenced this pull request Apr 8, 2017
Followup to #15581

Using the `nbsphinx: hidden` metadata to hide the ouptut, so
readers don't see matplotlib's fc-list warning.

Make the tables monospaced in CSS.
@jnothman
Copy link
Contributor

The documentation built with nbsphinx uses a different font to the rest of the docs, i.e. a serif font.

@TomAugspurger
Copy link
Contributor Author

Whoops, that's on me. Actually comes from my #15954 PR. I'll put up a fix. Thanks.

TomAugspurger added a commit to TomAugspurger/pandas that referenced this pull request Apr 20, 2017
This was overriding the CSS on the rest of the page.
Reported in
pandas-dev#15581 (comment)
Also inlines the CSS from our theme.

[ci skip]
TomAugspurger added a commit that referenced this pull request Apr 21, 2017
This was overriding the CSS on the rest of the page.
Reported in
#15581 (comment)
Also inlines the CSS from our theme.

[ci skip]
TomAugspurger added a commit to TomAugspurger/pandas that referenced this pull request Apr 26, 2017
When switching to nbsphinx, I modified the site's CSS so
that the converted notebook looks decent. This had some
unfortunate changes on tables elsewhere in the notebook.

This change fixes the headers to be left-aligned in the main site,
and right-aligned for the tables generated by `df.style` in the
nbsphinx-converted notebook.

xref pandas-dev#15581
TomAugspurger added a commit that referenced this pull request Apr 26, 2017
When switching to nbsphinx, I modified the site's CSS so
that the converted notebook looks decent. This had some
unfortunate changes on tables elsewhere in the notebook.

This change fixes the headers to be left-aligned in the main site,
and right-aligned for the tables generated by `df.style` in the
nbsphinx-converted notebook.

xref #15581
pcluo pushed a commit to pcluo/pandas that referenced this pull request May 22, 2017
This was overriding the CSS on the rest of the page.
Reported in
pandas-dev#15581 (comment)
Also inlines the CSS from our theme.

[ci skip]
pcluo pushed a commit to pcluo/pandas that referenced this pull request May 22, 2017
When switching to nbsphinx, I modified the site's CSS so
that the converted notebook looks decent. This had some
unfortunate changes on tables elsewhere in the notebook.

This change fixes the headers to be left-aligned in the main site,
and right-aligned for the tables generated by `df.style` in the
nbsphinx-converted notebook.

xref pandas-dev#15581
@TomAugspurger TomAugspurger deleted the nbsphinx branch May 29, 2017 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants