[MRG+2] DOC add links to github sourcecode in API reference #2777

jnothman · 2014-01-21T00:09:12Z

I enjoy using the API reference, but think a link to the source code on github would often help.

Because sphinx.ext.linkcode does not provide the full path or line number of the class/function, I'm getting this stuff using grep, which relies on a unix shell and certain path structures, which is a bit not-nice. Since github needs a branch/commit reference, I'm using git rev-parse --abbrev-ref HEAD which can return a branch/tag name, but a more static commit reference (i.e. rev-parse --short) might be more sensible.

WDYT? ping @jaquesgrobler

jaquesgrobler · 2014-01-21T16:17:10Z

I'm a fan of this.. There was some discussion on this idea a while back on #1680 but no consensus was reached..
I scanned through your code and looks fine .. Don't have time for an in detail look this second but will try later tonight or tomorrow.. that asside i'm +1 on this as a few people have requested this

👍

jnothman · 2014-01-21T20:56:07Z

What do you think of making the compilation specific to a git checkout, and
more-or-less to a shell? I can obviously extract the grepped content
through equivalent Python (rather than shell) code, or by importing the
objects themselves (I think we already assume that the compiled Python
package is available at doc build time?) and using inspect. I don't know if
I can get the commit ref another nice way. Do you have a preference for how
this is done?

And I wouldn't mind if this was turned into a generic sphinx extension, but
don't think I'll have time to do so in the short term.

On 22 January 2014 03:17, Jaques Grobler notifications@github.com wrote:

I'm a fan of this.. There was some discussion on this idea a while back
on #1680 #1680 but
no consensus was reached..
I scanned through your code and looks good .. Don't have time for an in
detail look this second but will try later tonight or tomorrow.. that
asside i'm +1 on this

[image: 👍]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2777#issuecomment-32900529
.

jnothman · 2014-01-21T22:36:54Z

Advantages of using inspect over grep

not shell-specific (although calling system git rev-parse, or employing GitPython, is still needed)
can easily produce source links for methods as well as module-level functions
doesn't rely on relative path structures, instead using Python's path
Disadvantages:
copy installed on path may not be in sync (but should be for the rest of doc compilation to work)
may be a litle slower than grep: not a single pass through files
may fail or get incorrect source for wrapped objects such as abstractmethods
requires an alternative solution (open and grep) for pyx objects (currently just sklearn.svm.libsvm.* of http://scikit-learn.org/stable/modules/classes.html#low-level-methods) that may require using relative path knowledge

GaelVaroquaux · 2014-01-25T15:23:09Z

I'd be much more favorable to using inspect rather than grep.

I'd also say that in a first time we could punt on the *.pyx.

jnothman · 2014-01-25T20:21:51Z

I've updated with links for classes, functions and methods in Python (but not Cython), using inspect.

I've also changed from using rev-parse --abbrev-ref to rev-parse --short to give a fixed commit reference, rather than a name. Note that I still use the shell for this, but could change it to a file operation (find closest dir containing .git, read .git/HEAD, and dereference the ref from the refs dir), but I think it's safer to rely on the official tools.

jnothman · 2014-01-25T20:23:34Z

I've updated with links for classes, functions and methods in Python (but not Cython), using inspect.

I've also changed from using rev-parse --abbrev-ref to rev-parse --short to give a fixed commit reference, rather than a name. Note that I still use the shell for this, but could change it to a file operation (find closest dir containing .git, read .git/HEAD, and dereference the ref from the refs dir), but I think it's safer to rely on the official tools.

coveralls · 2014-01-25T21:18:55Z

Coverage remained the same when pulling cf91053 on jnothman:doc_linkcode into 8c3efd4 on scikit-learn:master.

jnothman · 2014-01-30T00:05:40Z

I've now noticed scipy has similar, at https://github.com/scipy/scipy/blob/6a4460f68315f0669604054be91ceeacd606f0b6/doc/source/conf.py#L314. I need to look closer to see what cases they handle that we don't, etc. Since they explicitly version their dev builds with git revisions, they are able to use different commit references in the URL when for dev or release (has or tag name respectively).

GaelVaroquaux · 2014-01-31T07:38:46Z

Hey Joel, I think I'll let you have z look at scipy before I review this PR.

jnothman · 2014-02-01T14:25:46Z

Diffs:

scipy uses master or version tags. I'm using commit hashes
scipy handled __init__.py where I failed. Now fixed.

jnothman · 2014-02-12T21:41:55Z

@GaelVaroquaux, I think we should pull this in, and see if there are any problems with the current commit refs.

GaelVaroquaux · 2014-02-12T21:54:32Z

I'll have a look.

I however have a hard time feeling excited about this feature. For me it is additional code that will at some point fail and require debugging or evolution (all code does) for little feature.

I satisfy my need to look at source code by using "symbol??" in IPython.

Sorry, I should be thankful for the feature and it's not much code actually. It's just that I have the feeling that we are quite heavily loaded in terms of maintenance and I personally never have time to push features that I really need. So feel like focusing on the high-value code, where of course high value varies from one person to another. :)

