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+1: Use intersphinx #296

Merged
merged 7 commits into from Oct 4, 2017

Conversation

Projects
None yet
7 participants
@larsoner
Contributor

larsoner commented Oct 2, 2017

Closes #287.

Simplifies link resolution by using intersphinx.

I'm not sure the best way to test this. I was doing it locally by having a small testbuild directory with a conf.py, index.rst, examples/plot_numpy_scipy.py. I could turn this into a test if it seems useful. Would the best way be to directly include the files, copy them to a tempdir, call Sphinx(...), and check the output links to ensure they contain links to numpy/scipy/matplotlib docs, as they should?

:no-members:
:no-inherited-members:
:py:mod:`sphinx_gallery`:

This comment has been minimized.

@larsoner

larsoner Oct 2, 2017

Contributor

This allows other projects to link to the root module, which is nice, as:

:mod:`sphinx_gallery`
@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 2, 2017

Updated docs and example.

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Oct 2, 2017

Trying to build scipy-lectures with this PR, I get the following traceback:

# Sphinx version: 1.4.8
# Python version: 2.7.12+ (CPython)
# Docutils version: 0.12 release
# Jinja2 version: 2.8
# Last messages:
#   copying static files...
#   done
#   copying extra files...
#   done
#   dumping search index in English (code: en) ...
#   done
#   dumping object inventory...
#   done
#   build succeeded, 72 warnings.
#   embedding documentation hyperlinks in examples...
# Loaded extensions:
#   ipython_console_highlighting (unknown version) from /home/varoquau/dev/scipy-lecture-notes/sphinxext/ipython_console_highlighting.pyc
#   sphinx.ext.intersphinx (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/intersphinx.pyc
#   sphinx.ext.doctest (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/doctest.pyc
#   only_directives (unknown version) from /home/varoquau/dev/scipy-lecture-notes/sphinxext/only_directives.pyc
#   sphinx.ext.extlinks (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/extlinks.pyc
#   sphinx_gallery.gen_gallery (unknown version) from /home/varoquau/dev/sphinx-gallery/sphinx_gallery/gen_gallery.pyc
#   sphinx.ext.autodoc (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/autodoc.pyc
#   alabaster (0.7.9) from /home/varoquau/.local/lib/python2.7/site-packages/alabaster/__init__.pyc
#   sphinx.ext.imgmath (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/imgmath.pyc
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/sphinx/cmdline.py", line 244, in main
    app.build(opts.force_all, filenames)
  File "/usr/lib/python2.7/dist-packages/sphinx/application.py", line 315, in build
    self.emit('build-finished', None)
  File "/usr/lib/python2.7/dist-packages/sphinx/application.py", line 551, in emit
    results.append(callback(self, *args))
  File "/home/varoquau/dev/sphinx-gallery/sphinx_gallery/docs_resolv.py", line 383, in embed_code_links
    _embed_code_links(app, gallery_conf, gallery_dir)
  File "/home/varoquau/dev/sphinx-gallery/sphinx_gallery/docs_resolv.py", line 266, in _embed_code_links
    src_gallery_dir)
  File "/home/varoquau/dev/sphinx-gallery/sphinx_gallery/docs_resolv.py", line 166, in __init__
    index = get_data(index_url, gallery_dir)
  File "/home/varoquau/dev/sphinx-gallery/sphinx_gallery/docs_resolv.py", line 73, in get_data
    data = _get_data(url)
  File "/home/varoquau/dev/sphinx-gallery/sphinx_gallery/docs_resolv.py", line 50, in _get_data
    data = StringIO(data)
TypeError: initial_value must be unicode or None, not str

@larsoner larsoner force-pushed the larsoner:intersphinx branch from 3f83552 to b86fe6e Oct 2, 2017

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 2, 2017

That is a bug already fixed by #293. I've rebased this PR to latest master, feel free to try again.

@@ -305,6 +305,7 @@ def setup(app):
'matplotlib': ('https://matplotlib.org/', None),
'mayavi': ('http://docs.enthought.com/mayavi/mayavi', None),
'sklearn': ('http://scikit-learn.org/stable', None),
'sphinx': ('http://www.sphinx-doc.org/en/stable', None),

This comment has been minimized.

@choldgraf

choldgraf Oct 3, 2017

Contributor

