[SPARK-24530][PYTHON] Add a control to force Python version in Sphinx via environment variable, SPHINXPYTHON #21659

HyukjinKwon · 2018-06-28T17:10:56Z

What changes were proposed in this pull request?

This PR proposes to add SPHINXPYTHON environment variable to control the Python version used by Sphinx.

The motivation of this environment variable is, it seems not properly rendering some signatures in the Python documentation when Python 2 is used by Sphinx. See the JIRA's case. It should be encouraged to use Python 3, but looks we will probably live with this problem for a long while in any event.

For the default case of make html, it keeps previous behaviour and use SPHINXBUILD as it was. If SPHINXPYTHON is set, then it forces Sphinx to use the specific Python version.

$ SPHINXPYTHON=python3 make html
python3 -msphinx -b html -d _build/doctrees   . _build/html
Running Sphinx v1.7.5
...

if SPHINXPYTHON is set, use Python. If SPHINXBUILD is set, use sphinx-build.
If both are set, SPHINXBUILD has a higher priority over SPHINXPYTHON
By default, SPHINXBUILD is used as 'sphinx-build'.

Probably, we can somehow work around this via explicitly setting SPHINXBUILD but sphinx-build can't be easily distinguished since it (at least in my environment and up to my knowledge) doesn't replace sphinx-build when newer Sphinx is installed in different Python version. It confuses and doesn't warn for its Python version.

How was this patch tested?

Manually tested:

python (Python 2.7) in the path with Sphinx:

$ make html
sphinx-build -b html -d _build/doctrees   . _build/html
Running Sphinx v1.7.5
...

python (Python 2.7) in the path without Sphinx:

$ make html
Makefile:8: *** The 'sphinx-build' command was not found. Make sure you have Sphinx installed, then set the SPHINXBUILD environment variable to point to the full path of the 'sphinx-build' executable. Alternatively you can add the directory with the executable to your PATH. If you don't have Sphinx installed, grab it from http://sphinx-doc.org/.  Stop.

SPHINXPYTHON set python (Python 2.7) with Sphinx:

$ SPHINXPYTHON=python make html
Makefile:35: *** Note that Python 3 is required to generate PySpark documentation correctly for now. Current Python executable was less than Python 3. See SPARK-24530. To force Sphinx to use a specific Python executable, please set SPHINXPYTHON to point to the Python 3 executable..  Stop.

SPHINXPYTHON set python (Python 2.7) without Sphinx:

$ SPHINXPYTHON=python make html
Makefile:35: *** Note that Python 3 is required to generate PySpark documentation correctly for now. Current Python executable was less than Python 3. See SPARK-24530. To force Sphinx to use a specific Python executable, please set SPHINXPYTHON to point to the Python 3 executable..  Stop.

SPHINXPYTHON set python3 with Sphinx:

$ SPHINXPYTHON=python3 make html
python3 -msphinx -b html -d _build/doctrees   . _build/html
Running Sphinx v1.7.5
...

SPHINXPYTHON set python3 without Sphinx:

$ SPHINXPYTHON=python3 make html
Makefile:39: *** Python executable 'python3' did not have Sphinx installed. Make sure you have Sphinx installed, then set the SPHINXPYTHON environment variable to point to the Python executable having Sphinx installed. If you don't have Sphinx installed, grab it from http://sphinx-doc.org/.  Stop.

SPHINXBUILD set:

$ SPHINXBUILD=sphinx-build make html
sphinx-build -b html -d _build/doctrees   . _build/html
Running Sphinx v1.7.5
...

Both SPHINXPYTHON and SPHINXBUILD are set:

$ SPHINXBUILD=sphinx-build SPHINXPYTHON=python make html
sphinx-build -b html -d _build/doctrees   . _build/html
Running Sphinx v1.7.5
...

HyukjinKwon · 2018-06-28T17:11:52Z

cc @mengxr, let me update the release process and doc on this weekend separately. I was just looking into this script out of curiosity and I felt it's worth adding it to explicitly force the Python version used by Sphinx.

SparkQA · 2018-06-28T17:48:22Z

Test build #92430 has finished for PR 21659 at commit 1366b44.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2018-06-28T17:48:37Z

