Skip to content

[SPARK-8506] Add pakages to R context created through init. #6928

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-8506] Add pakages to R context created through init. #6928

wants to merge 6 commits into from

Conversation

@holdenk
Copy link
Contributor

@holdenk holdenk commented Jun 22, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Jun 22, 2015

Test build #35420 has finished for PR 6928 at commit c818556.

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

@holdenk
Copy link
Contributor Author

holdenk commented Jun 22, 2015

jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Jun 22, 2015

Test build #35425 has finished for PR 6928 at commit c818556.

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

shivaram
shivaram reviewed Jun 22, 2015
View reviewed changes
R/pkg/R/sparkR.R Outdated
Copy link
Contributor

@shivaram shivaram Jun 22, 2015

Choose a reason for hiding this comment

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

space before and after = here

@shivaram
Copy link
Contributor

shivaram commented Jun 22, 2015

Thanks @holdenk -- Change looks pretty good. I just had some minor style comments. BTW do you know if we re-use some test spark package to create a unit test for this ? Using a real package might mean going to maven central etc. which doesn't seem like a very good for a unit test, but I was wondering if we had code to generat some fake test package for Scala unit tests.

cc @brkyvz

@brkyvz
Copy link
Contributor

brkyvz commented Jun 22, 2015

Hey @shivaram and @holdenk! I have a lot of package related utils here. The most relevant method I believe would be withRepository. You may take a look here for examples on how to use them. I strongly suggest against using a package from Maven Central, as they lead to a lot of flakiness. This has been a big problem in the last month

@holdenk
Copy link
Contributor Author

holdenk commented Jun 22, 2015

Well, since SparkSubmitt should ready be tested, could we just test the param code gen?

@shivaram
Copy link
Contributor

shivaram commented Jun 22, 2015

Yeah I think that sounds reasonable. Tying this up with the SparkSubmitSuite will be pretty messy. One easy way to do this is to just pull out the code to construct the spark-submit command line into a separate function and then test that function. https://github.com/apache/spark/blob/master/R/pkg/inst/tests/test_utils.R#L23 has some examples on how to test util functions.

@shivaram shivaram mentioned this pull request Jun 23, 2015
Closed
@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35523 has finished for PR 6928 at commit c1a9233.

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

@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35525 has finished for PR 6928 at commit c7a4471.

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

@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35529 has finished for PR 6928 at commit 865a90c.

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

shivaram
shivaram reviewed Jun 23, 2015
View reviewed changes
R/pkg/R/client.R
Copy link
Contributor

@shivaram shivaram Jun 23, 2015

Choose a reason for hiding this comment

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

minor: sparkHome is not used in this function

@shivaram
Copy link
Contributor

shivaram commented Jun 24, 2015

Thanks @holdenk for the update. LGTM. Will merge after tests pass

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35707 has finished for PR 6928 at commit b60dd63.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Jun 24, 2015

Merged build finished. Test FAILed.

asfgit pushed a commit that referenced this pull request Jun 24, 2015
@holdenk @shivaram
[SPARK-8506] Add pakages to R context created through init.
f6682dd 
Author: Holden Karau <[email protected]>

Closes #6928 from holdenk/SPARK-8506-sparkr-does-not-provide-an-easy-way-to-depend-on-spark-packages-when-performing-init-from-inside-of-r and squashes the following commits:

b60dd63 [Holden Karau] Add an example with the spark-csv package
fa8bc92 [Holden Karau] typo: sparm -> spark
865a90c [Holden Karau] strip spaces for comparision
c7a4471 [Holden Karau] Add some documentation
c1a9233 [Holden Karau] refactor for testing
c818556 [Holden Karau] Add pakages to R

(cherry picked from commit 43e6619)
Signed-off-by: Shivaram Venkataraman <[email protected]>
@asfgit asfgit closed this in 43e6619 Jun 24, 2015
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 25, 2015
@holdenk @nemccarthy
[SPARK-8506] Add pakages to R context created through init.
0637964 
Author: Holden Karau <[email protected]>

Closes apache#6928 from holdenk/SPARK-8506-sparkr-does-not-provide-an-easy-way-to-depend-on-spark-packages-when-performing-init-from-inside-of-r and squashes the following commits:

b60dd63 [Holden Karau] Add an example with the spark-csv package
fa8bc92 [Holden Karau] typo: sparm -> spark
865a90c [Holden Karau] strip spaces for comparision
c7a4471 [Holden Karau] Add some documentation
c1a9233 [Holden Karau] refactor for testing
c818556 [Holden Karau] Add pakages to R

(cherry picked from commit 43e6619)
Signed-off-by: Shivaram Venkataraman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@holdenk @SparkQA @shivaram @brkyvz @AmplabJenkins