GaelVaroquaux · 2014-02-12T21:59:14Z

doc/conf.py

+        return
+
+    try:
+        revision = linkcode_resolve.revision


I am a bit dense, but where is this set?

8000
In a previous version of the code, apparently :s

jnothman · 2014-02-12T21:59:22Z

Sure, I understand that. I prefer symbol?? too... except when I'm
following a link from the narrative doc to the API reference, and then want
to clarify the code.

On 13 February 2014 08:54, Gael Varoquaux notifications@github.com wrote:

I'll have a look.

I however have a hard time feeling excited about this feature. For me it
is additional code that will at some point fail and require debugging or
evolution (all code does) for little feature.

I satisfy my need to look at source code by using "symbol??" in IPython.

Sorry, I should be thankful for the feature and it's not much code
actually. It's just that I have the feeling that we are quite heavily
loaded in terms of maintenance and I personally never have time to push
features that I really need. So feel like focusing on the high-value code,
where of course high value varies from one person to another. :)

Reply to this email directly or view it on GitHubhttps://github.com//pull/2777#issuecomment-34922636
.

GaelVaroquaux · 2014-02-12T22:01:28Z

doc/conf.py

+              'blob/{revision}/{pre_path}{path}#L{lineno}')
+
+
+def linkcode_resolve(domain, info):


I think that you should indicate (eg in a comment, or in the docstring) that this is used by the sphinx linkcode extension.

GaelVaroquaux · 2014-02-12T22:09:42Z

Lightweight and elegant code.

I made a couple of comment. My only additional and non trivial comments would be:

Could you please move the corresponding code in another file in the sphinx_ext. It would make the conf.py easier to read, and all you need is to import the right symbol (linkcode_resolve).
Can you check whether it is possible to have nosetest run tests in the sphinx_ext folder (it might require a bit of work on the setup.cfg). I am getting more and more nervous about or sphinx_ext folder and the fact that it is untested.

Thanks a lot for your work, as usual.

coveralls · 2014-02-12T22:16:00Z

Coverage remained the same when pulling 4b7973c on jnothman:doc_linkcode into b895586 on scikit-learn:master.

jnothman · 2014-02-12T22:55:10Z

I've recently moved to a new computer and it seems I can't even run make html-noplot.

I don't think there'll be a problem testing sphinxext if we want to.

On 13 February 2014 09:16, Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/514068

Coverage remained the same when pulling 4b7973c
4b7973c
on jnothman:doc_linkcode into b895586
b895586
on scikit-learn:master.

Reply to this email directly or view it on GitHubhttps://github.com//pull/2777#issuecomment-34924867
.

coveralls · 2014-02-12T23:42:11Z

Coverage remained the same when pulling 8b87b78 on jnothman:doc_linkcode into b895586 on scikit-learn:master.

jnothman · 2014-02-13T00:35:09Z

The refactored version can now be more easily tested, e.g. using standard library or sphinx modules for independence:

