You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/12/31 12:18:19 UTC
[GitHub] [spark] zhongjingxiong opened a new pull request #35082: [SPARK-37677][PYTHON] Decompress the ZIP file and grant the executable permission to the file
zhongjingxiong opened a new pull request #35082:
URL: https://github.com/apache/spark/pull/35082
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
`Unpacking an archive s3a://zjx/python3.6.9.zip#python3 from /tmp/spark-d861d788-bd72-4d3c-88fc-8f79b30b081d/python3.6.9.zip to /opt/spark/work-dir/./python3
Exception in thread "main" java.io.IOException: Cannot run program "python3/bin/python3": error=13, Permission denied
at java.base/java.lang.ProcessBuilder.start(Unknown Source)
at java.base/java.lang.ProcessBuilder.start(Unknown Source)
at org.apache.spark.deploy.PythonRunner$.main(PythonRunner.scala:97)
at org.apache.spark.deploy.PythonRunner.main(PythonRunner.scala)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
at java.base/java.lang.reflect.Method.invoke(Unknown Source)
at org.apache.spark.deploy.JavaMainApplication.start(SparkApplication.scala:52)
at org.apache.spark.deploy.SparkSubmit.org$apache$spark$deploy$SparkSubmit$$runMain(SparkSubmit.scala:955)
at org.apache.spark.deploy.SparkSubmit.doRunMain$1(SparkSubmit.scala:180)
at org.apache.spark.deploy.SparkSubmit.submit(SparkSubmit.scala:203)
at org.apache.spark.deploy.SparkSubmit.doSubmit(SparkSubmit.scala:90)
at org.apache.spark.deploy.SparkSubmit$$anon$2.doSubmit(SparkSubmit.scala:1045)
at org.apache.spark.deploy.SparkSubmit$.main(SparkSubmit.scala:1054)
at org.apache.spark.deploy.SparkSubmit.main(SparkSubmit.scala)
Caused by: java.io.IOException: error=13, Permission denied
at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
at java.base/java.lang.ProcessImpl.<init>(Unknown Source)
at java.base/java.lang.ProcessImpl.start(Unknown Source)
... 16 more`
When we set parameter "--archives hdfs:///user/zjx/python3.6.9.zip" to submit the Spark job, driver will unzip it, but it will lost executable permission after unzipping, so we should keep those permission. In this PR, I add the executable permission for all file after unzipping. It may cost some time to unzip that, but the time cost is controllable.
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
When we submit a py job and want to use our own version of Python，we may add "--archives hdfs:///user/zjx/python3.6.9.zip" ,after driver unzip this file, and want to execute the program using python3, it will report permission denied. So we should add executable permission to those script.
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
After the fix, the zip package submitted by the user for Python will run normally
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
First, I tested python3.6.9 and Python3.9. I compiled Python3.6.9 (which contains no other third party dependencies) and Python3.6.9 (which contains many third party dependencies) to include 8398 and 63570 files respectively and compressed them into a ZIP file, The zip file upload is then specified in spark-Submit, and the execution permission process is logged to record the time spent. The conclusion is that python3.9.zip took 50 to 65 milliseconds to find permissions, and the other one took 600 to 800 milliseconds.Then upload and test python3.6.9.zip and python3.6.9.tgz 100 times and find that the average unzip time of ZIP file is 15637.45 milliseconds, while the unzip time of TGZ file is 15758.78 milliseconds。That is to say, counting the time of adding execution permission, Zip also decompresses faster than TGZ.
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #35082: [SPARK-37677][PYTHON] Decompress the ZIP file and grant the executable permission to the file
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35082:
URL: https://github.com/apache/spark/pull/35082#discussion_r776997605
##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -605,6 +606,18 @@ private[spark] object Utils extends Logging {
}
}
+ def setExec(dest: File): Unit = {
+ val files = dest.listFiles()
+ for (i <- files) {
+ if (i.isDirectory) {
+ setExec(i)
+ }
+ else {
+ i.setExecutable(true)
Review comment:
No.. I think we shouldn't just change file permissions to be all executable blindly. Some of them might be intentionally not executable to prevent users mistakenly run. Is there any way to keep and restore the original file permissions?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 commented on a change in pull request #35082: [SPARK-37677][PYTHON] Decompress the ZIP file and grant the executable permission to the file
Posted by GitBox <gi...@apache.org>.
zero323 commented on a change in pull request #35082:
URL: https://github.com/apache/spark/pull/35082#discussion_r788772388
##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -605,6 +606,18 @@ private[spark] object Utils extends Logging {
}
}
+ def setExec(dest: File): Unit = {
+ val files = dest.listFiles()
+ for (i <- files) {
+ if (i.isDirectory) {
+ setExec(i)
+ }
+ else {
+ i.setExecutable(true)
Review comment:
I believe there is consensus here that, in the current form, it is no go. As far as I can tell Hadoop `FileUtil` doesn't seem to handle info-zip extensions, neither does `java.util.zip`. I am somewhat skeptical about bringing new dependencies just for the sake of this,especially, when better alternatives for distributing Python environments are already supported (pex, conda pack, and so on).
Python standard library [provides nice implementation](https://github.com/python/cpython/blob/f4c03484da59049eb62a9bf7777b963e2267d187/Lib/zipfile.py#L545) of metadata extraction, that supports, among other things, file permissions, but implementing it on our side could be an overkill.
If the problem is actually `"spark.pyspark.python` then we might consider changing permissions only for this single path, but not before we have more eyes on this, to be sure it won't backfire.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #35082: [SPARK-37677][PYTHON] Decompress the ZIP file and grant the executable permission to the file
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35082:
URL: https://github.com/apache/spark/pull/35082#issuecomment-1003504655
Can one of the admins verify this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] zero323 commented on a change in pull request #35082: [SPARK-37677][PYTHON] Decompress the ZIP file and grant the executable permission to the file
Posted by GitBox <gi...@apache.org>.
zero323 commented on a change in pull request #35082:
URL: https://github.com/apache/spark/pull/35082#discussion_r777228929
##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -605,6 +606,18 @@ private[spark] object Utils extends Logging {
}
}
+ def setExec(dest: File): Unit = {
+ val files = dest.listFiles()
+ for (i <- files) {
+ if (i.isDirectory) {
+ setExec(i)
+ }
+ else {
+ i.setExecutable(true)
Review comment:
Not to mention, it seems like a huge security risk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a change in pull request #35082: [SPARK-37677][PYTHON] Decompress the ZIP file and grant the executable permission to the file
Posted by GitBox <gi...@apache.org>.
itholic commented on a change in pull request #35082:
URL: https://github.com/apache/spark/pull/35082#discussion_r788308585
##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -605,6 +606,18 @@ private[spark] object Utils extends Logging {
}
}
+ def setExec(dest: File): Unit = {
+ val files = dest.listFiles()
+ for (i <- files) {
+ if (i.isDirectory) {
+ setExec(i)
+ }
+ else {
+ i.setExecutable(true)
Review comment:
+1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org