python/docs/Makefile

less then -> less than.

SparkQA · 2018-06-28T17:54:36Z

Test build #92431 has finished for PR 21659 at commit 24220d5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-06-29T01:43:16Z

Test build #92444 has finished for PR 21659 at commit d77c037.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-06-29T04:29:13Z

Test build #92450 has finished for PR 21659 at commit b40c41c.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-06-29T04:58:01Z

python/docs/Makefile

~~Put in SPHINXOPTS?~~

HyukjinKwon · 2018-06-29T05:07:38Z

Haa ... I guess API documentation builder in Jenkins should probably be broken if this one is merged in as is .. and the env there should be updated too.. Let me try to tweak this a bit on this weekend together.

SparkQA · 2018-06-29T07:05:01Z

Test build #92451 has finished for PR 21659 at commit 144ec68.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-06-29T07:05:02Z

Test build #92452 has finished for PR 21659 at commit e00a7ae.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-06-29T07:05:02Z

Test build #92453 has finished for PR 21659 at commit 2378726.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-06-29T07:07:21Z

retest this please.

This reverts commit dd303b6.

HyukjinKwon · 2018-06-29T08:42:30Z

Now, it keeps the previous behaviour and use the Python iff SPHINXPYTHON is set.

viirya · 2018-06-29T08:55:24Z

python/docs/Makefile

 #

+ifndef SPHINXPYTHON
+SPHINXBUILD ?= sphinx-build


If both are set, we prefer SPHINXPYTHON?

yup, I updated the manual tests above. There are commands executed in the logs.

viirya · 2018-06-29T08:57:56Z

python/docs/Makefile

