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

[MRG] Documentation Auto API #151

Merged
merged 10 commits into from Apr 9, 2017

Conversation

Projects
None yet
4 participants
@Titan-C
Member

Titan-C commented Oct 9, 2016

This PR improves the documentation on getting Sphinx gallery backreferences into auto documented API. Closes #126

There are some incompatible changes. The backreferences are disabled by default. The user must activate them manually by specifying the directory where to save them.

@Titan-C Titan-C added this to the Release V0.1.5 milestone Oct 9, 2016

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Oct 10, 2016

I am a bit worried about those backward incompatible changes. We now have a sizeable number of packages who use us, and we are going to break their documentation build the minute we release.

Is there any way to have a depreciation cycle? Could we have step by step instructions to migrate from the old way to the new way?

@Titan-C

This comment has been minimized.

Member

Titan-C commented Oct 10, 2016

Is there any way to have a depreciation cycle? Could we have step by step instructions to migrate from the old way to the new way?

I would also like to make a deprecation cycle. Maybe I can leave the old default value and put a warning that I'll now require the variable to be configured. The migration is just set up the value, the complete goal of this to make the user more aware of this configuration as it is error prone see #126 (comment), many places have to be configured coherently.

This PR is mostly a documentation change to deal with the confusion of setting up the sphinx-autodoc and sphinx-autosummary to work with sphinx gallery backreferences, read #126.

My approach to it was to force the user to become conscious of this process, as I don't see a way to automate this process, see #126 (comment).

@Titan-C Titan-C force-pushed the Titan-C:doc_autoapi branch from 946d722 to 1698470 Oct 19, 2016

@Titan-C Titan-C force-pushed the Titan-C:doc_autoapi branch from 596412e to 4a039ee Nov 7, 2016

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 12, 2016

Some conflicts to fix here.

@Titan-C Titan-C force-pushed the Titan-C:doc_autoapi branch 5 times, most recently from a1b578b to 60c6f51 Dec 17, 2016

@Titan-C

This comment has been minimized.

Member

Titan-C commented Dec 17, 2016

Rebased to master. New docs on the backreferences configuration. Proof reading required.

@Titan-C Titan-C force-pushed the Titan-C:doc_autoapi branch 2 times, most recently from 4c8dc4a to bf0b830 Dec 19, 2016

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jan 3, 2017

Can I get proof readers on this one? @lesteve @Eric89GXL
This is a documentation change, and behavior change of the gallery to deal with the issues from #126 about configuring the backreferences to be displayed automatically generated APIs

@Titan-C Titan-C modified the milestones: Release v0.1.8, Release V0.1.9 Jan 3, 2017

@larsoner

When building MNE docs (without making any changes to it) on this branch, I get an error:

...
Generating gallery
/home/larsoner/custombuilds/sphinx-gallery/sphinx_gallery/gen_gallery.py:91: UserWarning: 

=========
Sphinx Gallery now requires you to set the configuration variable
'backreferences_dir' in your config to activate the
backreferences. Read how to update in the online documentation

http://sphinx-gallery.readthedocs.io/en/latest/advanced_configuration.html#references-to-examples
  warnings.warn(backreferences_warning)
/home/larsoner/custombuilds/sphinx-gallery/sphinx_gallery/gen_gallery.py:95: UserWarning: 
 using old default 'backreferences_dir':'modules/generated'
  gallery_conf['backreferences_dir']))

Exception occurred:
  File "/home/larsoner/custombuilds/sphinx-gallery/sphinx_gallery/gen_gallery.py", line 99, in parse_config
    + backreferences_warning)
ValueError: Old configuration for backreferences detected 
using the configuration variable `mod_example_dir`


=========
Sphinx Gallery now requires you to set the configuration variable
'backreferences_dir' in your config to activate the
backreferences. Read how to update in the online documentation

http://sphinx-gallery.readthedocs.io/en/latest/advanced_configuration.html#references-to-examples
The full traceback has been saved in /tmp/sphinx-err-S6YABC.log, if you want to report the issue to the developers.

And the log basically just has the ValueError traceback. Is this intentional? I expected a warning, but successful build.

like this:
Sphinx-Gallery enables you, when documenting your modules, to
reference to the examples that use a particular function. For example
if we are documenting the ``numpy.exp`` function its possible to embed

This comment has been minimized.

@larsoner

larsoner Jan 4, 2017

Contributor
:func:`numpy.exp`