>>> from github_link import _linkcode_resolve
>>> _linkcode_resolve('py', {'module': 'distutils', 'fullname': 'config.PyPIRCCommand.initialize_options'},
...     package='distutils',
...     url_fmt='http://hg.python.org/cpython/file/{revision}/Lib/{package}/{path}#L{lineno}',
...     revision='xxxx')
'http://hg.python.org/cpython/file/xxxx/Lib/distutils/config.py#L105'

kastnerkyle · 2014-06-05T06:35:42Z

Getting an error building the docs with make html after rebasing this branch with master:

Exception occurred:
  File "/volatile/accounts/kkastner/src/scikit-learn/doc/sphinxext/github_link.py", line 31, in _linkcode_resolve
    class_name = info['fullname'].encode('utf-8').split('.')[0]
TypeError: Type str doesn't support the buffer API

I am also getting an error with nosetests, in the doctest part

ERROR: Failure: ImportError (No module named 'github_link')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/volatile/accounts/kkastner/anaconda3/lib/python3.4/site-packages/nose/failure.py", line 39, in runTest
    raise self.exc_val.with_traceback(self.tb)
  File "/volatile/accounts/kkastner/anaconda3/lib/python3.4/site-packages/nose/loader.py", line 414, in loadTestsFromName
    addr.filename, addr.module)
  File "/volatile/accounts/kkastner/anaconda3/lib/python3.4/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/volatile/accounts/kkastner/anaconda3/lib/python3.4/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/volatile/accounts/kkastner/anaconda3/lib/python3.4/imp.py", line 235, in load_module
    return load_source(name, filename, file)
  File "/volatile/accounts/kkastner/anaconda3/lib/python3.4/imp.py", line 171, in load_source
    module = methods.load()
  File "<frozen importlib._bootstrap>", line 1220, in load
  File "<frozen importlib._bootstrap>", line 1200, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 1129, in _exec
  File "<frozen importlib._bootstrap>", line 1471, in exec_module
  File "<frozen importlib._bootstrap>", line 321, in _call_with_frames_removed
  File "/volatile/accounts/kkastner/src/scikit-learn/doc/conf.py", line 26, in <module>
    from github_link import make_linkcode_resolve
ImportError: No module named 'github_link'

I am using Python 3.4, other packages are from Anaconda 2.0 - looks like the first might be a Py3 strings issue, specifically buffer vs. memoryview? As for the second, @ogrisel mentioned a possible issue with local imports in python 3?

kastnerkyle · 2014-06-05T11:51:05Z

In Python 2.7 I only get the github_link error - looks like the first is specific to Python 3.

Additionally, the docs that are built work fine locally (well, as good as master, which still has a few issues it seems). But, if you build from a git branch and click the link, the corresponding github text does not work. IE I get https://github.com/scikit-learn/scikit-learn/blob/03d48dd/sklearn/cluster/dbscan_.py#L178 . Changing 03d48dd to master, so the link becomes https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/cluster/dbscan_.py#L178 , everything works as expected.

~~I don't know if this is critical or not - a local doc with git already has the source directly, and on the website it should work as expected (I think). Thoughts?~~

After talking with @ogrisel, it seems this is happening because of the rebase with master - since rebase changed the commit hash the link doesn't show up correctly. It should work fine once merged, beyond the github_link thing, which is not happening on Travis apparently.

jnothman · 2014-07-15T09:24:09Z

@fabianp I'll revert it and see what happens.

jnothman · 2014-07-15T12:16:27Z

I can't understand the AppVeyor interface - it seems to be missing the consoles that I usually go to for explanation of the failure.

ogrisel · 2014-07-15T12:46:49Z

Yes there is a momentary bug. I reported it:

http://help.appveyor.com/discussions/problems/541-job-that-runs-untill-timeout-without-any-output

ogrisel · 2014-07-15T12:47:07Z

I will disable appveyor in the mean time.

ogrisel · 2014-07-17T16:02:22Z

@fabianp I'll revert it and see what happens.

@jnothman what do you want to revert? Is this PR ready for merge or not?

