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

Finish integration of Jupyter notebooks with Sphinx docs #1215

Merged
merged 18 commits into from Nov 22, 2017

Conversation

@Bultako
Copy link
Member

@Bultako Bultako commented Nov 20, 2017

$ setup.py build_docs

  • added flag execute_notebooks = False in setup.cfg sets no execution of notebooks during the docs building process
  • added flag rebuild_notebooks = True in setup.cfg sets always replace notebooks from $GAMMAPY_EXTRA and rebuild
  • copies notebooks from $GAMMAPY_EXTRA to _static/notebooks folder, so RST files, ipynb notebooks, and sphinx formatted notebooks live in the same version/tag
  • links added in fixed-text sphinx formatted notebooks for downloading live *.ipynb notebooks
  • transforms absolute links from notebooks to http://docs.gammapy.org website into relative links to RST files, so RST files, ipynb notebooks, and sphinx formatted notebooks live in the same version/tag
  • added some documentation related with these modifs

previous work:
#1176

see generated doc:
http://www.iaa.es/~jer/gammapydocs/notebooks/first_steps.html

Bultako added 8 commits Nov 17, 2017
regexp in cells replacing absolute links to http://docs.gammapy.org/en/latest into relative
links present in raw notebooks (.ipynb files) and sphinx notebooks (.html formatted notebooks)
relative links guarantee pointing to the same old doc version, not necessary the latest
Links from notebooks pointing to ../datasets/index.rst#gammapy-extra are not found raising a warning.
@cdeil cdeil self-assigned this Nov 21, 2017
@cdeil cdeil added the docs label Nov 21, 2017
@cdeil cdeil added this to the 0.7 milestone Nov 21, 2017
@cdeil
Copy link
Member

@cdeil cdeil commented Nov 21, 2017

@Bultako - Thanks!

The main thing before this can go in is that the docs build in our CI has to be green.
I'm not 100% sure, but I think it's failing because of these new warnings:
https://travis-ci.org/gammapy/gammapy/jobs/304759871#L1953
(we run with build_docs -w to turn warnings into errors to make the CI build fail, see here).

One suggestion I have would be to move the code you've added to docs/conf.py mostly out to gammapy/utils, like we did here:

from gammapy.utils.docs import gammapy_sphinx_ext_activate

The problem with putting more and more code in docs/conf.py is that it's hard to maintain (i.e. read / test) because docs/conf.py is already long and spaghetti code with a lot of Sphinx-specific stuff. So I think we should try to avoid it growing much and instead put as much code as possible in modules / functions / classes (whatever is most convenient) in gammapy/utils so that it becomes more readable (and in the future maybe even testable, although for now I don't think we should try to add tests -- the real docs build is the test we have and need).

I'll have a closer look now and leave some inline comments.

@@ -88,7 +92,11 @@
extensions.append('nbsphinx')
extensions.append('IPython.sphinxext.ipython_console_highlighting')
extensions.append('sphinx.ext.mathjax')
nbsphinx_execute = 'never'
if eval(setup_cfg.get('execute_notebooks')):

This comment has been minimized.

@cdeil

cdeil Nov 21, 2017
Member

This looks weird. Why take one boolean execute_notebooks and convert it to a two-choice nbsphinx_execute = {'always', 'never'}? Suggest to just put what you want in setup.cfg and then use it directly.

Also: calling eval here really isn't needed. If setup_cfg.get doesn't yield a bool directly, then pass the string through the bool function:

execute_notebooks = bool(setup_cfg.get('execute_notebooks'))
filepath = os.path.join(folder, filename)
if os.path.isfile(filepath) and filepath[-6:] == '.ipynb':
if folder=='notebooks':
strcell = DOWNLOAD_CELL.replace('nbfilename.ipynb', filename)

This comment has been minimized.

@cdeil

cdeil Nov 21, 2017
Member

I would suggest to use str.format instead of str.replace to fill things in.
I.e. something like this:
template = """
Download file: {nb_filename}.
"""
and then

