Skip to content

[SPARK-22766] Install R linter package in spark lib directory #19959

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

Closed
falaki wants to merge 3 commits into from
Closed

[SPARK-22766] Install R linter package in spark lib directory #19959

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2376b0b
Installing lintr package in Spark library location instead of site-li…
falaki Dec 12, 2017
6892725
Installing lintr from github no mwatter what
falaki Dec 12, 2017
4355fae
Updating Rcpp to 0.12.13 and using a CRAN a snapshot
falaki Dec 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions dev/lint-r.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,11 @@ if (! library(SparkR, lib.loc = LOCAL_LIB_LOC, logical.return = TRUE)) {
# Installs lintr from Github in a local directory.
# NOTE: The CRAN's version is too old to adapt to our rules.
if ("lintr" %in% row.names(installed.packages()) == FALSE) {
Copy link
Member

@HyukjinKwon HyukjinKwon Dec 12, 2017

Choose a reason for hiding this comment

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

@falaki, actually, seems it's not installing lintr in Jenkins:

Error in library(lintr, lib.loc = LOCAL_LIB_LOC) : 
  there is no package called 'lintr'
Execution halted

I think it's because there's lintr in Jenkins already and "lintr" %in% row.names(installed.packages()) returns TRUE.

So, I think lintr is already not synced to jimhester/lintr@5431140 and I had some discussion, for example, here - https://issues.apache.org/jira/browse/SPARK-22063

Copy link
Member

@HyukjinKwon HyukjinKwon Dec 12, 2017

Choose a reason for hiding this comment

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

cc @shivaram, @felixcheung and @shaneknapp as well for your information.

Copy link
Contributor Author

@falaki falaki Dec 12, 2017

Choose a reason for hiding this comment

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

Nice find. I think in that case we should just install it anyway and do not rely on the existing installation. What do you think?

Copy link
Member

@HyukjinKwon HyukjinKwon Dec 12, 2017

Choose a reason for hiding this comment

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

Yea, that sounds ok to me; however, I remember @shivaram had a different idea(?) (did I remember this correctly .. ?)

Copy link
Contributor

@shaneknapp shaneknapp Dec 13, 2017

Choose a reason for hiding this comment

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

i'd rather not have the build installing system-level packages, unless it's in it's own distinct environment... the R ecosystem on our jenkins is pretty fragile as it is, so let me think about how i want to go about addressing this. updating the workers' packages isn't that big of a deal, i would just like it to be reproducable.

Copy link
Contributor Author

@falaki falaki Dec 13, 2017

Choose a reason for hiding this comment

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

There is an existing Rcpp installed on our jenkins machines.

Copy link
Contributor

@shivaram shivaram Dec 14, 2017

Choose a reason for hiding this comment

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

Right - so it means that the existing Rcpp was used to build the existing lintr right ? And if we are not changing lintr versions then the same Rcpp should still work ? Sorry if I'm missing something here

Copy link
Contributor Author

@falaki falaki Dec 16, 2017

Choose a reason for hiding this comment

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

I can see two possibilities. Unfortunately, I cannot get access to machines to verify:

  1. There is already an older version so ("lintr" %in% row.names(installed.packages()) returns 'TRUE`
  2. When installing lintr in a new directory some transient dependency check behaves different and we end up rejecting existing Rcpp version.

Why do you think re-installing Rcpp in a non-system directory is an issue? This is under $SPARK_HOME and will be wiped with every new test.

Copy link
Contributor

@shivaram shivaram Dec 22, 2017

Choose a reason for hiding this comment

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

The thing I'm trying to make sure is that the lint-r version we are using is not changing because of this change. If we need a new Rcpp to install this lint-r then I'm worried something has changed ?

Also it seems fragile to expect Rcpp to be there at that repo with this version ? The github-based install lint-r seems more stable to me.

One more final question is what is xmlparsedata used for ?

Copy link
Member

@felixcheung felixcheung Mar 28, 2018

Choose a reason for hiding this comment

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

does export R_LIBS_USER=~/Rlib work for installing Rcpp?

devtools install_version can fetch a specific version of lintr I think
https://www.rdocumentation.org/packages/devtools/versions/1.13.3/topics/install_version

devtools::install_github("jimhester/lintr@5431140")
devtools::with_libpaths(new = LOCAL_LIB_LOC, devtools::install_github("jimhester/lintr@5431140"))
}

library(lintr)
library(lintr, lib.loc = LOCAL_LIB_LOC)
library(xmlparsedata, lib.loc = LOCAL_LIB_LOC)
library(methods)
library(testthat)
path.to.package <- file.path(SPARK_ROOT_DIR, "R", "pkg")
Expand Down