+# User-friendly check for sphinx-build if explicitly specified.
+ifeq ($(shell which $(SPHINXBUILD) >/dev/null 2>&1; echo $$?), 1)
+$(error The '$(SPHINXBUILD)' command was not found. Make sure you have Sphinx installed, then set the SPHINXBUILD environment variable to point to the full path of the '$(SPHINXBUILD)' executable. Alternatively you can add the directory with the executable to your PATH. If you don't have Sphinx installed, grab it from http://sphinx-doc.org/)
+endif


If SPHINXPYTHON -msphinx' is preferred instead of 'sphinx-build' by default, should we go for a default SPHINXPYTHON + -msphinx instead of a default SPHINXBUILD if both are not set?

Yea... that was the problem I faced.. Probably we should change but seems it needs some changes in Jenkins environment .. FYI there are some discussion about the potential effect by setting that as a default in the PR link of the comments.

I was thinking we set SPHINXPYTHON manually when we make a release for now.

That seems error prone - why not pick a good default of python3 and allow overriding?

SparkQA · 2018-06-29T09:17:01Z

Test build #92468 has finished for PR 21659 at commit dd303b6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-06-29T09:19:44Z

Test build #92469 has finished for PR 21659 at commit 69f31a9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-06-29T11:18:36Z

Test build #92462 has finished for PR 21659 at commit 2378726.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk

Thanks for working on this issue! It seems like we could improve this by setting the default to Python3 and allowing override to Python2?

holdenk · 2018-07-02T04:43:10Z

python/docs/Makefile

-SPHINXBUILD   ?= sphinx-build
 PAPER         ?=
 BUILDDIR      ?= _build
+# Also, you can set SPHINXBUILD to specify Sphinx build executable or SPHINXPYTHON to specify the Python executable used in Sphinx.


Specify the priority here.

holdenk · 2018-07-02T04:45:24Z

python/docs/Makefile

+# User-friendly check for sphinx-build if explicitly specified.
+ifeq ($(shell which $(SPHINXBUILD) >/dev/null 2>&1; echo $$?), 1)
+$(error The '$(SPHINXBUILD)' command was not found. Make sure you have Sphinx installed, then set the SPHINXBUILD environment variable to point to the full path of the '$(SPHINXBUILD)' executable. Alternatively you can add the directory with the executable to your PATH. If you don't have Sphinx installed, grab it from http://sphinx-doc.org/)
+endif


That seems error prone - why not pick a good default of python3 and allow overriding?

HyukjinKwon · 2018-07-02T15:59:11Z

re: #21659 (comment) Thing is, the build fails on Jenkins and it will probably fail on documentation builder in Jenkins. For now, I just gave a higher priority to SPHINXBUILD to just keep the previous behaviour and make Jenkins happy.

Now, SPHINXPYTHON is completely optional. I updated PR description. Should be ready for a look now.

SparkQA · 2018-07-02T16:38:16Z

Test build #92531 has finished for PR 21659 at commit 135d613.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-07-02T16:43:12Z

Test build #92532 has finished for PR 21659 at commit 71ff040.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mengxr · 2018-07-03T17:32:42Z

@HyukjinKwon I reported the issue to Sphinx at sphinx-doc/sphinx#5142. We are not sure if Python 2 is the root cause. I don't have strong preferences on the approaches discussed here. But maybe we should wait and see if there is a fix/workaround from upstream.

HyukjinKwon · 2018-07-03T17:53:13Z

@mengxr, thanks for reporting it. This will probably related with functools.wraps's behaviour between Python 2 and 3. Python 3 copies its signature and Python 2 doesn't. Let me make a progress here if we don't get a response for a quite long time.

FWIW, I believe the API documentation fixes in spark-website can be orthogonally progressed.

holdenk · 2018-07-03T18:49:27Z

When you say orthogonally do you mean with another PR?

holdenk · 2018-07-03T18:51:37Z

python/docs/Makefile

+endif
+endif
+
+ifdef SPHINXBUILD


Is there a way to check that Sphinx build is running with Py3 as done bellow?

I couldn't find an easy way.

HyukjinKwon · 2018-07-04T01:37:22Z

Ah, I mean PRs in spark-website to fix the API documentation.

HyukjinKwon · 2018-07-09T01:00:55Z

retest this please

SparkQA · 2018-07-09T01:38:56Z

Test build #92724 has finished for PR 21659 at commit 71ff040.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

This reverts commit 950ead0.

jerryshao · 2018-07-09T02:37:58Z

python/docs/Makefile

+else
+# Note that there is an issue with Python version and Sphinx in PySpark documentation generation.
+# Please remove this check below when this issue is fixed. See SPARK-24530 for more details.
+PYTHON_VERSION_CHECK = $(shell $(SPHINXPYTHON) -c 'import sys; print(sys.version_info < (3, 0, 0))')


Can we fix the SPHINXPYTHON to python3 in release script release-build.sh?

Or add some options/outputs in release script to let others know how to workaround this issue.

Forcing SPHINXPYTHON to python3 by default will probably break the distribution builder in Jenkins if they are tried ... Seems there's an issue to force Sphinx to use Python 3 in Jenkins environment. This was the problem I struggled to tweak :(.

Am trying to update the release process - apache/spark-website#122. Would this be enough to address your concern?

SparkQA · 2018-07-09T02:44:14Z

Test build #92725 has finished for PR 21659 at commit 950ead0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-07-09T02:48:37Z

Test build #92726 has finished for PR 21659 at commit d500e0d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2018-07-10T01:49:24Z

docs/README.md

 `$SPARK_HOME/python/docs` directory. Documentation is only generated for classes that are listed as
-public in `__init__.py`. The SparkR docs can be built by running `$SPARK_HOME/R/create-docs.sh`, and
+public in `__init__.py`. You can also set `SPHINXPYTHON` to specify the Python executable to use with Sphinx.
+ The SparkR docs can be built by running `$SPARK_HOME/R/create-docs.sh`, and


Let me take out this change in this PR, actually. It makes backporting harder

HyukjinKwon · 2018-07-10T01:55:17Z

@mengxr, mind if I go ahead? This seems actually bothering release processes.

SparkQA · 2018-07-10T02:36:26Z

Test build #92784 has finished for PR 21659 at commit 2e1e7da.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jerryshao · 2018-07-11T01:36:22Z

I think we can treat this fix as a temp fix, once sphinx issue is resolved, we can update in Spark accordingly. This is not a big problem, only affects python doc, so I think it is safe to merge.

HyukjinKwon · 2018-07-11T02:07:26Z

Let me push this one. Also, it doesn't change anything by default.

HyukjinKwon · 2018-07-11T02:10:28Z

Merged to master and branch-2.3.

… via environment variable, SPHINXPYTHON ## What changes were proposed in this pull request? This PR proposes to add `SPHINXPYTHON` environment variable to control the Python version used by Sphinx. The motivation of this environment variable is, it seems not properly rendering some signatures in the Python documentation when Python 2 is used by Sphinx. See the JIRA's case. It should be encouraged to use Python 3, but looks we will probably live with this problem for a long while in any event. For the default case of `make html`, it keeps previous behaviour and use `SPHINXBUILD` as it was. If `SPHINXPYTHON` is set, then it forces Sphinx to use the specific Python version. ``` $ SPHINXPYTHON=python3 make html python3 -msphinx -b html -d _build/doctrees . _build/html Running Sphinx v1.7.5 ... ``` 1. if `SPHINXPYTHON` is set, use Python. If `SPHINXBUILD` is set, use sphinx-build. 2. If both are set, `SPHINXBUILD` has a higher priority over `SPHINXPYTHON` 3. By default, `SPHINXBUILD` is used as 'sphinx-build'. Probably, we can somehow work around this via explicitly setting `SPHINXBUILD` but `sphinx-build` can't be easily distinguished since it (at least in my environment and up to my knowledge) doesn't replace `sphinx-build` when newer Sphinx is installed in different Python version. It confuses and doesn't warn for its Python version. ## How was this patch tested? Manually tested: **`python` (Python 2.7) in the path with Sphinx:** ``` $ make html sphinx-build -b html -d _build/doctrees . _build/html Running Sphinx v1.7.5 ... ``` **`python` (Python 2.7) in the path without Sphinx:** ``` $ make html Makefile:8: *** The 'sphinx-build' command was not found. Make sure you have Sphinx installed, then set the SPHINXBUILD environment variable to point to the full path of the 'sphinx-build' executable. Alternatively you can add the directory with the executable to your PATH. If you don't have Sphinx installed, grab it from http://sphinx-doc.org/. Stop. ``` **`SPHINXPYTHON` set `python` (Python 2.7) with Sphinx:** ``` $ SPHINXPYTHON=python make html Makefile:35: *** Note that Python 3 is required to generate PySpark documentation correctly for now. Current Python executable was less than Python 3. See SPARK-24530. To force Sphinx to use a specific Python executable, please set SPHINXPYTHON to point to the Python 3 executable.. Stop. ``` **`SPHINXPYTHON` set `python` (Python 2.7) without Sphinx:** ``` $ SPHINXPYTHON=python make html Makefile:35: *** Note that Python 3 is required to generate PySpark documentation correctly for now. Current Python executable was less than Python 3. See SPARK-24530. To force Sphinx to use a specific Python executable, please set SPHINXPYTHON to point to the Python 3 executable.. Stop. ``` **`SPHINXPYTHON` set `python3` with Sphinx:** ``` $ SPHINXPYTHON=python3 make html python3 -msphinx -b html -d _build/doctrees . _build/html Running Sphinx v1.7.5 ... ``` **`SPHINXPYTHON` set `python3` without Sphinx:** ``` $ SPHINXPYTHON=python3 make html Makefile:39: *** Python executable 'python3' did not have Sphinx installed. Make sure you have Sphinx installed, then set the SPHINXPYTHON environment variable to point to the Python executable having Sphinx installed. If you don't have Sphinx installed, grab it from http://sphinx-doc.org/. Stop. ``` **`SPHINXBUILD` set:** ``` $ SPHINXBUILD=sphinx-build make html sphinx-build -b html -d _build/doctrees . _build/html Running Sphinx v1.7.5 ... ``` **Both `SPHINXPYTHON` and `SPHINXBUILD` are set:** ``` $ SPHINXBUILD=sphinx-build SPHINXPYTHON=python make html sphinx-build -b html -d _build/doctrees . _build/html Running Sphinx v1.7.5 ... ``` Author: hyukjinkwon <[email protected]> Closes #21659 from HyukjinKwon/SPARK-24530. (cherry picked from commit 1f94bf4) Signed-off-by: hyukjinkwon <[email protected]>

HyukjinKwon · 2018-09-20T03:56:55Z

cc @cloud-fan fyi

HyukjinKwon changed the title ~~[SPARK-24530][PYTHON] Add a control to force Python version via environment variable, SPHINXPYTHON~~ [SPARK-24530][PYTHON] Add a control to force Python version in Sphinx via environment variable, SPHINXPYTHON Jun 28, 2018

dongjoon-hyun reviewed Jun 28, 2018

View reviewed changes

python/docs/Makefile Outdated

Copy link

Member

dongjoon-hyun Jun 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

less then -> less than.

HyukjinKwon force-pushed the SPARK-24530 branch from 144ec68 to e00a7ae Compare June 29, 2018 04:48

viirya reviewed Jun 29, 2018

View reviewed changes

python/docs/Makefile Outdated

Copy link

Member

viirya Jun 29, 2018 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~~Put in SPHINXOPTS?~~

HyukjinKwon changed the title ~~[SPARK-24530][PYTHON] Add a control to force Python version in Sphinx via environment variable, SPHINXPYTHON~~ [WIP][SPARK-24530][PYTHON] Add a control to force Python version in Sphinx via environment variable, SPHINXPYTHON Jun 29, 2018

Add a control to force Python version via enviornment variable

a1346e2

HyukjinKwon force-pushed the SPARK-24530 branch from 2378726 to a1346e2 Compare June 29, 2018 08:33

HyukjinKwon added 2 commits June 29, 2018 16:33

Python change to trigger the stlye check

dd303b6

Revert "Python change to trigger the stlye check"

69f31a9

This reverts commit dd303b6.

HyukjinKwon changed the title ~~[WIP][SPARK-24530][PYTHON] Add a control to force Python version in Sphinx via environment variable, SPHINXPYTHON~~ [SPARK-24530][PYTHON] Add a control to force Python version in Sphinx via environment variable, SPHINXPYTHON Jun 29, 2018

viirya reviewed Jun 29, 2018

View reviewed changes

holdenk reviewed Jul 2, 2018

View reviewed changes

Address comments and correct its priority

135d613

Fix some comments

71ff040

HyukjinKwon mentioned this pull request Jul 2, 2018

Update release process to use Python 3 for Python API documentation apache/spark-website#122

Merged

holdenk reviewed Jul 3, 2018

View reviewed changes

HyukjinKwon added 2 commits July 9, 2018 10:04

Run pydocstyle

950ead0

Revert "Run pydocstyle"

d500e0d

This reverts commit 950ead0.

jerryshao reviewed Jul 9, 2018

View reviewed changes

HyukjinKwon commented Jul 10, 2018

View reviewed changes

Remove changes in README.md which makes backporting harder

2e1e7da

asfgit closed this in 1f94bf4 Jul 11, 2018

HyukjinKwon deleted the SPARK-24530 branch October 16, 2018 12:43

[SPARK-24530][PYTHON] Add a control to force Python version in Sphinx via environment variable, SPHINXPYTHON #21659

[SPARK-24530][PYTHON] Add a control to force Python version in Sphinx via environment variable, SPHINXPYTHON #21659

Uh oh!

Conversation

HyukjinKwon commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon commented Jun 28, 2018

Uh oh!

SparkQA commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 28, 2018

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

viirya Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jun 29, 2018

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

viirya commented Jun 29, 2018

Uh oh!

HyukjinKwon commented Jun 29, 2018

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

SparkQA commented Jun 29, 2018

Uh oh!

holdenk left a comment

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!

HyukjinKwon commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 2, 2018

Uh oh!

SparkQA commented Jul 2, 2018

Uh oh!

mengxr commented Jul 3, 2018

Uh oh!

HyukjinKwon commented Jul 3, 2018

Uh oh!

holdenk commented Jul 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jul 4, 2018

Uh oh!

HyukjinKwon commented Jul 9, 2018

HyukjinKwon commented Jun 28, 2018 •

edited

Loading

viirya Jun 29, 2018 •

edited

Loading

HyukjinKwon commented Jul 2, 2018 •

edited

Loading

HyukjinKwon Jul 10, 2018 •

edited

Loading