could we add a comment above here that says something to the effect of "If you have intersphinx enabled, these lines are unnecessary"?

This comment has been minimized.

@larsoner

larsoner Oct 3, 2017

Contributor

This is the standard intersphinx_mapping dict. I've already removed the unnecessary lines below

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Oct 3, 2017

LGTM with one comment

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Oct 3, 2017

On a clean build of scipy-lectures, using this PR, I get the following crasher:

# Sphinx version: 1.4.8
# Python version: 2.7.12+ (CPython)
# Docutils version: 0.12 release
# Jinja2 version: 2.8
# Last messages:
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [ 20%] plot_airfare.html
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [ 30%] plot_iris_analysis.html
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [ 40%] plot_paired_boxplots.html
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [ 50%] plot_pandas.html
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [ 60%] plot_regression.html
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [ 70%] plot_regression_3d.html
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [ 80%] plot_wage_data.html
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [ 90%] plot_wage_education_gender.html
#   embedding documentation hyperlinks for packages/statistics/auto_examples... [100%] plot_brain_size.html
#   
# Loaded extensions:
#   ipython_console_highlighting (unknown version) from /home/varoquau/dev/scipy-lecture-notes/sphinxext/ipython_console_highlighting.pyc
#   sphinx.ext.intersphinx (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/intersphinx.pyc
#   sphinx.ext.doctest (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/doctest.pyc
#   only_directives (unknown version) from /home/varoquau/dev/scipy-lecture-notes/sphinxext/only_directives.pyc
#   sphinx.ext.extlinks (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/extlinks.pyc
#   sphinx_gallery.gen_gallery (0.1.12) from /home/varoquau/dev/sphinx-gallery/sphinx_gallery/gen_gallery.pyc
#   sphinx.ext.autodoc (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/autodoc.pyc
#   alabaster (0.7.9) from /home/varoquau/.local/lib/python2.7/site-packages/alabaster/__init__.pyc
#   sphinx.ext.imgmath (1.4.8) from /usr/lib/python2.7/dist-packages/sphinx/ext/imgmath.pyc
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/sphinx/cmdline.py", line 244, in main
    app.build(opts.force_all, filenames)
  File "/usr/lib/python2.7/dist-packages/sphinx/application.py", line 315, in build
    self.emit('build-finished', None)
  File "/usr/lib/python2.7/dist-packages/sphinx/application.py", line 551, in emit
    results.append(callback(self, *args))
  File "/home/varoquau/dev/sphinx-gallery/sphinx_gallery/docs_resolv.py", line 392, in embed_code_links
    _embed_code_links(app, gallery_conf, gallery_dir)
  File "/home/varoquau/dev/sphinx-gallery/sphinx_gallery/docs_resolv.py", line 264, in _embed_code_links
    url, src_gallery_dir)
  File "/home/varoquau/dev/sphinx-gallery/sphinx_gallery/docs_resolv.py", line 171, in __init__
    self._searchindex = js_index.loads(sindex)
  File "/usr/lib/python2.7/dist-packages/sphinx/search/__init__.py", line 146, in loads
    return jsdump.loads(data)
  File "/usr/lib/python2.7/dist-packages/sphinx/util/jsdump.py", line 187, in loads
    raise ValueError("read error at pos %d" % i)
ValueError: read error at pos 113500
@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

It looks unrelated to this PR. Does it pass on master?

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Oct 3, 2017

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

Get rid of the reference_url entry in sphinx_gallery_conf

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

(all of them, in this case)

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

... in other words, the standard intersphinx_mapping, which you appear to have defined properly already, should be sufficient. I still let reference_urls take precedence for reasons of backward compatibility.

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Oct 3, 2017

Indeed, of course; that works fine. Thank you!

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Oct 3, 2017

👍 for merge

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 3, 2017

I'm not sure the best way to test this. I was doing it locally by having a small testbuild directory with a conf.py, index.rst, examples/plot_numpy_scipy.py. I could turn this into a test if it seems useful.

Being able to quickly set-up a toy gallery, run sphinx on it and check the output programatically would be really great. Historically for a few things, we have been relying on doing make html and checking manually. @Titan-C can correct me if I am wrong, maybe he has already added tests that were similar to what I describe.

@Titan-C

This comment has been minimized.

Member

Titan-C commented Oct 3, 2017

