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: Error on sphinx 1.5 #178

Merged
merged 11 commits into from Dec 9, 2016

Conversation

Projects
None yet
3 participants
@larsoner
Contributor

larsoner commented Dec 9, 2016

Starting over on #137.

@larsoner larsoner force-pushed the larsoner:sphinx-file branch from 65c1a11 to c1f4a64 Dec 9, 2016

larsoner added some commits Dec 9, 2016

@larsoner larsoner force-pushed the larsoner:sphinx-file branch from 93edfa4 to acc7c1a Dec 9, 2016

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 9, 2016

We know sphinx-gallery tests are not passing on Windows ... we have an issue #162 and an ongoing PR. Maybe do not spend too much time on getting the tests to pass on Windows.

I have heard of the astropy CI stuff but never looked into it in detail, what do they give us exactly?

@lesteve lesteve changed the title from FIX: Error on new sphinx to FIX: Error on sphinx 1.5 Dec 9, 2016

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

I removed the AppVeyor stuff, don't worry. PR forthcoming. CI-helpers simplify Anaconda installation, I'll show you shortly :)

In the meantime this should be good to go.

@larsoner larsoner changed the title from FIX: Error on sphinx 1.5 to MRG: Error on sphinx 1.5 Dec 9, 2016

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 9, 2016

Hmm I do not get any errors on your PR with sphinx 1.5 but I get plenty of "HTTPError 404" printout like this:

Embedding documentation hyperlinks in examples..
        processing: plot_function_identifier.html
        processing: just_code.html
        processing: index.html
        processing: plot_strings.html
        processing: plot_gallery_version.html
The following error has occurred:

<HTTPError 404: 'Not Found'>
The following error has occurred:

<HTTPError 404: 'Not Found'>
The following error has occurred:

<HTTPError 404: 'Not Found'>
The following error has occurred:

<HTTPError 404: 'Not Found'>
        processing: plot_choose_thumbnail.html
The following error has occurred:

<HTTPError 404: 'Not Found'>
The following error has occurred:

<HTTPError 404: 'Not Found'>
        processing: plot_colors.html
The following error has occurred:

<HTTPError 404: 'Not Found'>
The following error has occurred:

<HTTPError 404: 'Not Found'>
The following error has occurred:

<HTTPError 404: 'Not Found'>
        processing: plot_raise.html
The following error has occurred:

<HTTPError 404: 'Not Found'>
        processing: plot_exp.html
The following error has occurred:

<HTTPError 404: 'Not Found'>
The following error has occurred:

<HTTPError 404: 'Not Found'>
        processing: plot_quantum.html
The following error has occurred:

You can see them in the Travis log actually:
https://travis-ci.org/sphinx-gallery/sphinx-gallery/jobs/182623353#L735

I am wondering where they are coming from ...

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

I see those, too. I've been using the equivalent of this branch for a while and haven't noticed any broken links or anything... Does it only happen for you on this branch? Maybe I need to dig into Sphinx to figure out where it's coming from and have it throw a more explicit error.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 9, 2016

Does it only happen for you on this branch?

I guess this is the only branch I have which works with sphinx 1.5 ;-) so it is hard to know whether sphinx 1.5 or this PR is the culprit for these noisy HTTPError messages.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

Looks like it's coming from docs_resolv.py around 374:

                    try:
                        link = doc_resolvers[this_module].resolve(cobj,
                                                                  full_fname)
                    except (HTTPError, URLError) as e:
                        print("The following error has occurred:\n")
                        print(repr(e))
                        continue
@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

More explicit error generation in latest commit:

Error in /home/travis/build/sphinx-gallery/sphinx-gallery/doc/_build/html/tutorials/plot_notebook.html for module numpy ({'module_short': 'numpy', 'name': 'linspace', 'module': 'numpy'}):

@larsoner larsoner force-pushed the larsoner:sphinx-file branch 3 times, most recently from fa5c9b2 to 23a91ea Dec 9, 2016

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

... so the underlying problem is probably in SphinxDocLinkResolver somewhere.

@larsoner larsoner referenced this pull request Dec 9, 2016

Merged

[MRG+1]: Fixes for Windows #179

@larsoner larsoner force-pushed the larsoner:sphinx-file branch from 23a91ea to 360b581 Dec 9, 2016

.travis.yml Outdated
@@ -45,7 +45,7 @@ install:
# force no mkl because mayavi requires old version of numpy
# which then crashes with pandas and seaborn
- if [ "$DISTRIB" == "conda" ]; then
conda create --yes -n testenv python=$PYTHON_VERSION pip nomkl numpy
conda create --yes -n testenv python=$PYTHON_VERSION pip nomkl numpy scipy

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 9, 2016

Contributor

I am puzzled: why was adding scipy necessary for this fix?

This comment has been minimized.

@larsoner

larsoner Dec 9, 2016

Contributor

There were some seaborn errors without it I thought

@larsoner larsoner force-pushed the larsoner:sphinx-file branch from 360b581 to f6e5fd8 Dec 9, 2016

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

The problem seems to be that SphinxDocLinkResolver tries to get the link to e.g. http://docs.scipy.org/doc/numpy-1.9.1/reference/generated/numpy.html but it doesn't exist. Now it gives a more informative error:

https://travis-ci.org/sphinx-gallery/sphinx-gallery/jobs/182643937#L1139

