Skip to content

[SPARK-20704][SPARKR] change CRAN test to run single thread #17945

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[SPARK-20704][SPARKR] change CRAN test to run single thread #17945

wants to merge 6 commits into from

Conversation

@felixcheung
Copy link
Member

@felixcheung felixcheung commented May 11, 2017 •
edited
Loading

What changes were proposed in this pull request?

  • need to test by running R CMD check --as-cran
  • sanity check vignettes

How was this patch tested?

Jenkins

shivaram
shivaram reviewed May 11, 2017
View reviewed changes
Copy link
Contributor

@shivaram shivaram left a comment

Choose a reason for hiding this comment

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

Thanks @felixcheung -- I just had one major comment on pulling out the OS_Type into a separate PR

R/pkg/DESCRIPTION Outdated
License: Apache License (== 2.0)
URL: http://www.apache.org/ http://spark.apache.org/
BugReports: http://spark.apache.org/contributing.html
OS_Type: unix
Copy link
Contributor

@shivaram shivaram May 11, 2017

Choose a reason for hiding this comment

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

Can we do this in a separate PR ? It'll be good to track this as a separate JIRA / PR just to lookup why we did this. Also I'll hopefully hear from the winbuilder maintainer if this is something we can avoid doing.

Copy link
Member Author

@felixcheung felixcheung May 11, 2017

Choose a reason for hiding this comment

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

right, I can revert this before merging. I want to have a branch to test devtools install

R/pkg/inst/tests/testthat/jarTest.R
library(SparkR)

sc <- sparkR.session()
sc <- sparkR.session(master = "local[1]")
Copy link
Contributor

@shivaram shivaram May 11, 2017

Choose a reason for hiding this comment

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

any specific reason these dont use sparkRMaster variable ?

Copy link
Member Author

@felixcheung felixcheung May 11, 2017

Choose a reason for hiding this comment

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

it's not accessible here - this is a separate R app

Copy link
Contributor

@shivaram shivaram May 11, 2017

Choose a reason for hiding this comment

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

Ah got it.

R/pkg/inst/tests/testthat/test_Serde.R Outdated
context("SerDe functionality")

sparkSession <- sparkR.session(enableHiveSupport = FALSE)
sparkSession <- sparkR.session(master = sparkRMaster, enableHiveSupport = FALSE)
Copy link
Contributor

@shivaram shivaram May 11, 2017

Choose a reason for hiding this comment

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

Minor nit - Is sparkRTestMaster is a better name ?

@SparkQA
Copy link

SparkQA commented May 11, 2017

Test build #76778 has finished for PR 17945 at commit 468cef1.

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

@SparkQA
Copy link

SparkQA commented May 11, 2017

Test build #76780 has finished for PR 17945 at commit 92e6bba.

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

@shivaram
Copy link
Contributor

shivaram commented May 11, 2017

Actually one more thing to do is to change the vignettes to use 1 core as well since they get rebuilt / checked during the CRAN check.

@felixcheung
Copy link
Member Author

felixcheung commented May 11, 2017

yes, it's here https://github.com/apache/spark/pull/17945/files#diff-56790892076daac0bc43a9b38c0b1470R51

@SparkQA
Copy link

SparkQA commented May 11, 2017

Test build #76821 has finished for PR 17945 at commit a718645.

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

@felixcheung
Copy link
Member Author

felixcheung commented May 11, 2017

tested devtools install on Windows (even with OS_type unix)

@felixcheung
Copy link
Member Author

felixcheung commented May 11, 2017 •
edited
Loading

hmm, I'm getting a NOTE from R CMD check

Unknown, possibly mis-spelled, fields in DESCRIPTION:
  ‘OS_Type’

@shivaram
Copy link
Contributor

shivaram commented May 11, 2017

I think its a lowercase t

@shivaram
Copy link
Contributor

shivaram commented May 11, 2017

Can you also retest with devtools after the fix ?

@felixcheung
Copy link
Member Author

felixcheung commented May 11, 2017

checked vignettes

@felixcheung
Copy link
Member Author

felixcheung commented May 11, 2017 •
edited
Loading

bummer, no one can install on windows then with OS_type: unix

Downloading GitHub repo felixcheung/spark@rchangesforpackage
from URL https://api.github.com/repos/felixcheung/spark/zipball/rchangesforpackage
Installing SparkR

ERROR:  Unix-only package
Error: Command failed (1)

@shivaram
Copy link
Contributor

shivaram commented May 11, 2017

Ah that is a bummer. Lets hold off on that and use a new JIRA to discuss it. Rest of the changes LGTM pending jenkins, appveyor

@SparkQA
Copy link

SparkQA commented May 11, 2017

Test build #76823 has finished for PR 17945 at commit 21bea08.

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

@SparkQA
Copy link

SparkQA commented May 11, 2017

Test build #76824 has finished for PR 17945 at commit 5aa7fd4.

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

@felixcheung
Copy link
Member Author

felixcheung commented May 12, 2017

AppVeyor was failing because of the SparkListenerBus issue.

@felixcheung
Copy link
Member Author

felixcheung commented May 12, 2017

merged to master/2.2

asfgit pushed a commit that referenced this pull request May 12, 2017
@felixcheung
[SPARK-20704][SPARKR] change CRAN test to run single thread
c1e5ac2 
## What changes were proposed in this pull request?

- [x] need to test by running R CMD check --as-cran
- [x] sanity check vignettes

## How was this patch tested?

Jenkins

Author: Felix Cheung <[email protected]>

Closes #17945 from felixcheung/rchangesforpackage.

(cherry picked from commit 888b84a)
Signed-off-by: Felix Cheung <[email protected]>
@asfgit asfgit closed this in 888b84a May 12, 2017
robert3005 pushed a commit to palantir/spark that referenced this pull request May 19, 2017
@felixcheung
[SPARK-20704][SPARKR] change CRAN test to run single thread
a9e55ca 
## What changes were proposed in this pull request?

- [x] need to test by running R CMD check --as-cran
- [x] sanity check vignettes

## How was this patch tested?

Jenkins

Author: Felix Cheung <[email protected]>

Closes apache#17945 from felixcheung/rchangesforpackage.
liyichao pushed a commit to liyichao/spark that referenced this pull request May 24, 2017
@felixcheung @liyichao
[SPARK-20704][SPARKR] change CRAN test to run single thread
6ab97e8 
## What changes were proposed in this pull request?

- [x] need to test by running R CMD check --as-cran
- [x] sanity check vignettes

## How was this patch tested?

Jenkins

Author: Felix Cheung <[email protected]>

Closes apache#17945 from felixcheung/rchangesforpackage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@shivaram shivaram shivaram left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@felixcheung @SparkQA @shivaram