[SPARK-22063][R] Fixes lint check failures in R by latest commit sha1 ID of lint-r #19290

HyukjinKwon · 2017-09-20T09:25:22Z

What changes were proposed in this pull request?

Currently, we set lintr to r-lib/lintr@a769c0b (see this and SPARK-14074).

I first tested and checked lintr-1.0.1 but it looks many important fixes are missing (for example, checking 100 length). So, I instead tried the latest commit, r-lib/lintr@5431140, in my local and fixed the check failures.

It looks it has fixed many bugs and now finds many instances that I have observed and thought should be caught time to time, here I filed the results.

The downside looks it now takes about 7ish mins, (it was 2ish mins before) in my local.

How was this patch tested?

Manually, ./dev/lint-r after manually updating the lintr package.

HyukjinKwon · 2017-09-20T09:26:18Z

R/pkg/.lintr

@@ -1,2 +1,2 @@
-linters: with_defaults(line_length_linter(100), multiple_dots_linter = NULL, camel_case_linter = NULL, open_curly_linter(allow_single_line = TRUE), closed_curly_linter(allow_single_line = TRUE))
+linters: with_defaults(line_length_linter(100), multiple_dots_linter = NULL, object_name_linter = NULL, camel_case_linter = NULL, open_curly_linter(allow_single_line = TRUE), closed_curly_linter(allow_single_line = TRUE))


object_name_linter = NULL looks required. Otherwise, it complains about inconsistent naming styles.

HyukjinKwon · 2017-09-20T09:28:14Z

R/pkg/R/DataFrame.R

@@ -2649,15 +2651,15 @@ setMethod("merge",
 #' @return list of columns
 #'
 #' @note generateAliasesForIntersectedCols since 1.6.0
-generateAliasesForIntersectedCols <- function (x, intersectedColNames, suffix) {
+generateAliasesForIntersectedCols <- function(x, intersectedColNames, suffix) { # nolint


Looks this hits 30 length identifier limit but we can't change as exposed in the doc.

wait... this isn't public though; it's not in https://github.com/apache/spark/blob/master/R/pkg/NAMESPACE
looks like this shouldn't be in the doc (@noRd or # instead of #')

HyukjinKwon · 2017-09-20T09:28:30Z

R/pkg/R/column.R

-#'        \href{https://spark.apache.org/docs/latest/sparkr.html#data-type-mapping-between-r-and-spark}{
-#'        Spark Data Types} for available data types.
+#'        \href{https://spark.apache.org/docs/latest/sparkr.html#data-type-mapping-between-
+#'        r-and-spark}{Spark Data Types} for available data types.


I checked this link is not broken for sure.

HyukjinKwon · 2017-09-20T09:29:06Z

R/pkg/R/context.R

@@ -329,7 +329,7 @@ spark.addFile <- function(path, recursive = FALSE) {
 #' spark.getSparkFilesRootDirectory()
 #'}
 #' @note spark.getSparkFilesRootDirectory since 2.1.0
-spark.getSparkFilesRootDirectory <- function() {
+spark.getSparkFilesRootDirectory <- function() { # nolint


Ditto: 30 length identifier limit but exposed in the doc

this one is an API...

HyukjinKwon · 2017-09-20T09:30:26Z

R/pkg/R/mllib_tree.R

+#' \href{http://spark.apache.org/docs/latest/ml-classification-regression.html#random-forest-
+#' regression}{Random Forest Regression} and
+#' \href{http://spark.apache.org/docs/latest/ml-classification-regression.html#random-forest-
+#' classifier}{Random Forest Classification}


links were checked

HyukjinKwon · 2017-09-20T09:30:27Z

R/pkg/R/mllib_tree.R

+#' \href{http://spark.apache.org/docs/latest/ml-classification-regression.html#gradient-boosted-
+#' tree-regression}{GBT Regression} and
+#' \href{http://spark.apache.org/docs/latest/ml-classification-regression.html#gradient-boosted-
+#' tree-classifier}{GBT Classification}


links were checked

HyukjinKwon · 2017-09-20T09:30:29Z

R/pkg/R/mllib_tree.R

+#' \href{http://spark.apache.org/docs/latest/ml-classification-regression.html#decision-tree-
+#' regression}{Decision Tree Regression} and
+#' \href{http://spark.apache.org/docs/latest/ml-classification-regression.html#decision-tree-
+#' classifier}{Decision Tree Classification}


links were checked

HyukjinKwon · 2017-09-20T09:31:11Z

dev/lint-r.R

@@ -28,6 +28,7 @@ if (! library(SparkR, lib.loc = LOCAL_LIB_LOC, logical.return = TRUE)) {
 # NOTE: The CRAN's version is too old to adapt to our rules.
 if ("lintr" %in% row.names(installed.packages())  == FALSE) {
  devtools::install_github("jimhester/lintr@a769c0b")
+  # devtools::install_github("jimhester/lintr@5431140")


Here, I didn't yet change this. Up to my knowledge, this doesn't actually try to install this package if already installed though.. just in case.

because of if ("lintr" %in% row.names(installed.packages()) == FALSE) {
?
it might be the case the Jenkins boxes already have a version installed and this won't update it

Yes .. I think I should add few more changes here if we are going to update this package.

It looks like the commented is an old code. Maybe a comment to explain it.

Oh, just to be clear, I didn't change this yet but just commented this for now ... Actually, devtools::install_github("jimhester/lintr@5431140") is new one but I didn't change this yet just in case.

Will add some comments around this when I add few more changes and clean up soon in any event.

HyukjinKwon · 2017-09-20T09:36:34Z

@felixcheung and @shivaram,

Should we upgrade this to the latest commit? it increases the time by 5ish mins.

If there is any worry about upgrading, I am willing to leave the script as was. The other fixes look reasonable and I have wondered why these are not being caught. Does this make sense to both of you too?

SparkQA · 2017-09-20T12:43:21Z

Test build #81988 has finished for PR 19290 at commit 6e41eff.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
#' equivalent to setting thresholds c(1-p, p). In multiclass (or binary)

SparkQA · 2017-09-20T17:12:10Z

Test build #82000 has finished for PR 19290 at commit 2baefd9.

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

shivaram · 2017-09-20T17:20:10Z

@HyukjinKwon Thanks for looking at this. The 5 min addition seems unfortunate though -- does that also happen with lintr-1.0.1 ? I wonder if we are seeing some specific performance slowdown because of not using a release (it might be good to follow up with lint-r authors if that is the case ?)

felixcheung

I wonder if we should use the git tag of the 1.0.1 release
https://github.com/jimhester/lintr/releases

felixcheung · 2017-09-21T05:13:34Z

R/pkg/R/DataFrame.R

-            colsX <- generateAliasesForIntersectedCols(x, by, suffixes[1])
-            colsY <- generateAliasesForIntersectedCols(y, by, suffixes[2])
+            colsX <- generateAliasesForIntersectedCols(x, by, suffixes[1]) # nolint
+            colsY <- generateAliasesForIntersectedCols(y, by, suffixes[2]) # nolint


what's the problem here? name too long I think?

felixcheung · 2017-09-21T05:16:22Z

R/pkg/R/DataFrame.R

@@ -2649,15 +2651,15 @@ setMethod("merge",
 #' @return list of columns
 #'
 #' @note generateAliasesForIntersectedCols since 1.6.0
-generateAliasesForIntersectedCols <- function (x, intersectedColNames, suffix) {
+generateAliasesForIntersectedCols <- function(x, intersectedColNames, suffix) { # nolint


wait... this isn't public though; it's not in https://github.com/apache/spark/blob/master/R/pkg/NAMESPACE
looks like this shouldn't be in the doc (@noRd or # instead of #')

felixcheung · 2017-09-21T05:17:23Z

R/pkg/R/context.R

@@ -329,7 +329,7 @@ spark.addFile <- function(path, recursive = FALSE) {
 #' spark.getSparkFilesRootDirectory()
 #'}
 #' @note spark.getSparkFilesRootDirectory since 2.1.0
-spark.getSparkFilesRootDirectory <- function() {
+spark.getSparkFilesRootDirectory <- function() { # nolint


this one is an API...

felixcheung · 2017-09-21T05:22:33Z

dev/lint-r.R

@@ -28,6 +28,7 @@ if (! library(SparkR, lib.loc = LOCAL_LIB_LOC, logical.return = TRUE)) {
 # NOTE: The CRAN's version is too old to adapt to our rules.
 if ("lintr" %in% row.names(installed.packages())  == FALSE) {
  devtools::install_github("jimhester/lintr@a769c0b")
+  # devtools::install_github("jimhester/lintr@5431140")


because of if ("lintr" %in% row.names(installed.packages()) == FALSE) {
?
it might be the case the Jenkins boxes already have a version installed and this won't update it

felixcheung · 2017-09-21T05:29:34Z

I think this is great to have, thanks for solving the mystery.

5 min: this is mildly concerning, is it possible this is caused by new checks in lintr? perhaps we could exclude them or something?
lintr version: I see what you say about 1.0.1. According this commit list https://github.com/jimhester/lintr/commits/master the tag for 1.0.1 vs the latest (the git tag you are using) are different only by some insignificant changes?

HyukjinKwon · 2017-09-21T13:59:16Z

To @shivaram:

does that also happen with lintr-1.0.1 ? I wonder if we are seeing some specific performance slowdown because of not using a release (it might be good to follow up with lint-r authors if that is the case ?)

I just ran it with lintr-1.0.1. This took 2ish mins in my local, which is almost identical with the current, r-lib/lintr@a769c0b. I opened an issue here - https://github.com/jimhester/lintr/issues/270.

To @felixcheung:

5 min: this is mildly concerning, is it possible this is caused by new checks in lintr? perhaps we could exclude them or something?

I tried to disable all the linters documented in https://github.com/jimhester/lintr/blob/master/README.md#available-linters:

diff --git a/R/pkg/.lintr b/R/pkg/.lintr
index ae50b28ec61..ca1efcfacc7 100644
--- a/R/pkg/.lintr
+++ b/R/pkg/.lintr
@@ -1,2 +1,2 @@
-linters: with_defaults(line_length_linter(100), multiple_dots_linter = NULL, camel_case_linter = NULL, open_curly_linter(allow_single_line = TRUE), closed_curly_linter(allow_single_line = TRUE))
+linters: with_defaults(commas_linter = NULL, object_usage_linter = NULL, absolute_path_linter = NULL, nonportable_path_linter = NULL, pipe_continuation_linter = NULL, assignment_linter = NULL, closed_curly_linter = NULL, extraction_operator_linter = NULL, implicit_integer_linter = NULL, infix_spaces_linter = NULL, line_length_linter = NULL, no_tab_linter = NULL, object_length_linter = NULL, object_name_linter = NULL, open_curly_linter = NULL, semicolon_terminator_linter = NULL, single_quotes_linter = NULL, spaces_inside_linter = NULL, spaces_left_parentheses_linter = NULL, todo_comment_linter = NULL, trailing_blank_lines_linter = NULL, trailing_whitespace_linter = NULL, T_and_F_symbol_linter = NULL, undesirable_function_linter = NULL, undesirable_operator_linter = NULL, unneeded_concatenation_linter = NULL)
 exclusions: list("inst/profile/general.R" = 1, "inst/profile/shell.R")

and this took 1.5 mins. Looks we could identify some linters that we might not need.

lintr version: I see what you say about 1.0.1. According this commit list https://github.com/jimhester/lintr/commits/master the tag for 1.0.1 vs the latest (the git tag you are using) are different only by some insignificant changes?

Diff looks 122 commits - r-lib/lintr@v1.0.1...master . Will take a look though the commit logs (pending ..)

SparkQA · 2017-09-21T16:03:37Z

Test build #82034 has finished for PR 19290 at commit 49a166d.

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

felixcheung · 2017-09-21T16:41:02Z

isn't it the other way around?
https://github.com/jimhester/lintr/compare/v1.0.1?expand=1

anyway I mean if we use the git tag 9951084 it should match v1.0.1 release exactly.
the one you listed r-lib/lintr@5431140 is in master I think and perhaps you are saying there are more changes in master (which is true from the diff)?

felixcheung · 2017-09-22T04:23:00Z

btw, could you check if haven't already, if nolint around the http link, roxygen is going to handle that correctly?

HyukjinKwon · 2017-09-22T04:36:22Z

Yes, I checked already.

SparkQA · 2017-09-22T13:47:22Z

Test build #82081 has started for PR 19290 at commit 7e6c2c5.

HyukjinKwon · 2017-09-22T15:05:04Z

Will update the comment tomorrow.

HyukjinKwon · 2017-09-23T14:21:20Z

anyway I mean if we use the git tag 9951084 it should match v1.0.1 release exactly.
the one you listed r-lib/lintr@5431140 is in master I think and perhaps you are saying there are more changes in master (which is true from the diff)?

Ah, yes. It looks some changes are intendedly not into v1.0.1 (vs the master), and given the results from the linter in my local, v1.0.1 missed some important changes. Although I am not used to the policy there, I see the large diff in the commit logs (master vs tag v1.0.1).

HyukjinKwon · 2017-09-23T16:03:17Z

5 min: this is mildly concerning, is it possible this is caused by new checks in lintr? perhaps we could exclude them or something?

Assuming from the doc, 1.0.1 looks missing the linters below (vs master):

nonportable_path_linter: check that file.path() is used to construct safe and portable paths.
pipe_continuation_linter: Check that each step in a pipeline is on a new line, or the entire pipe fits on one line.
extraction_operator_linter: check that the [[ operator is used when extracting a single element from an object, not [ (subsetting) nor $ (interactive use).
implicit_integer_linter: check that integers are explicitly typed using the form 1L instead of 1.
object_name_linter: check that object names conform to a single naming style, e.g. snake_case or lowerCamelCase.
semicolon_terminator_linter: check that no semicolons terminate statements.
todo_comment_linter: check that the source contains no TODO comments (case-insensitive).
T_and_F_symbol_linter: avoid the symbols T and F (for TRUE and FALSE).
undesirable_function_linter: report the use of undesirable functions, e.g. options or sapply and suggest an alternative.
undesirable_operator_linter: report the use of undesirable operators, e.g. ::: or <<- and suggest an alternative.
unneeded_concatenation_linter: check that the c function is not used without arguments nor with a single constant

I manually disabled the linters above but the elapsed time looks almost the same. So, I think some existing linters were slowed down in the master.

I tried to disable everything except for each linter, https://gist.github.com/HyukjinKwon/1f852bcdcc2a13f396dfd2d6a88bea16

HyukjinKwon · 2017-09-23T16:29:36Z

I also tried v1.0.1 against the master; however, it looks not detecting many instances this PR fixes. It only detected 3 extra instances:

R/DataFrame.R:2615:22: style: Variable and function names should not be longer than 30 characters.
            colsX <- generateAliasesForIntersectedCols(x, by, suffixes[1])
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
R/DataFrame.R:2616:22: style: Variable and function names should not be longer than 30 characters.
            colsY <- generateAliasesForIntersectedCols(y, by, suffixes[2])
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
R/DataFrame.R:2666:1: style: Variable and function names should not be longer than 30 characters.
generateAliasesForIntersectedCols <- function (x, intersectedColNames, suffix) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lintr checks failed.

So, I would like to propose to use r-lib/lintr@5431140 if possible.

I opened an issue to ask a question about this slowdown - https://github.com/jimhester/lintr/issues/270. Probably, we could wait for few days. if nothing could be done for now, I guess we could consider just use r-lib/lintr@5431140 as is ...

HyukjinKwon · 2017-09-23T16:29:54Z

retest this please

SparkQA · 2017-09-23T19:50:34Z

Test build #82120 has finished for PR 19290 at commit 7e6c2c5.

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

felixcheung · 2017-09-23T23:17:55Z

R/pkg/R/DataFrame.R

@@ -2650,8 +2650,9 @@ setMethod("merge",
 #' @param suffix a suffix for the column name
 #' @return list of columns
 #'
-#' @note generateAliasesForIntersectedCols since 1.6.0
-generateAliasesForIntersectedCols <- function(x, intersectedColNames, suffix) { # nolint
+#' @note genAliasesForIntersectedCols since 1.6.0


nit: I'd remove this @note too

felixcheung · 2017-09-23T23:23:11Z

oh, remember to remove WIP and update this line https://github.com/apache/spark/pull/19290/files#diff-74ca2b618d236bbd6faa23e13bff1403R30 before merging in?

HyukjinKwon · 2017-09-24T09:18:37Z

dev/lint-r.R

+# The current latest is jimhester/lintr@5431140 (see SPARK-22063), the dev version,
+# '1.0.1.9000'.
+if ("lintr" %in% row.names(installed.packages()) == FALSE ||
+    packageVersion("lintr") != "1.0.1.9000") {


I checked this - when downgrading, upgrading and not installed in my local.

HyukjinKwon · 2017-09-24T09:19:03Z

R/pkg/R/column.R

@@ -238,8 +238,10 @@ setMethod("between", signature(x = "Column"),
 #' @param x a Column.
 #' @param dataType a character object describing the target data type.
 #'        See
+# nolint start


I just double checked the links.

HyukjinKwon · 2017-09-24T09:29:37Z

Ugh.. it looks failed to install due to permission issue ...

Downloading GitHub repo jimhester/lintr@5431140
from URL https://api.github.com/repos/jimhester/lintr/zipball/5431140
Installing lintr
Downloading GitHub repo hadley/xml2@master
from URL https://api.github.com/repos/hadley/xml2/zipball/master
Installing xml2
'/usr/lib64/R/bin/R' --no-site-file --no-environ --no-save --no-restore  \
  --quiet CMD INSTALL  \
  '/tmp/RtmprFnaBu/devtools438c516b7717/r-lib-xml2-5799cd9'  \
  --library='/usr/lib64/R/library' --install-tests 

Error: ERROR: no permission to install to directory '/usr/lib64/R/library'
Installation failed: Command failed (1)
'/usr/lib64/R/bin/R' --no-site-file --no-environ --no-save --no-restore  \
  --quiet CMD INSTALL  \
  '/tmp/RtmprFnaBu/devtools438c286c3792/jimhester-lintr-5431140'  \
  --library='/usr/lib64/R/library' --install-tests 

Error: ERROR: no permission to install to directory '/usr/lib64/R/library'
Installation failed: Command failed (1)

HyukjinKwon · 2017-09-24T09:38:13Z

Hi @shaneknapp. I am sorry but it's me again ...

Here, this PR tries to upgrade an R package, lintr for static code analysis for R, which is ran via ./dev/lint-r -> dev/lint-r.R as a part of Jenkins PR build.

This upgrade catches many meaningful instances that should have been caught before in SparkR codes and if I understood correctly, we decided to upgrade this if it does not cause a problem.

I tried to fix the dev/lint-r.R script so that lintr can be upgraded if this package is not installed or not the specific version (1.0.1.9000) after testing this in my local, but looks failed to upgrade in Jenkins due to a permission issue.

I believe the bash command below ~~installs~~ upgrades the package correctly (tested in my local):

Rscript -e "devtools::install_github('jimhester/lintr@5431140')"

Maybe, I should have cc'ed you here first before trying this one. I apologise and I will cc you first in such cases in the future.

SparkQA · 2017-09-24T12:31:14Z

Test build #82128 has finished for PR 19290 at commit 387228d.

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

SparkQA · 2017-09-24T12:37:18Z

Test build #82129 has finished for PR 19290 at commit ee7eb9d.

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

shaneknapp · 2017-09-25T00:23:04Z

@HyukjinKwon -- you will absolutely not have builds install packages on the build system. this is a really bad idea.

is this absolutely required, or just to fix a warning in the build output?

HyukjinKwon · 2017-09-29T06:05:06Z

I just double checked the newer lintr, lintr@5431140, passes on the top of the master with the current change.

SparkQA · 2017-09-29T07:04:43Z

Test build #82308 has finished for PR 19290 at commit 550750e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-09-29T07:09:40Z

retest this please

felixcheung

yap, let's update the PR title too

SparkQA · 2017-09-29T10:29:53Z

Test build #82312 has finished for PR 19290 at commit 550750e.

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

shaneknapp · 2017-09-29T15:18:38Z

hey all, i'm here. was out sick for the past few days and trying to get caught up. sorry about that! so... what version of lintr do we need to put on the workers?

…

On Fri, Sep 29, 2017 at 3:32 AM, UCB AMPLab ***@***.***> wrote: Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/82312/ Test PASSed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19290 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABiDrC3TwFkXTT_wInCvG0W450khEmugks5snMdJgaJpZM4PdlZS> .

HyukjinKwon · 2017-09-30T05:12:20Z

Ah, thank you @shaneknapp. The command below:

Rscript -e "devtools::install_github('jimhester/lintr@5431140')"

should upgrades lintr to the desired version here.

Actually, looks we should merge this one first before upgrading lintr in Jenkins. I realised upgrading it first could make R lintr check failures in other PRs up to my knowledge ..

I updated the change here back to not try to upgrade lintr. So, merging this one would not cause a problem on the build system. Let me go ahead with this one first and then I guess we could upgrade.

Thanks again and I am sorry for this, @shaneknapp.

felixcheung · 2017-09-30T17:06:17Z

btw, lintr once upgraded will run on all builds on all branches right? wouldn't the upgrade break other branches?

shaneknapp · 2017-09-30T17:16:10Z

@felixcheung -- yes, this is the system default lintr, meaning all calls to lintr will be against this version.

as for other branches, i think it could possibly break them.

@shivaram thoughts?

HyukjinKwon · 2017-10-01T09:41:51Z

Let me merge this one first. This shouldn't cause any problem to built system for now.

HyukjinKwon · 2017-10-01T09:46:07Z

Merged to master.

HyukjinKwon · 2017-10-01T09:52:29Z

So, @felixcheung, @shaneknapp and @shivaram, looks we have comments, #19290 (comment) and #19290 (comment), to discuss further.

The problem looks upgrading lintr to jimhester/lintr@5431140 could break other builds in other branches.

Should I rather try to install this for each build, for example, via checking the environment variables as a workaround? I checked those before - #17669 (comment)

shaneknapp · 2018-01-08T23:47:30Z

i filed a bug about this on the spark jira:
https://issues.apache.org/jira/browse/SPARK-22996

as we're about to move all the spark builds to new ubuntu machines, w/much more up2date package installs (especially R packages), we need to fix the lint problems and help make builds green again.

felixcheung · 2018-01-09T03:08:19Z

Shane is this going to affect one particular branch (eg. 2.3.0), or is it going to be all branches and all test runs? The changes are fairly substantial - if we need to back port then it could be quite a bit of work

shaneknapp · 2018-01-09T03:41:02Z

the driving force behind this is that the version of R (and all associated packages) is really quite old on the centos workers (3.1.1), and i'd really like us to get as up to date as possible on the new ubuntu machines. so far, lintr and testthat (opening a bug on that one soon) are the only packages that haven't played nice after the upgrade. if lintr 1.0.0.9001 is "good enough", i'd be ok w/locking the install to this version.

…

On Mon, Jan 8, 2018 at 7:08 PM, Felix Cheung ***@***.***> wrote: Shane is this going to affect one particular branch (eg. 2.3.0), or is it going to be all branches and all test runs? The changes are fairly substantial - if we need to back port then it could be quite a bit of work — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19290 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABiDrKH2y12reG2Ojum5cF5j4FOJU7lyks5tIthFgaJpZM4PdlZS> .

felixcheung · 2018-01-09T04:44:13Z

Given that we are forking 2.3 and locking down the branch any time now, it might make sense to stay with the "current version running on old centos workers", even though they are old.

HyukjinKwon · 2018-01-09T08:16:22Z

BTW, I believe we are testing it with R 3.4.1 via AppVeyor too. I have been thinking it's good to test both old and new versions ... I think we have a weak promise for R 3.1+ - http://spark.apache.org/docs/latest/index.html#downloading

shaneknapp · 2018-01-09T19:01:37Z

ok sounds good -- we'll keep things 'old' for now.

shivaram · 2018-01-09T19:41:36Z

The minimum R version supported is something that we can revisit though. I think we do this for Python, Java versions as well in the project

felixcheung · 2018-01-09T23:28:53Z

Right we could bump the supported R version for the next release. It should have minimal impact (since we are testing the close to the latest on appveyor... somewhat internally) lintr is quite a bit more complicated. Shane what’s the issue with testthat?

HyukjinKwon · 2018-01-09T23:58:44Z

I believe the issue with testthat he meatn is, it fails to run tests with testthat 2.0.0 is released. Just for a reminder, I rushed to update appveyor.yml to use a fixed version #20003 as it broke the tests. There are some details there to remember back.

felixcheung · 2018-01-10T03:29:59Z

argh, thanks for the reminder and the fix.
I knew calling internal method is going to bite us ... :(

HyukjinKwon commented Sep 20, 2017

View reviewed changes

felixcheung reviewed Sep 21, 2017

View reviewed changes

felixcheung approved these changes Sep 23, 2017

View reviewed changes

HyukjinKwon force-pushed the upgrade-r-lint branch from 7e6c2c5 to 387228d Compare September 24, 2017 09:16

HyukjinKwon commented Sep 24, 2017

View reviewed changes

HyukjinKwon changed the title ~~[WIP][SPARK-22063][R] Upgrades lintr to latest commit sha1 ID~~ [SPARK-22063][R] Upgrades lintr to latest commit sha1 ID Sep 24, 2017

Revert auto-detection back so that it does not try to upgrade lintr

550750e

HyukjinKwon force-pushed the upgrade-r-lint branch from ee7eb9d to 550750e Compare September 29, 2017 05:57

felixcheung reviewed Sep 29, 2017

View reviewed changes

HyukjinKwon changed the title ~~[SPARK-22063][R] Upgrades lintr to latest commit sha1 ID~~ [SPARK-22063][R] Fixes lint check failures in R by latest commit sha1 ID of lint-r Sep 30, 2017

asfgit closed this in 02c91e0 Oct 1, 2017

HyukjinKwon mentioned this pull request Dec 13, 2017

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

Closed

HyukjinKwon deleted the upgrade-r-lint branch January 2, 2018 03:41

HyukjinKwon mentioned this pull request Nov 18, 2019

[SPARK-29936][R] Fix SparkR lint errors and add lint-r GitHub Action #26564

Closed

		@@ -1,2 +1,2 @@
		linters: with_defaults(line_length_linter(100), multiple_dots_linter = NULL, camel_case_linter = NULL, open_curly_linter(allow_single_line = TRUE), closed_curly_linter(allow_single_line = TRUE))
		linters: with_defaults(line_length_linter(100), multiple_dots_linter = NULL, object_name_linter = NULL, camel_case_linter = NULL, open_curly_linter(allow_single_line = TRUE), closed_curly_linter(allow_single_line = TRUE))

[SPARK-22063][R] Fixes lint check failures in R by latest commit sha1 ID of lint-r #19290

[SPARK-22063][R] Fixes lint check failures in R by latest commit sha1 ID of lint-r #19290

Conversation

HyukjinKwon commented Sep 20, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

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

HyukjinKwon Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Sep 20, 2017

SparkQA commented Sep 20, 2017

SparkQA commented Sep 20, 2017

shivaram commented Sep 20, 2017

felixcheung 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

felixcheung commented Sep 21, 2017

HyukjinKwon commented Sep 21, 2017 • edited Loading

SparkQA commented Sep 21, 2017

felixcheung commented Sep 21, 2017 • edited Loading

felixcheung commented Sep 22, 2017

HyukjinKwon commented Sep 22, 2017 • edited Loading

SparkQA commented Sep 22, 2017

HyukjinKwon commented Sep 22, 2017

HyukjinKwon commented Sep 23, 2017 • edited Loading

HyukjinKwon commented Sep 23, 2017 • edited Loading

HyukjinKwon commented Sep 23, 2017

HyukjinKwon commented Sep 23, 2017

SparkQA commented Sep 23, 2017

Choose a reason for hiding this comment

felixcheung commented Sep 23, 2017

HyukjinKwon Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Sep 24, 2017 • edited Loading

HyukjinKwon commented Sep 24, 2017 • edited Loading

SparkQA commented Sep 24, 2017

SparkQA commented Sep 24, 2017

shaneknapp commented Sep 25, 2017

HyukjinKwon commented Sep 29, 2017

SparkQA commented Sep 29, 2017

HyukjinKwon commented Sep 29, 2017

felixcheung left a comment

Choose a reason for hiding this comment

SparkQA commented Sep 29, 2017

shaneknapp commented Sep 29, 2017 via email

HyukjinKwon commented Sep 30, 2017

felixcheung commented Sep 30, 2017

shaneknapp commented Sep 30, 2017

HyukjinKwon commented Oct 1, 2017

HyukjinKwon commented Oct 1, 2017

HyukjinKwon commented Oct 1, 2017

shaneknapp commented Jan 8, 2018

felixcheung commented Jan 9, 2018 via email

shaneknapp commented Jan 9, 2018 via email

felixcheung commented Jan 9, 2018

HyukjinKwon commented Jan 9, 2018

shaneknapp commented Jan 9, 2018

shivaram commented Jan 9, 2018

felixcheung commented Jan 9, 2018 via email

HyukjinKwon commented Jan 9, 2018

felixcheung commented Jan 10, 2018

HyukjinKwon commented Sep 20, 2017 •

edited

Loading

HyukjinKwon Sep 20, 2017 •

edited

Loading

viirya Sep 21, 2017 •

edited

Loading

HyukjinKwon commented Sep 21, 2017 •

edited

Loading

felixcheung commented Sep 21, 2017 •

edited

Loading

HyukjinKwon commented Sep 22, 2017 •

edited

Loading

HyukjinKwon commented Sep 23, 2017 •

edited

Loading

HyukjinKwon commented Sep 23, 2017 •

edited

Loading

HyukjinKwon Sep 24, 2017 •

edited

Loading

HyukjinKwon commented Sep 24, 2017 •

edited

Loading

HyukjinKwon commented Sep 24, 2017 •

edited

Loading