ctx = dict(nb_filename=nb_filename) # gather things in a "context" dict if you have many
txt = template.format(**ctx) # pass context to template.format
nb = nbformat.read(filepath, as_version=nbformat.NO_CONVERT)
nb.cells.insert(0, new_markdown_cell(strcell))
nbformat.write(nb, filepath)
with open(filepath, "r") as f:

This comment has been minimized.

@cdeil

cdeil Nov 21, 2017
Member

"r" mode is the default -> remove.

f.write(txt)

# remove existing notebooks if rebuilding
if eval(setup_cfg.get('rebuild_notebooks')):

This comment has been minimized.

@cdeil

cdeil Nov 21, 2017
Member

I find this name "rebuild_notebooks" non-optimal, because it doesn't rebuild them, it's removes the old ones.

Note that we already have this option:

python setup.py build_docs --help

  --clean-docs (-l)            Completely clean previous builds, including
                               automodapi-generated files before building new
                               ones

It's probably not easy to add to this option or add a new one. But then for now I'd suggest to rename your option in setup.cfg to clean_notebooks.

@cdeil
Copy link
Member

@cdeil cdeil commented Nov 21, 2017

@Bultako - I've left a few inline comments.

I like how you've introduced the info box at the top with a download link for the notebook!

screen shot 2017-11-21 at 10 55 02

I have a few suggestions that I think would make it more intuitive for users:

For the "download notebook" link, I think it would be great to change to "Download notebook: nbfile.ipynb" and to just have the "nbfile.ipynb" be the link. This would IMO make it more intuitive for users which file they will download and then will have to look for locally to execute. I also think it would be good to add the filenames to every notebook linked to from this page: http://docs.gammapy.org/en/latest/notebooks.html#notebooks again because people will have to know the filenames of the notebooks to start executing them and recognise them.

For the description you have for downloading all HTML locally, I think it would be better if you put a link "Download this version of the docs with all notebooks: gammapy.zip" and have the filename be a link that downloads http://readthedocs.org/projects/gammapy/downloads/htmlzip/latest/ . So don't rely on people finding the javascript applet from RTD and the "HTML" link inside that. Since this part will be the same for all notebooks, and then users will need some more info how to find and start the notebooks, that could also go on a separate docs page and then just link to it from the info box. As you like for now.

