[MRG+1] MAINT Use magic to list documentation versions #9841
Conversation
|
Circle and Travis errors are both scikit-learn build errors related to changing g++ versions? |
|
My personal investigation starts in #9840 (comment). I am still hoping to work around the problem by using an older miniconda installer and not update conda, let's see ... |
|
At http://scikit-learn.org/circle?13937/documentation.html, see:
A nicer way to produce the "all versions" page (at least in terms of templating) would be to get sphinx to compile it as part of the dev documentation. This would mean automatically generating it by pulling scikit-learn.github.io and inspecting, which can be slow (although that would change with something like #7887 (comment)). It could alternatively be done with github contents API. |
|
That seems nice, I'll try to take a look today. |
|
@lesteve, let me know if this seems reasonable by you or if we're better off just patching up the HTML ad-hoc. |
|
It's definitely on my todo list for the 0.19.1 release but I have not got around to look at it yet. Will try to do it soon. |
|
I'm taking this out of 0.19.1 it makes no significant benefit there. |
build_tools/circle/list_versions.py
Outdated
| if suffix == SUFFIXES[0]: | ||
| return "%d%s" % (quantity, suffix) | ||
| else: | ||
| return "%.1f%s" % (quantity, suffix) |
There was a problem hiding this comment.
Currently, the generated list of versions includes e.g. "(PDF 41.8MB)" I think it would be useful to add a space between the number and the suffix.
build_tools/circle/list_versions.py
Outdated
| try: | ||
| html = urlopen(RAW_FMT % name).read().decode('utf8') | ||
| except Exception: | ||
| print('Failed to fetch %s' % (RAW_FMT % name), file=sys.stderr) |
There was a problem hiding this comment.
The latest CicleCI log has,
+ python build_tools/circle/list_versions.py
Failed to fetch https://raw.githubusercontent.com/scikit-learn/scikit-learn.github.io/master/circle/documentation.html
that link is not correct.
What happens if fetching a resource fails? Does it mean that one of the linked version will be missing? Or that's not critical?
There was a problem hiding this comment.
Perhaps we're better off with exceptions being critical and having a more curated glob.
| <li><a href="http://scikit-learn.org/0.16/documentation.html">scikit-learn 0.16</a></li> | ||
| <script>if (VERSION_SUBDIR != "stable") document.write('<li><a href="http://scikit-learn.org/stable/documentation.html">Stable version</a></li>')</script> | ||
| <script>if (VERSION_SUBDIR != "dev") document.write('<li><a href="http://scikit-learn.org/dev/documentation.html">Development version</a></li>')</script> | ||
| <li><a href="http://scikit-learn.org/dev/versions.html">Previous versions</a></li> |
There was a problem hiding this comment.
This doesn't work for circle ci preview. Can't one use relative paths here i.e. ./versions.html?
There was a problem hiding this comment.
As below, it can't be a local link if it's to work on /stable and /0.19, etc. But the moment this is visible for master, the link will also work.
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| <div class="container-index"> | |||
|
|
|||
| Documentation of scikit-learn 0.19.dev0 | |||
| Documentation of scikit-learn |version| | |||
There was a problem hiding this comment.
using sphinx magic |version|
Would you have a reference on how this works? Couldn't find it in sphinx docs...
There was a problem hiding this comment.
Like many things in sphinx I stumbled upon it by accident, so now I can't remember how to get back there!
There was a problem hiding this comment.
http://www.sphinx-doc.org/en/stable/config.html#confval-version hints at it, but I know it's elsewhere too
build_tools/circle/list_versions.py
Outdated
|
|
||
|
|
||
| digit_names = [k for k in dirs if k[:1].isdigit()] | ||
| word_names = [k for k in dirs if not k[:1].isdigit()] |
There was a problem hiding this comment.
I'm not really able to follow what's happening in the 6 lines above. Maybe a few additional comments wouldn't hurt?
| <li><a href="http://scikit-learn.org/0.16/documentation.html">Scikit-learn 0.16</a></li> | ||
| <li><a href="{{ pathto('_downloads/scikit-learn-docs.pdf', 1) }}">PDF documentation</a></li> | ||
| <script>if (VERSION_SUBDIR != "stable") document.write('<li><a href="http://scikit-learn.org/stable/documentation.html">Stable version</a></li>')</script> | ||
| <script>if (VERSION_SUBDIR != "dev") document.write('<li><a href="http://scikit-learn.org/dev/documentation.html">Development version</a></li>')</script> |
There was a problem hiding this comment.
Couldn't this be done with sphinx templating ? My concern is that links dynamically changed with javascript are not so friendly toward search engines. Though it might not matter much in the case of scikit-learn...
There was a problem hiding this comment.
Concern about spidering? Now it would have to spider through versions.html.... but that's not a great deal is it?
Sphinx templating... Lemme think.
There was a problem hiding this comment.
I think for sphinx templating to work, we'd need to set a setting when building as to whether the document will be viewed as stable/dev/other. I don't think this is reasonable. When something moves from stable to historical, we don't want to recompile its docs. That's sort of the point of this PR.
There was a problem hiding this comment.
I think for sphinx templating to work, we'd need to set a setting when building as to whether the document will be viewed as stable/dev/other. I don't think this is reasonable.
Aww yes, I see your point.
| <li><a href="{{ pathto('_downloads/scikit-learn-docs.pdf', 1) }}">PDF documentation</a></li> | ||
| <script>if (VERSION_SUBDIR != "stable") document.write('<li><a href="http://scikit-learn.org/stable/documentation.html">Stable version</a></li>')</script> | ||
| <script>if (VERSION_SUBDIR != "dev") document.write('<li><a href="http://scikit-learn.org/dev/documentation.html">Development version</a></li>')</script> | ||
| <li><a href="http://scikit-learn.org/dev/versions.html">Previous versions</a></li> |
There was a problem hiding this comment.
No, it can't be a local link if it's to work on /stable and /0.19, etc.
|
Thanks for the review, @rth! Do you think this is worthwhile overall, or
too magic for new maintainers?
|
Codecov Report
@@ Coverage Diff @@
## master #9841 +/- ##
==========================================
+ Coverage 96.17% 96.17% +<.01%
==========================================
Files 336 336
Lines 62406 62413 +7
==========================================
+ Hits 60017 60024 +7
Misses 2389 2389
Continue to review full report at Codecov.
|
| <script> | ||
| VERSION_SUBDIR = (function(groups) { | ||
| return groups ? groups[1] : null; | ||
| })(location.href.match(/^https?:\/\/scikit-learn.org\/([^\/]+)/)); |
There was a problem hiding this comment.
Checked that the above javascript regexp works as expected with the following code (run here),
<!DOCTYPE html>
<html>
<body>
<button onclick="myFunction()">Match</button>
<p id="demo"></p>
<script>
var url = "http://scikit-learn.org/0.19/modules/linear_model.html#lars-lasso"
function matchUrl(groups) {
return groups ? groups[1] : null;
};
function myFunction() {
var x = matchUrl(url.match(/^https?:\/\/scikit-learn.org\/([^\/]+)/));
document.getElementById("demo").innerHTML = x;
}
</script>
</body>
</html>|
@jnothman Sorry for the late reply. Thanks for responding to all the comments.
It would require some effort to understand how this works for new maintainers (and I am moderately enthusiastic about JS generated links) but I think it's really useful to have an always up to date list of the documentation for previous versions including the PDF. Also, I don't have other ideas how the same results could be achieved in a simpler way. So generally this LGTM. |
|
thanks for the affirmation!
|
doc/support.rst
Outdated
| versions can be found here: | ||
| This documentation is relative to |release|. Documentation for | ||
| other versions can be found `here | ||
| <http://scikit-learn.org/dev/documentation.html>`_. |
There was a problem hiding this comment.
I think http://scikit-learn.org/dev/versions.html would be more direct, no?
|
And feel free to merge if that seems to be consensus. |
|
Thanks @jnothman ! |
| @@ -114,6 +114,13 @@ pip install sphinx-gallery numpydoc | |||
| # Build and install scikit-learn in dev mode | |||
| python setup.py develop | |||
|
|
|||
| # TO BE UNCOMMENTED BEFORE MERGE | |||
There was a problem hiding this comment.
Seems strange for me. Ignore if that's intended.
There was a problem hiding this comment.
Yep good catch @qinhanmin2014, I have pushed e2584c3
Not sure, also I was wondering why the version list is only generated on master and not on 0.X.Y branches (if I am understanding the code correctly): scikit-learn/build_tools/circle/build_doc.sh Lines 119 to 123 in e2584c3 |
|
All generated docs link to the same page: |
|
@lesteve Thanks :) I advocate to push it to 0.19 branch mainly bacause I think we should provide users a way to get access to the dev doc through the stable doc apart from typing dev in the url. |
|
I've cherry-picked to 0.19.X (6554ed3)
|
|
Small comment, on the stable doc, I think that "Previous versions" is a bit misleading because its lists 0.20.dev0: Maybe "Other versions"? |
But seems that it lists 0.19.1 itself. Maybe something like "All available versions" ? |
Good point, your suggestion seems fine. Better suggestions welcome! |
|
Shrug. Be bold. Make the change.
|
* tag '0.19.2': Release 0.19.2 [MRG] Fix collections.abc deprecations with Python 3.7 (0.19.X backport) (scikit-learn#11509) FIX need to explicitly raise SkipTest MAINT disabled some tests failing under macOS and 32-bit Python on windows. MNT Release 0.19.2 with python 3.7 support (scikit-learn#11422) DOC previous versions -> all available versions (scikit-learn#10179) MAINT only build versions list on master branch [MRG+1] MAINT Use magic to list documentation versions (scikit-learn#9841) DOC Add Examples heading minor fixes to whatsnew fix release 0.19.1 whatsnew link (scikit-learn#9981)
* releases: Release 0.19.2 [MRG] Fix collections.abc deprecations with Python 3.7 (0.19.X backport) (scikit-learn#11509) FIX need to explicitly raise SkipTest MAINT disabled some tests failing under macOS and 32-bit Python on windows. MNT Release 0.19.2 with python 3.7 support (scikit-learn#11422) DOC previous versions -> all available versions (scikit-learn#10179) MAINT only build versions list on master branch [MRG+1] MAINT Use magic to list documentation versions (scikit-learn#9841) DOC Add Examples heading minor fixes to whatsnew fix release 0.19.1 whatsnew link (scikit-learn#9981)
* dfsg: Release 0.19.2 [MRG] Fix collections.abc deprecations with Python 3.7 (0.19.X backport) (scikit-learn#11509) FIX need to explicitly raise SkipTest MAINT disabled some tests failing under macOS and 32-bit Python on windows. MNT Release 0.19.2 with python 3.7 support (scikit-learn#11422) DOC previous versions -> all available versions (scikit-learn#10179) MAINT only build versions list on master branch [MRG+1] MAINT Use magic to list documentation versions (scikit-learn#9841) DOC Add Examples heading minor fixes to whatsnew fix release 0.19.1 whatsnew link (scikit-learn#9981)
Fixes #9838 with the exception of dealing with the NEWS section of index.rst. See #9838 (comment)
This makes the listing of documentation versions dynamic by:
|version|DOCUMENTATION_OPTIONS.VERSIONdevorstabledir