@Titan-C

LGTM.
I would try to have a reference test on this, @larsoner could you copy your testbuild directory at least with a readme reference. Or much better integrate the test to our pytest tests.
Finally a line on CHANGES.rst to say we directly use intersphinx sources for the source code function references is needed

as the ``intersphinx`` inventory will automatically be used.
If you do not use ``intersphinx``, then you should add entries that
point to the directory containing ``searchindex.js``, such as
``'matplotlib': 'https://matplotlib.org'``.

This comment has been minimized.

@Titan-C

Titan-C Oct 3, 2017

Member

I will suggest that we do include a code example on this configuration style without intersphinx. That is just keep the old version of the configuration code example.

This comment has been minimized.

@larsoner

larsoner Oct 3, 2017

Contributor

Removing the code sample is designed to help push people to really use intersphinx where possible. Advanced users and backward compat still allow doing otherwise, but I don't think we should advise people to do it. sphinx.ext.intersphinx is much better tested, has more functionality beyond just SG, and our current re-implementation of this functionality is a bit broken (see @GaelVaroquaux's comments on this PR and the related issue).

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

Or much better integrate the test to our pytest tests.

This should be pretty straightforward with the Sphinx app. I'll give it a shot.

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Oct 3, 2017

@larsoner larsoner force-pushed the larsoner:intersphinx branch 3 times, most recently from 867084a to a4f406d Oct 3, 2017

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

Added the tinybuild with pytest unit tests, CircleCI rendering, and easy testing when running make within the tinybuild directory. I also added an auto-removing _TempDir() function ported from MNE (we've used it for years) to avoid filling up people's /tmp.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

Travis is getting this error, anybody see a reason why? I don't get it locally:

==================================== ERRORS ====================================
______________________________ ERROR collecting  _______________________________
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/py/_path/common.py:372: in visit
    for x in Visitor(fil, rec, ignore, bf, sort).gen(self):
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/py/_path/common.py:421: in gen
    for p in self.gen(subdir):
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/py/_path/common.py:421: in gen
    for p in self.gen(subdir):
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/py/_path/common.py:421: in gen
    for p in self.gen(subdir):
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/py/_path/common.py:411: in gen
    if p.check(dir=1) and (rec is None or rec(p))])
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/_pytest/main.py:728: in _recurse
    ihook = self.gethookproxy(path)
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/_pytest/main.py:632: in gethookproxy
    my_conftestmodules = pm._getconftestmodules(fspath)
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/_pytest/config.py:356: in _getconftestmodules
    mod = self._importconftest(conftestpath)
../../../miniconda2/envs/testenv/lib/python2.7/site-packages/_pytest/config.py:381: in _importconftest
    raise ConftestImportFailure(conftestpath, sys.exc_info())
E   ConftestImportFailure: ImportMismatchError('sphinx_gallery.tests.conftest', '/home/travis/build/sphinx-gallery/sphinx-gallery/sphinx_gallery/tests/conftest.py', local('/home/travis/build/sphinx-gallery/sphinx-gallery/build/lib/sphinx_gallery/tests/conftest.py'))
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!
=========================== 1 error in 4.35 seconds ============================
The command "python setup.py test" exited with 2.

@larsoner larsoner force-pushed the larsoner:intersphinx branch from a963a80 to c6b4d49 Oct 3, 2017

@larsoner larsoner force-pushed the larsoner:intersphinx branch 2 times, most recently from 6060949 to f790605 Oct 3, 2017

@larsoner larsoner force-pushed the larsoner:intersphinx branch from f790605 to 77d374b Oct 3, 2017

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

@GaelVaroquaux:

  • sudo apt-get install texlive texlive-latex-extras latexmk takes 1 minute
  • make latexpdf on tinybuild takes 7 seconds and renders like this.

Total build time is 7 minutes, so it's not a trivial addition, but still pretty quick, so it seems worth it to me.

@larsoner larsoner force-pushed the larsoner:intersphinx branch from 77d374b to 0511d15 Oct 3, 2017

@larsoner larsoner changed the title from ENH: Use intersphinx to MRG: Use intersphinx Oct 3, 2017

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 3, 2017

=========================== 1 error in 4.35 seconds ============================
The command "python setup.py test" exited with 2.