Finally, on nbviewer (e.g. https://nbviewer.jupyter.org/github/gammapy/gammapy-extra/blob/master/notebooks/cta_1dc_introduction.ipynb) in the top right corner there's two more useful links: one to the notebook on Github (i.e. the file someone would have to edit if they wanted to contribute) and one called "view as code" that is just a text file that people could copy & paste from into a Python script. Both of those aren't essential, but if they are easy to do, I think it would be useful to have.

@Bultako - The big extra feature would be if people could execute the examples on Binder. Axel and I tried quite a bit in the past, but mostly failed to get it to run properly. If you are interested to add it, please have a look at #726 and comment there if you want to try setting it up again for Gammapy. (Let's keep that separate from this PR, to avoid it getting very large or stalled).

@Bultako Bultako force-pushed the Bultako:nbsphinx branch from 2567e74 to e3c8536 Nov 21, 2017
@Bultako Bultako force-pushed the Bultako:nbsphinx branch from e3c8536 to 26a4a2e Nov 21, 2017
@cdeil
Copy link
Member

@cdeil cdeil commented Nov 21, 2017

@Bultako - Locally I always see these three warnings on python setup.py build_docs:
https://gist.github.com/cdeil/9419c8d57ecf7f2e4bc90522757d942e#file-gistfile1-txt-L485

/Users/deil/temp/gammapy/docs/notebooks/cta_1dc_introduction.ipynb:9: WARNING: Explicit markup ends without a blank line; unexpected unindent.
/Users/deil/temp/gammapy/docs/notebooks/cta_1dc_introduction.ipynb:1657: WARNING: Explicit markup ends without a blank line; unexpected unindent.
/Users/deil/temp/gammapy/docs/notebooks/cta_data_analysis.ipynb:9: WARNING: Explicit markup ends without a blank line; unexpected unindent.

This is on a clean gammapy git checkout - it shouldn't have anything to do with me having old build files lying around. I don't understand why these warnings appear for me, but not on travis-ci (see https://travis-ci.org/gammapy/gammapy/jobs/305233458).

!?

@Bultako
Copy link
Member Author

@Bultako Bultako commented Nov 21, 2017

@cdeil
we are using notebooks from last version of gammapy-extragithub repo
so I guess you should check if you are in a clean gammapy-extra git checkout -

@cdeil
Copy link
Member

@cdeil cdeil commented Nov 21, 2017

My gammapy-extra is at fe9daa9b59d97570175b85803bc0bf2d11037260 which is up-to-date as of today: https://github.com/gammapy/gammapy-extra/commits/master

Also git status there doesn't show any modifications and I also did git clean -fdx in the gammapy-extra repo just to be sure.

I really don't understand this. The only thing I can think of are version differences between my machine (e.g. in nbconvert)!?

@Bultako
Copy link
Member Author

@Bultako Bultako commented Nov 21, 2017

@cdeil
I would check the version of nbsphinx
.travis.yml is using last version of nbsphinx 0.2.17 installed with pip

https://github.com/Bultako/gammapy/blob/26a4a2e9ca08d316705a8199478a90688a5565b2/.travis.yml#L38

conda-forge provides an older version that does not handle links with RST files properly

@cdeil
Copy link
Member

@cdeil cdeil commented Nov 21, 2017

I updated nbsphinx, but I still see these three warnings. This is what I have now:

In [2]: nbsphinx.__version__
Out[2]: '0.2.17'

In [4]: nbconvert.__version__
Out[4]: '5.3.1'

In [6]: nbformat.__version__
Out[6]: '4.4.0'
@Bultako
Copy link
Member Author

@Bultako Bultako commented Nov 21, 2017

just for info:

In [5]: nbsphinx.__version__
Out[5]: '0.2.17'

In [6]: nbconvert.__version__
Out[6]: '5.2.1'

In [7]: nbformat.__version__    
Out[7]: '4.3.0'

missing a shared docker container for reproducibility.
when conda config YAML files for virtual envs are not enough.
I'll have a deeper look later.

@Bultako
Copy link
Member Author

@Bultako Bultako commented Nov 21, 2017

@cdeil
I think I've fixed all your review comments, you can see the new generated docs here:
http://www.iaa.es/~jer/gammapydocs/notebooks.html

I suspect some more fixes may be needed so RTD could serve *.ipynb and *.py files as downloadable files.

in relation to your local warnings, not a clue :(

@Bultako Bultako force-pushed the Bultako:nbsphinx branch from e214af9 to 010f258 Nov 21, 2017
Copy link
Member

@cdeil cdeil left a comment

@Bultako - This looks very good!

I've put some minor code suggestions inline.

The download links look great now:

Source files: astropy_introduction.ipynb | astropy_introduction.py

Currently clicking the links doesn't download a file, but opens up the file as text in the browser. Would it be possible and preferable to have the default behaviour on click be a download? I'm not sure it's possible, but with this or by putting a raw HTML a tag with download attribute like this it might.
Alternatively: maybe add something like this on the line below:

(to download the files to your local machine: right-click and select "save as")
@@ -88,7 +88,8 @@
extensions.append('nbsphinx')
extensions.append('IPython.sphinxext.ipython_console_highlighting')
extensions.append('sphinx.ext.mathjax')
nbsphinx_execute = 'never'
nbsphinx_execute = setup_cfg['execute_notebooks']

This comment has been minimized.

@cdeil

cdeil Nov 22, 2017
Member

This line can now be removed from conf.py, because that local variable is now unused, no?

This comment has been minimized.

@Bultako

Bultako Nov 22, 2017
Author Member

we actually need it in conf.py if we want to manage the option to enable/disable the execution of notebooks during the process of doc generation
http://nbsphinx.readthedocs.io/en/0.2.9/never-execute.html

we can also remove it from step.cfg and leave it as hard text in conf.py

This comment has been minimized.

@cdeil

cdeil Nov 22, 2017
Member

Ah, OK. Could you please add a one-line comment?

# Configure whether to run nbsphinx; see http://nbsphinx.readthedocs.io/en/stable/never-execute.html

I think leaving this configurable via setup.cfg is better, no?

This comment has been minimized.

@Bultako

Bultako Nov 22, 2017
Author Member

yes, I would leave it in setup.cfg since other config options (also the ones used for doc generation) are living there.

Sphinx formatted documentation files and at the same time provides access to raw
.ipynb Jupyter notebooks for the same version of the gammapy documentation. This
behaviour may be modified in the `setup.cfg` configuration file changing the
value of `rebuild_notebooks` boolean.

This comment has been minimized.

@cdeil

cdeil Nov 22, 2017
Member

option was renamed, is no longer rebuild_notebooks -> adapt docs.


# remove existing notebooks if rebuilding
if bool(setup_cfg.get('clean_notebooks')):
print('*** Cleaning notebooks')

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@cdeil

cdeil Nov 22, 2017
Member

@Bultako - Thanks for figuring out how to use logging with Sphinx!

Our docs / sphinx tooling will grow over time, so it's good if we do things properly.

While you're at it, would you mind also changing these lines further up in the file to log messages?

$ ack print gammapy/utils/docs.py 
        print('*** Found GAMMAPY_EXTRA = {}'.format(gammapy_extra_path))
        print('*** Nice!')
        print('*** gammapy-extra *not* found.')
        print('*** Set the GAMMAPY_EXTRA environment variable!')
        print('*** Docs build will be incomplete.')
        print('*** Notebook links will not be verified.')

This comment has been minimized.

@Bultako

Bultako Nov 22, 2017
Author Member

sure, it's done :)

Manages the processes for the building of sphinx formatted notebooks
"""

url_docs = setup_cfg.get('url_docs')

This comment has been minimized.

@cdeil

cdeil Nov 22, 2017
Member

Suggest to change to url_docs = setup_cfg['url_docs'].

The difference is that dict.get will return None by default. And if you put None here in url_docs, I think your code will error much later with a TypeError.

Better to fail early and clearly. And in this case, the url_docs key should always be present, so no need to implement special code for the case where it's not present.

url_docs = setup_cfg.get('url_docs')

# remove existing notebooks if rebuilding
if bool(setup_cfg.get('clean_notebooks')):

This comment has been minimized.

@cdeil

cdeil Nov 22, 2017
Member

Same here, I'd suggest to use setup_cfg['clean_notebooks'] instead of the get call.

@Bultako Bultako force-pushed the Bultako:nbsphinx branch from a3f6244 to 708d853 Nov 22, 2017
@Bultako
Copy link
Member Author

@Bultako Bultako commented Nov 22, 2017

@cdeil
re: forcing download in links

I don't know how to do this. The point is that nb_sphinx makes the conversion from .ipynb to .rst files before actually executing Sphinx, transforming markdown syntax for links in notebooks to rst syntax for links. I have no access to these intermediate short-living rst files, ipynb are converted to html directly. This does not allow any simple way for fine-tuning links and add a download attribute to a hrefs, neither the :download: role added in the .ipynb file seems to work.

For the moment I've just added the message right-click and select "save as"

@cdeil
Copy link
Member

@cdeil cdeil commented Nov 22, 2017

OK. Ready to merge?

@Bultako
Copy link
Member Author

@Bultako Bultako commented Nov 22, 2017

Ready

@cdeil
cdeil approved these changes Nov 22, 2017
@cdeil cdeil merged commit af4c7a9 into gammapy:master Nov 22, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil
Copy link
Member

@cdeil cdeil commented Nov 22, 2017

Thank you!

@Bultako Bultako deleted the Bultako:nbsphinx branch Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants