[SPARK-8603][SPARKR] Use shell() instead of system2() for SparkR on Windows #13165

HyukjinKwon · 2016-05-18T08:09:13Z

What changes were proposed in this pull request?

This PR corrects SparkR to use shell() instead of system2() on Windows.

Using system2(...) on Windows does not process windows file separator \. shell(tralsate = TRUE, ...) can treat this problem. So, this was changed to be chosen according to OS.

Existing tests were failed on Windows due to this problem. For example, those were failed.

8. Failure: sparkJars tag in SparkContext (@test_includeJAR.R#34)
9. Failure: sparkJars tag in SparkContext (@test_includeJAR.R#36)

The cases above were due to using of system2.

In addition, this PR also fixes some tests failed on Windows.

5. Failure: sparkJars sparkPackages as comma-separated strings (@test_context.R#128) 
6. Failure: sparkJars sparkPackages as comma-separated strings (@test_context.R#131) 
7. Failure: sparkJars sparkPackages as comma-separated strings (@test_context.R#134)

The cases above were due to a weird behaviour of normalizePath(). On Linux, if the path does not exist, it just prints out the input but it prints out including the current path on Windows.

# On Linus
path <- normalizePath("aa")
print(path)
[1] "aa"

# On Windows
path <- normalizePath("aa")
print(path)
[1] "C:\\Users\\aa"

How was this patch tested?

Jenkins tests and manually tested in a Window machine as below:

Here is the stdout of testing.

Closes #7025

…Java and Scripts from R

HyukjinKwon · 2016-05-18T08:09:28Z

Please let me cc @sun-rui and @JoshRosen

sun-rui · 2016-05-18T08:18:07Z

Could you add a test case for Windows specific test? @shivaram

SparkQA · 2016-05-18T08:32:30Z

Test build #58758 has finished for PR 13165 at commit 454b7f5.

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

SparkQA · 2016-05-18T08:47:16Z

Test build #58760 has finished for PR 13165 at commit 3057844.

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

srowen · 2016-05-18T08:51:17Z

R/pkg/R/client.R

+# of the operating system. So, this should be manaully changed.
+determinefileSeperator <- function() {
+  if (.Platform$OS.type == "windows") {
+    fileSeperator <- "\\"


Nit: separator not seperator

Thank you so much.

SparkQA · 2016-05-18T09:16:08Z

Test build #58763 has finished for PR 13165 at commit 935d796.

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

felixcheung · 2016-05-19T03:03:52Z

Does this apply to other cases:

spark/R/pkg/inst/worker/daemon.R

Line 22 in d6dc12e

script <- file.path(dirs[[1]], "SparkR", "worker", "worker.R")

spark/R/pkg/inst/profile/shell.R

Line 20 in d6dc12e

.libPaths(c(file.path(home, "R", "lib"), .libPaths()))

spark/examples/src/main/r/dataframe.R

Line 37 in 6ab4d9e

    
           path <- file.path(Sys.getenv("SPARK_HOME"), "examples/src/main/resources/people.json")

(last one is an example)

HyukjinKwon · 2016-05-19T04:19:43Z

@sun-rui @felixcheung Let me try to build and run all tests for R first in Windows and then will try to correct and add each test one by one.

This will take a bit of time and I might have to ask a lot of questions (I am not familiar with R and Windows as I said in the stale PR above, #7025) but anyway I will try.

Meanwhile, If anybody wants to take over this, please do so.

sun-rui · 2016-05-19T06:19:20Z

@felixcheung, this issue seems to relate to system2() only. However, let's wait for HyukjinKwon's test result.

@HyukjinKwon, great, go ahead please.

HyukjinKwon · 2016-05-20T04:34:48Z

@sun-rui @felixcheung Right. It seems finally I made it. I made gists and uploaded a PDF file of Spark UI to my google drive.

Let me tell you the test results first. The test was proceeded against master branch.

Here is the stdout output for the tests on Windwos 7 32bit, output.msg.
Here is the stderr output for the tests on Windwos 7 32bit, output.err.
Here is the PDF for Spark UI PDF (Sorry, it seems some history are truncated. I will make a proper one if I have to run this again)

Here are the steps I took:

I run tests after building Spark on Windows according to ./R/WINDOWS.md
It seems $HADOOP_HOME should be set. So, I added the variable in my system.
It seems winutils.exe is required which is included in Hadoop official binary although it reads files in the local file system during the tests.

And run the tests by the command below (after setting log levels to ERROR in log4j.properties because it was so verbose):

cd bin
spark-submit2.cmd --conf spark.hadoop.fs.defualt.name="file:///" ..\R\pkg\tests\run-all.R > output.msg 2> output.err

HyukjinKwon · 2016-05-20T04:36:32Z

This raises some questions to me.

It seems several tests were failed. Could you please inform me your thoughts?
Now, I think I can add some tests but could you please tell me where I should write the related tests and maybe rough ideas of the tests I should add?
Can I separately update ./R/WINDOWS.md in another PR to describe how I tested this on Winodws as above?

sun-rui · 2016-05-20T05:39:23Z

A rough scan of the test failures shows most of them are probably related to path handling. You can replay the failed test case in R on Windows. For debug facilities in R, refer to http://www.inside-r.org/r-doc/base/traceback, https://stat.ethz.ch/R-manual/R-devel/library/base/html/debug.html, http://www.inside-r.org/r-doc/base/browser
You can add a new file named test_Windows.R under R/pkg/inst/tests/testthat
Sure.

sun-rui · 2016-05-20T06:36:08Z

I checked the source code of system2. Since a batch file with .cmd extension is to be launched on Windows, I think it is more formal to use shell() instead of system2(). Refer to http://www.astrostatistics.psu.edu/datasets/R/html/base/html/shell.html. The proposed fix is like follows:

if (.Platform$OS.type == "windows") {
  shell(sparkSubmitBin, translate = TRUE, ...)
} else {
  system2(sparkSubmitBin, ...)
}

HyukjinKwon · 2016-05-20T07:07:03Z

@sun-rui Thank you so much. I will try to add a test.

shivaram · 2016-05-24T00:16:31Z

Related to my comment on #13217 -- I will test this out on windows using the new instructions.

HyukjinKwon · 2016-05-24T00:18:41Z

@shivaram Thank you so much. Let me try to add a test meanwhile.

HyukjinKwon · 2016-05-25T13:29:28Z

@shivaram and @sun-rui I addressed the comment, added a test for this PR and fixed some tests not working on Windows due to path separator.

This PR also fixes the test failures below.

Fixed case 1

5. Failure: sparkJars sparkPackages as comma-separated strings (@test_context.R#128) 
6. Failure: sparkJars sparkPackages as comma-separated strings (@test_context.R#131) 
7. Failure: sparkJars sparkPackages as comma-separated strings (@test_context.R#134)

The cases above were due to a weird behaviour of normalizePath(). On Linux, if the path does not exist, it just prints out the input but it prints out including the current path on Windows.

# On Linus
path <- normalizePath("aa")
print(path)
[1] "aa"

# On Windows
path <- normalizePath("aa")
print(path)
[1] "C:\\Users\\aa"

Fixed case 2

8. Failure: sparkJars tag in SparkContext (@test_includeJAR.R#34)
9. Failure: sparkJars tag in SparkContext (@test_includeJAR.R#36)

The cases above were due to using of system2.

Here is full stdout of my testing, output.msg on Windows.

SparkQA · 2016-05-25T13:35:10Z

Test build #59275 has finished for PR 13165 at commit 64141d2.

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

SparkQA · 2016-05-25T13:50:03Z

Test build #59276 has finished for PR 13165 at commit 4a53360.

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

sun-rui · 2016-05-25T13:54:34Z

R/pkg/R/client.R

@@ -60,6 +60,15 @@ generateSparkSubmitArgs <- function(args, sparkHome, jars, sparkSubmitOpts, pack
  combinedArgs
 }

+determineLauncher <- function(sparkSubmitBin, combinedArgs, capture = FALSE) {


can this be a common utility function and be re-used in test_includeJAR.R?

Sure. I will.

BTW, could you inform me if I can disable style checking for some blocks in R like Scala? It seems it fails due to the usages of shell.

You can use nolint - See context.R for an example. BTW what is the lint error here ?

@shivaram For some reasons, it seems there is no mothod shell() on Linux (I tested this on my Mac) but on Windows. I am using this twice within this PR. Here is a thread for this, http://r.789695.n4.nabble.com/Shell-Function-not-on-Linux-td4672569.html.

I could not find the official documentation though. Let me address the comments first.

the common function could be like:

launchScript(script) { if (windowsOS) use shell() else (system2() }

This makes it more general, not binding to launching SparkSubmit

@sun-rui Thank you so much. I was trying to come up with a good name.

SparkQA · 2016-05-26T04:09:47Z

Test build #59339 has finished for PR 13165 at commit 0482ebb.

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

sun-rui · 2016-05-26T05:57:16Z

R/pkg/inst/tests/testthat/test_includeJAR.R

-  res <- system2(command = submitPath,
-                 args = c(jarPath, scriptPath),
-                 stdout = TRUE)
+  if (.Platform$OS.type == "windows") {


you can call determineSparkSubmitBin() here

HyukjinKwon · 2016-05-26T08:03:30Z

@sun-rui Thanks, I just updated it. Here is the stdout for this time

SparkQA · 2016-05-26T08:23:32Z

Test build #59363 has finished for PR 13165 at commit a1ccd5f.

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

sun-rui · 2016-05-26T08:46:05Z

LGTM

sun-rui · 2016-05-26T08:48:10Z

could you rebase it to latest master. It is hopeful other test failures will gone

HyukjinKwon · 2016-05-26T08:50:16Z

Sure, Thanks!

SparkQA · 2016-05-26T09:08:45Z

Test build #59367 has finished for PR 13165 at commit 7eadefd.

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

SparkQA · 2016-05-26T13:55:31Z

Test build #59383 has finished for PR 13165 at commit 521f07b.

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

shivaram · 2016-05-27T00:20:16Z

@HyukjinKwon Thanks for the update. I verified this today using a Windows VM and followed the instructions you have written up as well. All the tests passed and the stdout I got is at https://gist.github.com/shivaram/bd3c8bf7466d6cd515f4877f2a9611b3

One minor thing - there is a typo in the last line of https://github.com/apache/spark/blob/branch-2.0/R/WINDOWS.md
spark.hadoop.fs.defualt.name should be spark.hadoop.fs.default.name (typo in the spelling of default).

Could you make that change as a part of this PR ?

HyukjinKwon · 2016-05-27T00:26:26Z

@shivaram Thank you so much! I will keep this machine as it is anyway. Please cc me if there are some PRs dealing with Windows in the future. I can run tests and leave some comments.

SparkQA · 2016-05-27T00:49:52Z

Test build #59439 has finished for PR 13165 at commit 752c40a.

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

shivaram · 2016-05-27T03:54:21Z

Merging this to master and branch-2.0

…indows ## What changes were proposed in this pull request? This PR corrects SparkR to use `shell()` instead of `system2()` on Windows. Using `system2(...)` on Windows does not process windows file separator `\`. `shell(tralsate = TRUE, ...)` can treat this problem. So, this was changed to be chosen according to OS. Existing tests were failed on Windows due to this problem. For example, those were failed. ``` 8. Failure: sparkJars tag in SparkContext (test_includeJAR.R#34) 9. Failure: sparkJars tag in SparkContext (test_includeJAR.R#36) ``` The cases above were due to using of `system2`. In addition, this PR also fixes some tests failed on Windows. ``` 5. Failure: sparkJars sparkPackages as comma-separated strings (test_context.R#128) 6. Failure: sparkJars sparkPackages as comma-separated strings (test_context.R#131) 7. Failure: sparkJars sparkPackages as comma-separated strings (test_context.R#134) ``` The cases above were due to a weird behaviour of `normalizePath()`. On Linux, if the path does not exist, it just prints out the input but it prints out including the current path on Windows. ```r # On Linus path <- normalizePath("aa") print(path) [1] "aa" # On Windows path <- normalizePath("aa") print(path) [1] "C:\\Users\\aa" ``` ## How was this patch tested? Jenkins tests and manually tested in a Window machine as below: Here is the [stdout](https://gist.github.com/HyukjinKwon/4bf35184f3a30f3bce987a58ec2bbbab) of testing. Closes #7025 Author: hyukjinkwon <[email protected]> Author: Hyukjin Kwon <[email protected]> Author: Prakash PC <[email protected]> Closes #13165 from HyukjinKwon/pr/7025. (cherry picked from commit 1c40373) Signed-off-by: Shivaram Venkataraman <[email protected]>

…ting on Windows (currently SparkR only) ## What changes were proposed in this pull request? This PR adds the build automation on Windows with [AppVeyor](https://www.appveyor.com/) CI tool. Currently, this only runs the tests for SparkR as we have been having some issues with testing Windows-specific PRs (e.g. apache#14743 and apache#13165) and hard time to verify this. One concern is, this build is dependent on [steveloughran/winutils](https://github.com/steveloughran/winutils) for pre-built Hadoop bin package (who is a Hadoop PMC member). ## How was this patch tested? Manually, https://ci.appveyor.com/project/HyukjinKwon/spark/build/88-SPARK-17200-build-profile This takes roughly 40 mins. Some tests are already being failed and this was found in apache#14743 (comment). Author: hyukjinkwon <[email protected]> Closes apache#14859 from HyukjinKwon/SPARK-17200-build.

pcp-dev and others added 3 commits June 25, 2015 11:29

[SPARK-8603] [sparkR] In windows, Incorrect file seperator passed to …

6f0f5ab

…Java and Scripts from R

Fetch upstream

24e37e1

Correct condition, fix style and comments

454b7f5

Fetch upstream

201887d

Update indentation

3057844

srowen reviewed May 18, 2016
View reviewed changes

Fix typo

935d796

HyukjinKwon mentioned this pull request May 21, 2016

[MINOR][SPARKR][DOC] Add a description for running unit tests in Windows #13217

Closed

HyukjinKwon added 2 commits May 25, 2016 21:10

Address comments, fix failing tests on windos, add a test.

6f0933d

Skip Windows test on Linux and repleace jars to list

64141d2

Fix R style

4a53360

sun-rui reviewed May 25, 2016
View reviewed changes

HyukjinKwon changed the title ~~[SPARK-8603][SPARKR] Incorrect file separator passed to Java and Scripts from R in windows~~ [SPARK-8603][SPARKR] Incorrect paths being used for Spark R on Windows May 26, 2016

HyukjinKwon changed the title ~~[SPARK-8603][SPARKR] Incorrect paths being used for Spark R on Windows~~ [SPARK-8603][SPARKR] Use shell() instead of system2() for SparkR on Windows May 26, 2016

sun-rui reviewed May 26, 2016
View reviewed changes

HyukjinKwon added 2 commits May 26, 2016 15:55

Reuse determineSparkSubmitBin()

a3ccac6

Fix style and correct arguments

a1ccd5f

Fetch upstream

7eadefd

Change test context name

521f07b

Fix a typo

752c40a

asfgit closed this in 1c40373 May 27, 2016

HyukjinKwon mentioned this pull request Aug 29, 2016

[SPARK-17200][PROJECT INFRA][BUILD][SPARKR] Automate building and testing on Windows (currently SparkR only) #14859

Closed

HyukjinKwon mentioned this pull request Sep 8, 2016

[SPARK-17339][CORE][BRANCH-2.0] Do not use path to get a filesystem in hadoopFile and newHadoopFile APIs #15008

Closed

HyukjinKwon deleted the pr/7025 branch January 2, 2018 03:42

[SPARK-8603][SPARKR] Use shell() instead of system2() for SparkR on Windows #13165

[SPARK-8603][SPARKR] Use shell() instead of system2() for SparkR on Windows #13165

Conversation

HyukjinKwon commented May 18, 2016 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

HyukjinKwon commented May 18, 2016

sun-rui commented May 18, 2016

SparkQA commented May 18, 2016

SparkQA commented May 18, 2016

srowen May 18, 2016

Choose a reason for hiding this comment

HyukjinKwon May 18, 2016

Choose a reason for hiding this comment

SparkQA commented May 18, 2016

felixcheung commented May 19, 2016

HyukjinKwon commented May 19, 2016 • edited Loading

sun-rui commented May 19, 2016

HyukjinKwon commented May 20, 2016 • edited Loading

HyukjinKwon commented May 20, 2016 • edited Loading

sun-rui commented May 20, 2016

sun-rui commented May 20, 2016

HyukjinKwon commented May 20, 2016

shivaram commented May 24, 2016

HyukjinKwon commented May 24, 2016

HyukjinKwon commented May 25, 2016 • edited Loading

SparkQA commented May 25, 2016

SparkQA commented May 25, 2016

sun-rui May 25, 2016

Choose a reason for hiding this comment

HyukjinKwon May 25, 2016 • edited Loading

Choose a reason for hiding this comment

shivaram May 25, 2016

Choose a reason for hiding this comment

HyukjinKwon May 26, 2016 • edited Loading

Choose a reason for hiding this comment

sun-rui May 26, 2016

Choose a reason for hiding this comment

HyukjinKwon May 26, 2016

Choose a reason for hiding this comment

SparkQA commented May 26, 2016

sun-rui May 26, 2016

Choose a reason for hiding this comment

HyukjinKwon commented May 26, 2016 • edited Loading

SparkQA commented May 26, 2016

sun-rui commented May 26, 2016

sun-rui commented May 26, 2016

HyukjinKwon commented May 26, 2016

SparkQA commented May 26, 2016

SparkQA commented May 26, 2016

shivaram commented May 27, 2016

HyukjinKwon commented May 27, 2016

SparkQA commented May 27, 2016

shivaram commented May 27, 2016

HyukjinKwon commented May 18, 2016 •

edited

Loading

HyukjinKwon commented May 19, 2016 •

edited

Loading

HyukjinKwon commented May 20, 2016 •

edited

Loading

HyukjinKwon commented May 20, 2016 •

edited

Loading

HyukjinKwon commented May 25, 2016 •

edited

Loading

HyukjinKwon May 25, 2016 •

edited

Loading

HyukjinKwon May 26, 2016 •

edited

Loading

HyukjinKwon commented May 26, 2016 •

edited

Loading