MRG: Error on sphinx 1.5 #178

larsoner · 2016-12-09T15:23:54Z

Starting over on #137.

lesteve · 2016-12-09T15:46:34Z

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?

larsoner · 2016-12-09T15:54:24Z

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.

lesteve · 2016-12-09T16:04:42Z

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 · 2016-12-09T16:06:08Z

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 · 2016-12-09T16:28:38Z

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 · 2016-12-09T16:31:18Z

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 · 2016-12-09T16:35:57Z

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 · 2016-12-09T16:41:03Z

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

GaelVaroquaux · 2016-12-09T16:48:29Z

.travis.yml

    # 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


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

There were some seaborn errors without it I thought

larsoner · 2016-12-09T16:58:28Z

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 · 2016-12-09T17:08:30Z

@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 · 2016-12-09T17:10:20Z

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 · 2016-12-09T17:11:19Z

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

OK. Good point. Still puzzled but it's not really important. +1 from my side.

larsoner · 2016-12-09T17:13:01Z

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 · 2016-12-09T17:14:22Z

(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?

lesteve · 2016-12-09T17:14:43Z

sphinx_gallery/docs_resolv.py

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


Why would the sphinx import fail?

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? :)

lesteve · 2016-12-09T17:18:48Z

sphinx_gallery/docs_resolv.py

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


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

larsoner · 2016-12-09T17:23:39Z

@lesteve done

larsoner · 2016-12-09T17:36:37Z

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 · 2016-12-09T17:37:47Z

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 · 2016-12-09T17:38:42Z

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 · 2016-12-09T17:42:13Z

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.

GaelVaroquaux · 2016-12-09T17:52:25Z

sphinx_gallery/docs_resolv.py

 import re
 import shelve
 import sys
+import sphinx


Are the sphinx import and the LooseVesion import still useful?

lesteve · 2016-12-09T17:58:18Z

sphinx_gallery/docs_resolv.py

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


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.

lesteve · 2016-12-09T18:00:02Z

sphinx_gallery/docs_resolv.py

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


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

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 · 2016-12-09T18:03:38Z

Unnecessary imports removed.

GaelVaroquaux · 2016-12-09T18:07:22Z

sphinx_gallery/docs_resolv.py

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


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.

It's okay, this bit is new :)

Yes I can catch just HTTPError and URLError

lesteve · 2016-12-09T18:13:04Z

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 · 2016-12-09T18:14:42Z

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 · 2016-12-09T18:15:32Z

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 · 2016-12-09T19:23:57Z

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

WIP: Fix link error to sphinx file

55552ed

larsoner force-pushed the sphinx-file branch from 65c1a11 to c1f4a64 Compare December 9, 2016 15:24

larsoner added 2 commits December 9, 2016 10:25

FIX: Minor fixes

c1f4a64

ENH: Add AppVeyor

59f8eb1

larsoner mentioned this pull request Dec 9, 2016

WIP: Fix link error to sphinx file #137

Closed

larsoner force-pushed the sphinx-file branch from 93edfa4 to acc7c1a Compare December 9, 2016 15:40

FIX: Fix AppVeyor

acc7c1a

lesteve changed the title ~~FIX: Error on new sphinx~~ FIX: Error on sphinx 1.5 Dec 9, 2016

larsoner changed the title ~~FIX: Error on sphinx 1.5~~ MRG: Error on sphinx 1.5 Dec 9, 2016

larsoner force-pushed the sphinx-file branch 3 times, most recently from fa5c9b2 to 23a91ea Compare December 9, 2016 16:38

larsoner mentioned this pull request Dec 9, 2016

[MRG+1]: Fixes for Windows #179

Merged

larsoner force-pushed the sphinx-file branch from 23a91ea to 360b581 Compare December 9, 2016 16:44

GaelVaroquaux reviewed Dec 9, 2016

View reviewed changes

larsoner force-pushed the sphinx-file branch from 360b581 to f6e5fd8 Compare December 9, 2016 16:48

ENH: Better message

f6e5fd8

FIX: Roll back scipy

b54e5e9

lesteve reviewed Dec 9, 2016

View reviewed changes

FIX: Move import

4cf4c28

lesteve reviewed Dec 9, 2016

View reviewed changes

DOC: Add comment

09d8eb6

FIX: Minor fix

dd59cee

GaelVaroquaux reviewed Dec 9, 2016

View reviewed changes

lesteve reviewed Dec 9, 2016

View reviewed changes

FIX: Minor fixes

daa2502

lesteve reviewed Dec 9, 2016

View reviewed changes

GaelVaroquaux reviewed Dec 9, 2016

View reviewed changes

larsoner force-pushed the sphinx-file branch from 40c6196 to b05ddca Compare December 9, 2016 18:12

FIX: Fix except

b05ddca

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

larsoner deleted the sphinx-file branch December 9, 2016 19:27

MRG: Error on sphinx 1.5 #178

MRG: Error on sphinx 1.5 #178

Uh oh!

Conversation

larsoner commented Dec 9, 2016

Uh oh!

lesteve commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

lesteve commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

lesteve commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

GaelVaroquaux commented Dec 9, 2016

Uh oh!

lesteve commented Dec 9, 2016

Uh oh!

GaelVaroquaux commented Dec 9, 2016 via email

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

GaelVaroquaux commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Dec 9, 2016

Uh oh!

larsoner commented Dec 9, 2016

Uh oh!

lesteve commented Dec 9, 2016

Uh oh!

Reviewers

Assignees

Labels

lesteve commented Dec 9, 2016 •

edited

Loading