This should work with sphinx crossref

This comment has been minimized.

@larsoner

larsoner Mar 27, 2017

Contributor

Not addressed

@larsoner

This comment has been minimized.

Contributor

larsoner commented Jan 4, 2017

I can confirm that this change gets everything working for MNE:

-    'mod_example_dir': 'generated',
+    'backreferences_dir': 'generated',

And the changes make sense to me.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Jan 4, 2017

Also, having both config values (old and new) I get the same error:

Exception occurred:
  File "/home/larsoner/custombuilds/sphinx-gallery/sphinx_gallery/gen_gallery.py", line 99, in parse_config
    + backreferences_warning)
ValueError: Old configuration for backreferences detected 
using the configuration variable `mod_example_dir`

If both are present, I think the new one should be used and no warning issued. That way I can just update MNE to have redundant old and new keys, it will work for people with old and new sphinx_gallery installs, and somewhere down the line I'll clean up the conf.py.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jan 6, 2017

And the log basically just has the ValueError traceback. Is this intentional? I expected a warning, but successful build.

I seem to remember a discussion about this but I can't find it. IMHO warnings are easily ignored especially in the middle of a noisy output like the one sphinx + sphinx-gallery produces. In principle, I would rather have an error saying that something is very likely configured wrong somewhere and please read the doc or remove this line. What I suspect is that nobody has this setting right, but (lucky) people using exactly the same setup as scikit-learn and @Eric89GXL who asked about the warnings he was getting and thanks to @Titan-C managed to set everything correctly.

Having said that, if we think it is better to warn for a few releases and then have an error I am fine with this as well.

Most importantly what I think is important is that the warning/error message clearly states:

  • the feature that is in play here: mostly having mini galleries in the reference API. A link to how it looks in sphinx-gallery is needed because I don't think explaining it with words is good enough.
  • if you don't care about this feature, do this (I am guessing removing one line in conf.py is enough?).
  • If you care about the feature and want a quick fix, try to replace 'mod_example_dir' by 'backreferences_dir'
  • If the quick fix does not produce the mini-galleries then go and read very carefully this section of the doc.

If both are present, I think the new one should be used and no warning issued. That way I can just update MNE to have redundant old and new keys, it will work for people with old and new sphinx_gallery installs, and somewhere down the line I'll clean up the conf.py.

From this I am guessing that you don't have sphinx-gallery bundled in externals (the latter is what most projects do AFAIK). If that is the case would you consider requiring a recent enough version of sphinx_gallery in your conf.py (once sphinx-gallery 0.1.8 is released) ?

@larsoner

This comment has been minimized.

Contributor

larsoner commented Jan 6, 2017

From this I am guessing that you don't have sphinx-gallery bundled in externals (the latter is what most projects do AFAIK). If that is the case would you consider requiring a recent enough version of sphinx_gallery in your conf.py (once sphinx-gallery 0.1.8 is released) ?

We could do that, sure. Allowing the old value to seems like a reasonable low-cost way to maintain better backward compatibility/transition with this change, but it probably (hopefully) won't affect too many people anyway.

@Titan-C Titan-C force-pushed the Titan-C:doc_autoapi branch from d70079d to 752e79c Jan 16, 2017

@Titan-C Titan-C referenced this pull request Jan 17, 2017

Merged

[MRG] Better path handling #190

Titan-C and others added some commits Sep 24, 2016

[DOC] Organization
- Fixes to Sphinx Headers

Apparently Sphinx does not like the overline for the headers. Especially
when using the auto docs. Also it does not look good in the Jupyter
notebooks. Removing the overline does not have any negative effect on
the html heading.

- Fixes to level documentation
Use backreferences_dir variable
Deprecationwarning on mod_examples_dir new of default
[DOC] including example backreferences
Default to None example back-references

Move examples backreferences sources

@Titan-C Titan-C force-pushed the Titan-C:doc_autoapi branch from 752e79c to e9107ac Mar 27, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented Mar 27, 2017

reviewed and rebased

@larsoner

Okay, on MNE (working with master) on this PR I get:

Generating gallery
/Users/larsoner/python/sphinx-gallery/sphinx_gallery/gen_gallery.py:99: UserWarning: 

=========
Sphinx Gallery now requires you to set the configuration variable
'backreferences_dir' in your config to activate the
backreferences. That is mini galleries clustered by the functions used
in the example script. Have a look at it in sphinx-gallery