jnothman · 2014-07-17T23:31:55Z

I've done with revert, forcing a Travis check by rebasing.

jnothman · 2014-07-18T01:41:18Z

Failing with

======================================================================
ERROR: Failure: ImportError (No module named github_link)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/nose/loader.py", line 411, in loadTestsFromName
    addr.filename, addr.module)
  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/home/travis/virtualenv/python2.7_with_system_site_packages/local/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/home/travis/build/scikit-learn/scikit-learn/doc/conf.py", line 26, in <module>
    from github_link import make_linkcode_resolve
ImportError: No module named github_link

vene · 2014-07-20T15:20:29Z

doc/sphinxext/github_link.py

+from functools import partial
+
+
+def _get_git_revision(CMD='git rev-parse --short HEAD'):


I'm not crazy about this function definition. I can't think of any context where you'd want to call this function with an argument, and argument names shouldn't be all caps.

I'd make CMD=... a variable defined within the function.

goes to current head, so everything is fine.

I was just commenting about the style.

Sometimes default args are used as a not-quite-global way to store constants (which was more relevant when github_link wasn't its own module). However, I may have also done this because one may choose to use --abrev-ref (which shows the branch name) instead of --short (which shows the SHA). I'll change it to a global constant.

jnothman · 2014-07-21T07:32:39Z

Ubuntu 14.04 LTS, Python3.4, NumPy 1.8.1: I get the same error, @kastnerkyle , on this branch, but not on master

jnothman · 2014-07-21T07:51:20Z

Found and fixed, @kastnerkyle.

coveralls · 2014-07-21T08:07:45Z

Coverage remained the same when pulling 79afc76 on jnothman:doc_linkcode into 0d57c23 on scikit-learn:master.

kastnerkyle · 2014-07-21T11:21:52Z

It builds fine for me now - the only thing I still see is

Warning: Embedding documentation hyperlinks requires internet access

because I am offline right now on my docbuild machine.

This seems fine to me but just wanted to report it. Other than that, this is 👍 for me

jnothman · 2014-07-21T11:25:29Z

That's actually a different matter. It's referring to links to numpy and
scipy documentation.

On 21 July 2014 21:21, Kyle Kastner notifications@github.com wrote:

It builds fine for me now - the only thing I still see is

Warning: Embedding documentation hyperlinks requires internet access

because I am offline.

This seems fine to me but just wanted to report it. Other than that, this
is [image: 👍] for me

—
Reply to this email directly or view it on GitHub
#2777 (comment)
.

kastnerkyle · 2014-07-21T11:28:06Z

OK great. I am usually online all the time - my connection bumped sometime during the doc build I guess. This seems like a really nice feature for usability and browsing!

ogrisel · 2014-07-21T11:37:17Z

I am building the doc from this PR on my box as well. Will let you know when it's done.

ogrisel · 2014-07-21T12:00:49Z

Under Python 3.4 I get URL such as:

https://github.com/scikit-learn/scikit-learn/blob/b%2779afc76%27/sklearn/ensemble/forest.py#L1116

The git hash should be decode with the .decode('ascii') method prior to building the URL.

kastnerkyle · 2014-07-21T12:01:53Z

Good catch - I didn't notice cause I have no internet :)

On Mon, Jul 21, 2014 at 2:01 PM, Olivier Grisel notifications@github.com
wrote:

Under Python 3.4 I get URL such as:

https://github.com/scikit-learn/scikit-learn/blob/b%2779afc76%27/sklearn/ensemble/forest.py#L1116

The git hash should be decode with the .decode('ascii') method prior to
building the URL.

—
Reply to this email directly or view it on GitHub
#2777 (comment)
.

ogrisel · 2014-07-21T12:02:05Z

If I fix the URL manually to remobe the leading b' and the trailing ' in my browser, it I get the right page / line in my browser.

ogrisel · 2014-07-21T12:03:04Z

doc/sphinxext/github_link.py

+                                   'blob/{revision}/{package}/'
+                                   '{path}#L{lineno}')
+    """
+    revision = _get_git_revision()


revision = _get_git_revision().decode('ascii') for instance.

Actually it's probably better to fold the decode call into the _get_git_revision function itself.

Thanks @ogrisel

jnothman · 2014-07-21T12:51:37Z

I've squashed the commit history.

ogrisel · 2014-07-21T12:55:32Z

Before your pushed 060c248, I launched a full make clean html under Python 3 with the following change:

diff --git a/doc/sphinxext/github_link.py b/doc/sphinxext/github_link.py
index 7e606a6..4f28477 100644
--- a/doc/sphinxext/github_link.py
+++ b/doc/sphinxext/github_link.py
@@ -14,7 +14,7 @@ def _get_git_revision():
     except subprocess.CalledProcessError:
         print('Failed to execute git to get revision')
         return None
-    return revision
+    return revision.decode('ascii')

and I confirm that it works as expected. I have not checked with Python 2 though.

coveralls · 2014-07-21T13:06:48Z

Coverage remained the same when pulling 060c248 on jnothman:doc_linkcode into e79d2bb on scikit-learn:master.

jnothman · 2014-07-21T13:14:01Z

I'm now generating the URL as a str in both versions; it works in both. If you'd rather an explicit unicode string in Py2, I'll try that now.

jnothman · 2014-07-21T13:27:35Z

Changed to produce a unicode string.

ogrisel · 2014-07-21T14:17:37Z

bytes is the type for byte strings in Python 3
str is the type for byte strings in Python 2 and the type for unicode strings in Python 3
unicode is the type for unicode string in Python 2

So I think we should always decode the bytes to generate URL that is a unicode string, that is str in Python 3 and unicode in Python 2.

ogrisel · 2014-07-21T14:18:46Z

Thanks for the fix. merging!

[MRG+2] DOC add links to github sourcecode in API reference

jnothman · 2014-07-21T21:18:38Z

Great! I look forward to using this when introducing people to particular scikit-learn components.

arjoly · 2014-07-22T07:45:45Z

Does it point on master or a particular branch?

jnothman · 2014-07-22T09:44:46Z

A commit ref, to ensure line number match. It's live, e.g.
http://scikit-learn.org/dev/modules/generated/sklearn.base.BaseEstimator.html#sklearn.base.BaseEstimator

On 22 July 2014 17:45, Arnaud Joly notifications@github.com wrote:

Does it point on master or a particular branch?

—
Reply to this email directly or view it on GitHub
#2777 (comment)
.

GaelVaroquaux reviewed Feb 12, 2014
View reviewed changes

jnothman mentioned this pull request Jun 6, 2014

[MRG+1] DOC more complete coverage of API linking in examples #3165

Merged

vene reviewed Jul 20, 2014
View reviewed changes

jnothman changed the title ~~[MRG+1] DOC add links to github sourcecode in API reference~~ [MRG+2] DOC add links to github sourcecode in API reference Jul 21, 2014

ogrisel reviewed Jul 21, 2014
View reviewed changes

DOC add links to github sourcecode in API reference

06b57ae

ogrisel added a commit that referenced this pull request Jul 21, 2014

Merge pull request #2777 from jnothman/doc_linkcode

7a7fca8

[MRG+2] DOC add links to github sourcecode in API reference

ogrisel merged commit 7a7fca8 into scikit-learn:master Jul 21, 2014

jnothman deleted the doc_linkcode branch July 21, 2014 21:18

larsmans mentioned this pull request Nov 30, 2014

Links from documentation to Github #1680

Closed

vinayak-mehta mentioned this pull request Mar 5, 2015

Running tests should not print anything on stdout / stderr or warnings #2274

Closed

		'blob/{revision}/{pre_path}{path}#L{lineno}')


		def linkcode_resolve(domain, info):

		from functools import partial


		def _get_git_revision(CMD='git rev-parse --short HEAD'):

Uh oh!

[MRG+2] DOC add links to github sourcecode in API reference #2777

[MRG+2] DOC add links to github sourcecode in API reference #2777

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!