In the meantime, this gets Travis passing and SG at least more usable. I'm not sure how to fix the underlying SphinxDocLinkResolver issue. Looks like that's code by @Titan-C?

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Dec 9, 2016

@Eric89GXL : I do note that this gets travis passing, and I didn't see anything that looked wrong to me, so overall I am 👍 for the merge.

But still, I don't get why we need scipy on the CI. Maybe you answered this question, but I didn't understand the answer, then.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 9, 2016

But still, I don't get why we need scipy on the CI. Maybe you answered this question, but I didn't understand the answer, then.

We install seaborn so getting pandas and scipy automatically via the dependency resolver anyway.

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Dec 9, 2016

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

I added a new commit to removes the SciPy change, and it works fine. I thought it threw an error at one point but I must have misread it!

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

(Also FWIW python-scipy is installed in the Ubuntu distro version, so adding it to the conda version is a little bit more explicit.) Ready for merge from my end anyway. Should I open an issue about the unresolved links?

fname = self._searchindex['filenames'][fname_idx]
try:
import sphinx
except Exception:

This comment has been minimized.

@lesteve

lesteve Dec 9, 2016

Contributor

Why would the sphinx import fail?

This comment has been minimized.

@larsoner

larsoner Dec 9, 2016

Contributor

It looked like nowhere in the package did sphinx actually get imported, so I didn't want to add a top-level import. But since this is sphinx-gallery I guess it's okay, eh? :)

except Exception:
fname = os.path.splitext(fname)[0]
else:
if LooseVersion(sphinx.__version__) >= LooseVersion('1.5'):

This comment has been minimized.

@lesteve

lesteve Dec 9, 2016

Contributor

Bonus points if you add a comment to clarify why the work-around for 1.5 is needed.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

@lesteve done

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

Oh wait, I realized the problem with the link resolution. We will have an issue with building docs with 1.5+ but linking to older doc versions. I can push a fix.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

I don't love this fix, but it should work. Eventually once more pages start using 1.5+ we'll end up with a lot of errors being shown for links that couldn't be grabbed even though they work on the second iteration of the for fname in fnames loop...

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Dec 9, 2016

Please open an issue for the unresolved links (or check that there is not one already). They have been annoying me too.

OK, I was about to merge this PR, but it seems that you're on something, so I'll hold back a wee bit.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

Okay @GaelVaroquaux feel free to merge if you're happy with the try/except loop that I made. It's not pretty but it seems to work on both Ubuntu which uses < 1.5 and Anaconda which uses >= 1.5.

import gzip
import os
import posixpath
import re
import shelve
import sys
import sphinx

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 9, 2016

Contributor

Are the sphinx import and the LooseVesion import still useful?

else:
html = get_data(link, self.gallery_dir)
self._page_cache[link] = html
raise e

This comment has been minimized.

@lesteve

lesteve Dec 9, 2016

Contributor

Would raise not be work to raise the last caught exception? My experience is that raise e creates a misleading traceback especially on Python 2.7 since you lose the provenance of the original exception.

else:
extra = e.reason
print("\t\tError resolving %s.%s: %r (%s)"
% (cobj['module'], cobj['name'], e, extra))

This comment has been minimized.

@lesteve

lesteve Dec 9, 2016

Contributor

For another PR, but it does seem that it would be useful to have the URL that we tried to access, rather than the module ...

This comment has been minimized.

@larsoner

larsoner Dec 9, 2016

Contributor

Yeah I agree, but that will require a good bit of refactoring because the URL is a function or two deeper than this. And really it should be refactored to use the proper local filename based on sphinx version, and then check the two possible remote names, rather than trying both in both cases as it does now. But I think that will again require a lot of refactoring to work.

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

Unnecessary imports removed.

else:
html = get_data(link, self.gallery_dir)
self._page_cache[link] = html
except Exception:

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux Dec 9, 2016

Contributor

Do you really want to catch all the exceptions? Can't we be slightly more specific?

Sorry for making a new comment at each round, I am seeing these aspects only gradually.

This comment has been minimized.

@larsoner

larsoner Dec 9, 2016

Contributor

It's okay, this bit is new :)

Yes I can catch just HTTPError and URLError

@larsoner larsoner force-pushed the larsoner:sphinx-file branch from 40c6196 to b05ddca Dec 9, 2016

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 9, 2016

I think the fix is fine, you could tidy it up by putting the for loop in a function but we can do that later.

+1 for merging, thanks a lot @Eric89GXL.

I wish I would have paid more attention when #137 was created, I have to admit I have no recollection of it at all :( ...

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 9, 2016

I think the fix is fine, you could tidy it up by putting the for loop in a function but we can do that later.

And the warnings about not being able to access the URL would make sense inside this function as well, since at this point you know the two URLs you tried to access (rather than catching the exception later and having only one URL).

@larsoner

This comment has been minimized.

Contributor

larsoner commented Dec 9, 2016

It's alright, I think Anaconda just rolled out 1.5 so I doubt many people have been hit by it. I also probably should have followed up once the issue didn't magically disappear like I was hoping it would...

Exception made less broad, Travis builds look good (green and logs have no complaints).

@lesteve

This comment has been minimized.

Contributor

lesteve commented Dec 9, 2016

OK I am going to merge this one, thanks a lot !

@lesteve lesteve merged commit 81b6502 into sphinx-gallery:master Dec 9, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@larsoner larsoner deleted the larsoner:sphinx-file branch Dec 9, 2016

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