Are we really running "python setup.py test" I always wondered why people do that instead of just using pytest directly.

I saw ImportMismatchError at one point in scikit-learn and I did not understand why they happened. In my case that was about testing doctests in rst files, and I worked around the problem by specifying a list of rst files to test rather than trying to use pytest automatic discovery, e.g. pytest $(find doc -name '*.rst') rather than pytest doc. Not sure whether that helps ...

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

I think I figured it out -- I added an exclude to setup.cfg that overrode the default. This made it include the build directory. Now that I'm excluding it, tests should pass again. (They pass locally when I reproduced the python setup.py install && python setup.py test pattern.)

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 3, 2017

Okay CIs are happy, ready for review/merge from my end.

@larsoner larsoner changed the title from MRG: Use intersphinx to MRG+1: Use intersphinx Oct 4, 2017

@larsoner

This comment has been minimized.

Contributor

larsoner commented Oct 4, 2017

I think @GaelVaroquaux gave a +1, @choldgraf also good to go from your end?

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Oct 4, 2017

@choldgraf choldgraf merged commit c212bb5 into sphinx-gallery:master Oct 4, 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
@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Oct 4, 2017

thx @larsoner !

from shutil import rmtree
class _TempDir(str):

This comment has been minimized.

@QuLogic

QuLogic Oct 4, 2017

Contributor

If this is to be used for tests, you could use the tmpdir fixture.

This comment has been minimized.

@larsoner

larsoner Oct 4, 2017

Contributor

Neat, didn't know about that

@larsoner larsoner deleted the larsoner:intersphinx branch Oct 4, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented on 5a33a72 Oct 8, 2017

@larsoner There is currently something wrong with the latex install in circle CI. a library is missing at install time. We might need to update the packages list before installing.

This comment has been minimized.

Contributor

larsoner replied Oct 9, 2017

Yeah probably. The weird thing is that my PR passed, but master appears not to.

@@ -330,9 +331,6 @@ def setup(app):
'doc_module': ('sphinx_gallery', 'numpy'),
'reference_url': {
'sphinx_gallery': None,

This comment has been minimized.

@lesteve

lesteve Oct 11, 2017

Contributor

The sphinx_gallery looks a bit akward on its own, is it really needed and is there a way we can get rid of reference_url all together and only use intersphinx?

This comment has been minimized.

@Titan-C

Titan-C Oct 11, 2017

Member

Indeed, this becomes redundant with intersphinx available. I don't even think we need it. The None is there to tell sphinx to find the docs database locally. But we can have that done at execution time.

This comment has been minimized.

@larsoner

larsoner Oct 11, 2017

Contributor

Ahh I see what you're saying, I can make a quick PR for that.

This comment has been minimized.

@larsoner

larsoner Oct 11, 2017

Contributor

... but you still need to at least get this_module name from somewhere, otherwise I'm not sure how else you'd set the doc_resolvers[this_module] = .... Without that, we don't know if we should look locally or to use intersphinx.

I don't see the module name in the config anywhere else. Storing it as a dict instead of a single string entry now is a bit silly, as is the name reference_url instead of something like root_module or something, but I don't mind the current version too much.

This comment has been minimized.

@Titan-C

Titan-C Oct 11, 2017

Member

I don't consider this very urgent. There is indeed some redundancy. And having now a dictionary for a single entry is no too elegant.
My idea is that one usually documents one local module and that module is in the "doc_module" value. Since there isn't now any need of specifying all other "reference_url" for external modules, as intersphinx take care of it. We could iterate the values of "doc_module" to see which modules we should try to resolve locally.
I don't know what will happen in the case of sphinx-gallery that documents numpy too track the use of it in the code examples and generate the mini-galleries. I know is not related, but using the values of doc_module intersphinx will try to search for numpy locally and fail, and then totally independent, the value of interphinx sources in conf.py will take over?

@weber-s

This comment has been minimized.

weber-s commented Apr 30, 2018

Hi,

I spotted something strange in my docs. I try to use intersphinx together with SG, but it does not work unless I add 'backreferences_dir': 'gen_modules/backreferences' to SG options. Is it the correct behaviour (sorry I'm new to sphinx world)? If so, it should be documented.

Thanks for this great job!
GwendalD

@Titan-C

This comment has been minimized.

Member

Titan-C commented Apr 30, 2018

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