http://sphinx-gallery.readthedocs.io/en/stable/index.html#examples-using-numpy-linspace



If you don't care about this features set
            in your conf.py
 'backreferences_dir':
                False


  warnings.warn(backreferences_warning + no_care_msg)
/Users/larsoner/python/sphinx-gallery/sphinx_gallery/gen_gallery.py:103: UserWarning: 
 using old default 'backreferences_dir':'modules/generated'. This will be disabled in future releases



  gallery_conf['backreferences_dir']))

Exception occurred:
  File "/Users/larsoner/python/sphinx-gallery/sphinx_gallery/gen_gallery.py", line 113, in parse_config
    + backreferences_warning)
ValueError: Old configuration for backreferences detected 
using the configuration variable `mod_example_dir`


=========
Sphinx Gallery now requires you to set the configuration variable
'backreferences_dir' in your config to activate the
backreferences. That is mini galleries clustered by the functions used
in the example script. Have a look at it in sphinx-gallery

http://sphinx-gallery.readthedocs.io/en/stable/index.html#examples-using-numpy-linspace
The full traceback has been saved in /var/folders/76/jtmt9q4j18b96prw86rb_17c0000gn/T/sphinx-err-enD3sE.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make: *** [html_dev-noplot] Error 1

If I add both config entries, it still doesn't work:

Generating gallery

Exception occurred:
  File "/Users/larsoner/python/sphinx-gallery/sphinx_gallery/gen_gallery.py", line 113, in parse_config
    + backreferences_warning)
ValueError: Old configuration for backreferences detected 
using the configuration variable `mod_example_dir`


=========
Sphinx Gallery now requires you to set the configuration variable
'backreferences_dir' in your config to activate the
backreferences. That is mini galleries clustered by the functions used
in the example script. Have a look at it in sphinx-gallery

http://sphinx-gallery.readthedocs.io/en/stable/index.html#examples-using-numpy-linspace
The full traceback has been saved in /var/folders/76/jtmt9q4j18b96prw86rb_17c0000gn/T/sphinx-err-KyRh7H.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
make: *** [html_dev-noplot] Error 1

It would be nice if one or both of these worked so that users with legacy sphinx_gallery aren't stuck.

like this:
Sphinx-Gallery enables you, when documenting your modules, to
reference to the examples that use a particular function. For example
if we are documenting the ``numpy.exp`` function its possible to embed

This comment has been minimized.

@larsoner

larsoner Mar 27, 2017

Contributor

Not addressed

@@ -173,20 +182,48 @@ it with the standard sphinx extensions `autodoc
...
'sphinx.ext.autodoc',
'sphinx.ext.autosummary',
'sphinx_gallery.gen_gallery',

This comment has been minimized.

@larsoner

larsoner Mar 27, 2017

Contributor

the line above (184) is not lined up properly

Backreferences Generator
========================
Reviews generated example files in order to keep track of used modules
Parses example files code in order to keep track of used functions

This comment has been minimized.

@larsoner

larsoner Mar 27, 2017

Contributor

"example file code" reads better

print('Generating gallery')
def parse_config(app):
"""Process the Sphinx Gallery configuration"""
# TODO: Test this behavior.

This comment has been minimized.

@larsoner

larsoner Mar 27, 2017

Contributor

Do you want to add such tests as part of this PR, or save for later?

This comment has been minimized.

@Titan-C

Titan-C Apr 1, 2017

Member

This goes for the future

This comment has been minimized.

@lesteve

lesteve Apr 3, 2017

Contributor

@Titan-C sorry I don't have time to review this PR in detail so feel free to merge it.

It does not have to happen in this PR but tests for the both old and new configuration are a must. It is very easy to break this kind of things without realizing and the impact on the user that encounters the regression is very bad IMO.

Titan-C added some commits Apr 1, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented Apr 1, 2017

The backreferences configurations will now only issue warnings. Old and new configurations should work.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Apr 2, 2017

Now with both config entries present, I get a warning about using the new config value, which we can live with for a while to get people migrated to the newer version.

LGTM, thanks @Titan-C

@Titan-C Titan-C merged commit dd661dd into sphinx-gallery:master Apr 9, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

lheagy added a commit to simpeg/simpeg that referenced this pull request Apr 21, 2017

@Titan-C Titan-C deleted the Titan-C:doc_autoapi branch May 14, 2017

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