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

[MRG] Documentation Auto API #151

Merged
merged 10 commits into from
Apr 9, 2017
Merged

Conversation

Titan-C
Copy link
Member

@Titan-C 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
Copy link
Contributor

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
Copy link
Member Author

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).

@lesteve
Copy link
Member

lesteve commented Dec 12, 2016

Some conflicts to fix here.

@Titan-C Titan-C force-pushed the doc_autoapi branch 5 times, most recently from a1b578b to 60c6f51 Compare December 17, 2016 19:02
@Titan-C
Copy link
Member Author

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 doc_autoapi branch 2 times, most recently from 4c8dc4a to bf0b830 Compare December 23, 2016 13:58
@Titan-C
Copy link
Member Author

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
Copy link
Contributor

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

:func:`numpy.exp`

This should work with sphinx crossref

Copy link
Contributor

Choose a reason for hiding this comment

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

Not addressed

@larsoner
Copy link
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
Copy link
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
Copy link
Member

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
Copy link
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 and others added 5 commits March 27, 2017 06:58
Deprecationwarning on mod_examples_dir new of default
- 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
Default to None example back-references

Move examples backreferences sources
@Titan-C
Copy link
Member Author

Titan-C commented Mar 27, 2017

reviewed and rebased

Copy link
Contributor

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Not addressed

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

"example file code" reads better

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This goes for the future

Copy link
Member

Choose a reason for hiding this comment

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

@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
Copy link
Member Author

Titan-C commented Apr 1, 2017

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

@larsoner
Copy link
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
@Titan-C Titan-C deleted the doc_autoapi branch May 14, 2017 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants