-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-30216][INFRA] Use python3 in Docker release image #26848
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
Conversation
|
Test build #115167 has finished for PR 26848 at commit
|
|
Test build #115188 has finished for PR 26848 at commit
|
| pip3 install $BASE_PIP_PKGS && \ | ||
| pip3 install $PIP_PKGS && \ | ||
| cd && \ | ||
| virtualenv -p python3 /opt/p35 && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to use virtualenv when we have a system python3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will verify it later.
srowen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if it works!
dev/make-distribution.sh
Outdated
| # Delete the egg info file if it exists, this can cache older setup files. | ||
| rm -rf pyspark.egg-info || echo "No existing egg info file, skipping deletion" | ||
| python3 setup.py sdist | ||
| python setup.py sdist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep python3 by default? I think the compatibility change was intentionally removed as of 04e99c1#diff-e77989ed21758e78331b20e477fc5582 to prepare the complete removal of Python 2 support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, we also do not need to add from __future__ import print_function in python/setup.py because it is always called by python3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for @HyukjinKwon 's advice.
HyukjinKwon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine
| pip3 install $PIP_PKGS && \ | ||
| cd && \ | ||
| virtualenv -p python3 /opt/p35 && \ | ||
| python /opt/p35 && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No such file or directory when building docker image:
Successfully built pypandoc pycparser
Installing collected packages: six, pycparser, cffi, cryptography, pyopenssl, pypandoc, numpy, Pygments, sphinxcontrib-applehelp, pytz, babel, snowballstemmer, imagesize, urllib3, certifi, chardet, idna, requests, sphinxcontrib-serializinghtml, sphinxcontrib-devhelp, sphinxcontrib-htmlhelp, alabaster, sphinxcontrib-jsmath, sphinxcontrib-qthelp, pyparsing, packaging, MarkupSafe, Jinja2, docutils, sphinx
Successfully installed Jinja2-2.10.3 MarkupSafe-1.1.1 Pygments-2.5.2 alabaster-0.7.12 babel-2.7.0 certifi-2019.11.28 cffi-1.13.2 chardet-3.0.4 cryptography-2.8 docutils-0.15.2 idna-2.8 imagesize-1.1.0 numpy-1.17.4 packaging-19.2 pycparser-2.19 pyopenssl-19.1.0 pypandoc-1.4 pyparsing-2.4.5 pytz-2019.3 requests-2.22.0 six-1.13.0 snowballstemmer-2.0.0 sphinx-2.2.2 sphinxcontrib-applehelp-1.0.1 sphinxcontrib-devhelp-1.0.1 sphinxcontrib-htmlhelp-1.0.2 sphinxcontrib-jsmath-1.0.1 sphinxcontrib-qthelp-1.0.2 sphinxcontrib-serializinghtml-1.1.3 urllib3-1.25.7
python: can't open file '/opt/p35': [Errno 2] No such file or directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan Can we remove virtualenv?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove python /opt/p35 && /opt/p35/bin/activate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems in order to activate virtualenv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this?
spark/dev/create-release/do-release-docker.sh
Lines 139 to 140 in 8d1b5ba
| # SPARK-24530: Sphinx must work with python 3 to generate doc correctly. | |
| echo "SPHINXPYTHON=/opt/p35/bin/python" >> $ENVFILE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @HyukjinKwon . We need to do it:
= Building documentation...
Command: /opt/spark-rm/release-build.sh docs
Log file: docs.log
Command FAILED. Check full logs for details.
Copying jquery.min.js from Scala API to Java API for page post-processing of badges
Copying api_javadocs.js to Java API for page post-processing of badges
Appending content of api-javadocs.css to JavaDoc stylesheet.css for badge styles
Moving to python/docs directory and building sphinx.
make: /opt/p35/bin/python: Command not found
/opt/p35/bin/python -msphinx -b html -d _build/doctrees . _build/html
make: /opt/p35/bin/python: Command not found
Makefile:80: recipe for target 'html' failed
make: *** [html] Error 127
jekyll 3.8.6 | Error: Python doc generation failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make: /opt/p35/bin/python: Command not found
Can we try to set SPHINXPYTHON=python and see if it works? Seems it's just because python was not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Seems right to revert two commits (and push both reverts into this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @cloud-fan.
|
Test build #115206 has finished for PR 26848 at commit
|
|
Test build #115204 has finished for PR 26848 at commit
|
|
Test build #115209 has finished for PR 26848 at commit
|
|
retest this please |
|
Test build #115228 has finished for PR 26848 at commit
|
|
retest this please |
|
Test build #115237 has finished for PR 26848 at commit
|
|
retest this please |
|
Test build #115242 has finished for PR 26848 at commit
|
HyukjinKwon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine if it worked - I haven't tested it by myself.
|
I tested it on the master branch. Do we need to test on branch-2.4? |
|
No~ This is for
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Merged to master.
Thank you so much, @wangyum , @srowen and @HyukjinKwon .
cc @gatorsmile and @cloud-fan

What changes were proposed in this pull request?
Why are the changes needed?
dev/make-distribution.shandpython/setup.pyare use python3.https://github.com/apache/spark/pull/26844/files#diff-ba2c046d92a1d2b5b417788bfb5cb5f8L236
https://github.com/apache/spark/pull/26330/files#diff-8cf6167d58ce775a08acafcfe6f40966
Does this PR introduce any user-facing change?
No.
How was this patch tested?
manual test:
Generated doc:
