[SPARK-22344][SPARKR] clean up install dir if running test as source package #19657

felixcheung · 2017-11-04T08:17:02Z

What changes were proposed in this pull request?

remove spark if spark downloaded & installed

How was this patch tested?

manually by building package
Jenkins, AppVeyor

felixcheung · 2017-11-04T08:18:55Z

odd build failure

[EnvInject] - Variables injected successfully.
[SparkPullRequestBuilder] $ /bin/bash /tmp/hudson3188739775964134398.sh
fixing target dir permissions
chmod: cannot access `target/*': No such file or directory
running git clean -fdx
Python versions prior to 2.7 are not supported.

felixcheung · 2017-11-04T08:19:04Z

Jenkins, retest this please

felixcheung · 2017-11-04T08:20:08Z

need to update vignettes build... and realizing that we are downloading the spark jar twice, once for test and once for vignettes

HyukjinKwon · 2017-11-04T11:14:07Z

retest this please

SparkQA · 2017-11-04T12:06:35Z

Test build #83442 has finished for PR 19657 at commit d4433e1.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

felixcheung · 2017-11-05T04:56:07Z

retest this please

SparkQA · 2017-11-05T05:43:36Z

Test build #83458 has finished for PR 19657 at commit 0ea7c9b.

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

felixcheung · 2017-11-05T20:20:00Z

@shivaram could you take a look? I think this would do it

shivaram

Thanks @felixcheung - I think the approach looks good. I had some minor questions

shivaram · 2017-11-06T01:21:44Z

R/pkg/R/install.R

@@ -152,6 +152,9 @@ install.spark <- function(hadoopVersion = "2.7", mirrorUrl = NULL,
                     })
  if (!tarExists || overwrite || !success) {
    unlink(packageLocalPath)
+    if (success) {


why should this be in inside this if block for overwrite || !success -- Can't we just have it outside this if as an else below ?

basically if success is TRUE then only the other cases matter: !tarExists || overwrite - that's the exact condition where the download would have occurred in L126 which is the else case of tarExists && !overwrite on L123

Hmm ok that still looks weird in the code. Maybe add a comment before this of the form If we downloaded a tarfile or overwrote it, set sparkDownloaded flag ?

shivaram · 2017-11-06T01:31:05Z

R/pkg/vignettes/sparkr-vignettes.Rmd

+```{r cleanup, include=FALSE}
+# clean up if Spark was downloaded
+# get0 not supported before R 3.2.0
+sparkDownloaded <- mget(".sparkDownloaded"[1L],


Can we make this an internal util function and call it from here ?

yes! but both call sites are outside of the package technically and we would need to call the private function with SparkR:::, which is kinda ugly...

I guess we could wrap the entire cleanup thing into a private/internal function (since it has to access a private flag anyway..)

Yeah - lets do that. I might even be fine with exposing an external function 'uninstallSpark' or 'uninstallDownloadedSpark' ?

since this needs to go into 2.2, let's not add a public method for now, we could revisit this for 2.3

shivaram · 2017-11-06T01:32:48Z

R/pkg/tests/run-all.R

+  parentDir <- SparkR:::sparkCachePath()
+  dirs <- list(parentDir, dirname(parentDir), dirname(dirname(parentDir)))
+  lapply(dirs, function(d) {
+    if (length(list.files(d, all.files = TRUE, include.dirs = TRUE, no.. = TRUE)) == 0) {


One consequence of this is that if we run R CMD check --as-cran we will do the download twice -- once for the unit tests and once for the vignettes

yes, it would, as commented above #19657 (comment)
problem is we have no idea whether the vignettes build is going to happen or not (it could easily be disabled via commandline)

SparkQA · 2017-11-07T11:41:48Z

Test build #83542 has finished for PR 19657 at commit 31f3bd0.

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

SparkQA · 2017-11-07T11:45:01Z

Test build #83544 has finished for PR 19657 at commit ca5349b.

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

shivaram · 2017-11-07T16:57:56Z

R/pkg/tests/fulltests/test_utils.R

+                "c:\\Users\\user\\AppData\\Local\\Apache\\Spark",
+                "c:\\Users\\user\\AppData\\Local\\Apache")
+  } else {
+    dirs <- traverseParentDirs("/Users/user/Library/Caches/spark/spark2.2", 1)


can we also test the linux one (/home/user/.cache) - Just want to make sure we will not miss hidden files / directories.

sure, but well hopefully the implementation is not platform dependent, otherwise we will need to test linux as well as osx
(and it doesn't check if the path is valid/present)

shivaram · 2017-11-07T16:59:35Z

Thanks @felixcheung -- The Appveyor test seems to have failed with the following err

1. Failure: traverseParentDirs (@test_utils.R#255) -----------------------------
`dirs` not equal to `expect`.
3/4 mismatches
x[2]: "c:/Users/user/AppData/Local/Apache/Spark/Cache"
y[2]: "c:\\Users\\user\\AppData\\Local\\Apache\\Spark\\Cache"
x[3]: "c:/Users/user/AppData/Local/Apache/Spark"
y[3]: "c:\\Users\\user\\AppData\\Local\\Apache\\Spark"
x[4]: "c:/Users/user/AppData/Local/Apache"
y[4]: "c:\\Users\\user\\AppData\\Local\\Apache"

@HyukjinKwon could you also take a quick look at this PR ?

SparkQA · 2017-11-07T18:26:09Z

Test build #83553 has finished for PR 19657 at commit f2aa5b7.

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

SparkQA · 2017-11-07T18:33:43Z

Test build #83554 has finished for PR 19657 at commit f21a90b.

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

shivaram · 2017-11-07T20:11:11Z

AppVeyor still has an error

1. Failure: traverseParentDirs (@test_utils.R#252) -----------------------------
`dirs` not equal to `expect`.
1/4 mismatches
x[1]: "c:\\Users\\user\\AppData\\Local\\Apache\\Spark\\Cache\\spark2.2"
y[1]: "c:/Users/user/AppData/Local/Apache/Spark/Cache/spark2.2"

HyukjinKwon · 2017-11-07T23:36:11Z

Will take a look within today (KST).

HyukjinKwon · 2017-11-08T04:57:42Z

Yup, I just checked it too and was writing a comment .. The current change should pass :).

SparkQA · 2017-11-08T05:49:48Z

Test build #83582 has finished for PR 19657 at commit 18e238a.

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

felixcheung · 2017-11-08T17:29:51Z

build failure this time

felixcheung · 2017-11-09T06:48:19Z

@HyukjinKwon hey I think the appveyor test pass is just timing out after 1 hr 30 min - is there a way to up the timeout?

HyukjinKwon · 2017-11-09T07:07:57Z

I actually took a look to decrease the build time (as you know) and am currently away from it. If I remember correctly, what I observed was that a single(?) particular test takes 20ish(?) mins. It was related with ML in R.

Let me try to take a look again first and will leave some comments about what I investigated in SPARK-21693 if I can't deal with it by myself (probably by my limited ML knowledge).

If that's actually not that quite simple, then, let me ask it to increase 2 hours (like my own account).
In AppVeyor, sounds they actually recommend to separate the build as I proposed in the JIRA or reduce the time ..

felixcheung · 2017-11-09T07:43:32Z

ok thanks, in that case, would you mind cherry pick these changes into your account to run under appveyor - fixing test run is lower priority than getting this merged to kick off 2.2.1... :) thanks

HyukjinKwon · 2017-11-09T08:26:08Z

Sure, I can! .. but seems something has gone wrong with package installiation .. Let me trigger it by my account anyway. I can retrigger it.

HyukjinKwon · 2017-11-09T08:29:16Z

Build started: [SparkR] ALL
Diff: master...spark-test:33DABC16-224C-40D8-8A29-EF9CFA72625E

felixcheung · 2017-11-09T08:35:59Z

ouch

Error in packageVersion("knitr") : package 'knitr' not found
[00:03:37] Execution halted

HyukjinKwon · 2017-11-09T21:11:12Z

Looks mine was set to 1.5 hours back lately ..

HyukjinKwon · 2017-11-09T21:16:48Z

Oh but the test passed now with Apache account here.

…package ## What changes were proposed in this pull request? remove spark if spark downloaded & installed ## How was this patch tested? manually by building package Jenkins, AppVeyor Author: Felix Cheung <felixcheung_m@hotmail.com> Closes #19657 from felixcheung/rinstalldir. (cherry picked from commit b70aa9e) Signed-off-by: Felix Cheung <felixcheung@apache.org>

felixcheung · 2017-11-10T18:24:17Z

thanks! merged to master, 2.2.
2.1 attempt had conflict, so leaving that out for now.

…package ## What changes were proposed in this pull request? remove spark if spark downloaded & installed ## How was this patch tested? manually by building package Jenkins, AppVeyor Author: Felix Cheung <felixcheung_m@hotmail.com> Closes apache#19657 from felixcheung/rinstalldir. (cherry picked from commit b70aa9e) Signed-off-by: Felix Cheung <felixcheung@apache.org>

add flag to cleanup

d4433e1

no get0

0ea7c9b

shivaram reviewed Nov 6, 2017

View reviewed changes

felixcheung added 3 commits November 7, 2017 02:27

make into function

d0064ca

fix lint

31f3bd0

comment

ca5349b

shivaram reviewed Nov 7, 2017

View reviewed changes

felixcheung added 3 commits November 7, 2017 09:31

fix windows

f2aa5b7

more test

90d36c9

fix

f21a90b

fix 2

18e238a

felixcheung closed this Nov 8, 2017

felixcheung reopened this Nov 8, 2017

felixcheung closed this Nov 9, 2017

felixcheung reopened this Nov 9, 2017

felixcheung closed this Nov 9, 2017

felixcheung reopened this Nov 9, 2017

asfgit closed this in b70aa9e Nov 10, 2017

[SPARK-22344][SPARKR] clean up install dir if running test as source package #19657

[SPARK-22344][SPARKR] clean up install dir if running test as source package #19657

Conversation

felixcheung commented Nov 4, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

felixcheung commented Nov 4, 2017

felixcheung commented Nov 4, 2017

felixcheung commented Nov 4, 2017

HyukjinKwon commented Nov 4, 2017

SparkQA commented Nov 4, 2017

felixcheung commented Nov 5, 2017

SparkQA commented Nov 5, 2017

felixcheung commented Nov 5, 2017

shivaram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 7, 2017

SparkQA commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivaram commented Nov 7, 2017

SparkQA commented Nov 7, 2017

SparkQA commented Nov 7, 2017

shivaram commented Nov 7, 2017

HyukjinKwon commented Nov 7, 2017 • edited Loading

HyukjinKwon commented Nov 8, 2017

SparkQA commented Nov 8, 2017

felixcheung commented Nov 8, 2017

felixcheung commented Nov 9, 2017

HyukjinKwon commented Nov 9, 2017 • edited Loading

felixcheung commented Nov 9, 2017

HyukjinKwon commented Nov 9, 2017 • edited Loading

HyukjinKwon commented Nov 9, 2017

felixcheung commented Nov 9, 2017

HyukjinKwon commented Nov 9, 2017

HyukjinKwon commented Nov 9, 2017

felixcheung commented Nov 10, 2017

felixcheung commented Nov 4, 2017 •

edited

Loading

HyukjinKwon commented Nov 7, 2017 •

edited

Loading

HyukjinKwon commented Nov 9, 2017 •

edited

Loading

HyukjinKwon commented Nov 9, 2017 •

edited

Loading