[R-package][ci] added CI stage for R package (fixes #2335, fixes #2569) #2530

jameslamb · 2019-10-27T15:10:46Z

I'm still committed to getting R CI running in GitHub Actions (#2353 ) but I think there is also an immediate benefit to having the R package built + unit tests run at all, and tacking that on to Travis is the easiest path.

In this PR, I propose adding an r-pkg stage to our Travis configuration to at least test the R package on Ubuntu and macOS. That will catch issues like #2524 , where a change on the C++ side breaks the R package. We can handle tests on Windows, tests with different versions of R, running on GitHub Actions, etc. in later PRs.

Opening as a draft PR since I may need to build a few times to get feedback from Travis. I'll change to regular PR and add reviewers when I think it's ready and worthy of your time, but comments are welcome any time!

.travis.yml

StrikerRUS · 2019-10-27T16:09:32Z

I think that R environment from conda is not appropriate to run unit tests. Not many users use it, it has its own bugs, it has huge delay in updating package versions, it doesn't work normally on Windows, it means that compilation of LightGBM will be performed by conda's specific compilers.

Can you please consider using vanilla R environment? #1143 (comment)

jameslamb · 2019-10-27T16:22:51Z

I think that R environment from conda is not appropriate to run unit tests. Not many users use it, it has its own bugs, it has huge delay in updating package versions, it doesn't work normally on Windows, it means that compilation of LightGBM will be performed by conda's specific compilers.

Can you please consider using vanilla R environment? #1143 (comment)

Sure I can look. To be clear, I'm only using conda as an installer for R itself (r-base) since we already had conda laying around. All the other packages I'm installing are source distributions directly from CRAN (those install.packages() calls).

I'll take a look at non-conda R environment.

R-package/R/lightgbm.R

R-package/R/lgb.cv.R

.ci/test.sh

jameslamb · 2019-10-28T02:18:48Z

I am getting this WARNING from R CMD check

Somehow additional files like a .coveralls.yml (which we don't even have in this project) and a .git/ directory are being included in the package. I believe these are coming from the compute git submodule, so I need to figure out how to deal with that.

jameslamb · 2019-10-28T22:24:27Z

ok next issue, I am seeing this on Travis but not locally

* checking for GNU extensions in Makefiles ... WARNING
Found the following file(s) containing GNU extensions:
  src/build/Makefile
Portable Makefiles do not use GNU extensions such as +=, :=, $(shell),
$(wildcard), ifeq ... endif, .NOTPARALLEL See section ‘Writing portable

When I run locally, I do see that a file lightgbm.Rcheck/00_pkg_src/build/Makefile is created after running

export CXX=/usr/local/bin/g++-8
export CC=/usr/local/bin/gcc-8
Rscript build_r.R
R CMD check *.tar.gz --as-cran

However, that WARNING is not generated on my local machine (macOS High Sierra,R 3.5.0).

Of the extensions complained about in that warning, I only see one in that file:

# Allow only one "make -f Makefile" at a time, but pass parallelism.
.NOTPARALLEL:

Currently on Travis I'm building with R 3.6.1. I'm going to try with R3.5.0 just to see if the . R version difference is to blame.

Next I'll check cmake version differences, since this Makefile is something that is created by cmake (not a file we have checked into this package's source).

jameslamb · 2019-11-05T21:17:30Z

R-package/.Rbuildignore

+^src/compute/.git$
+^src/compute/.gitignore$
+^src/compute/CONTRIBUTING.md$
+^src/compute/README.md$


src/compute is the boost stuff we pull in as a git submodule. It's important that the random other stuff from that repo isn't bundled with the R package, as R CMD check will complain about things like a .git/ folder being put into a source distribution.

jameslamb · 2019-11-05T21:18:10Z

R-package/R/lgb.prepare_rules2.R

@@ -46,7 +46,7 @@
 #'   Species = c(
 #'     "setosa" = 3L
 #'     , "versicolor" = 2L
-#'     , virginica" = 1L
+#'     , "virginica" = 1L


fixes

Error: LaTeX error. Mismatched quote or braces

When building the documentation.

jameslamb · 2019-11-05T21:27:14Z

.travis.yml

+    # - TASK=mpi METHOD=pip
+    # - TASK=gpu METHOD=source PYTHON_VERSION=3.5
+    # - TASK=gpu METHOD=pip PYTHON_VERSION=3.6
+    - TASK=r-pkg R_VERSION=3.6.1


right now we'll get this R version when building on Mac, but on Ubuntu we're at the whim of whatever is "latest" for the particular ubuntu version we are running. In this case the latest version for R available on bionic is 3.4.4 (see here). You can see here for more.

I think that ok is ok for now.

Then this variable should be named something like R_VERSION_MAC to avoid any misleading interpretation.

BTW, why did you abandoned the idea to install the latest version from CRAN sources for Ubuntu?

Do we have any limitation in using chars here? 😄 TASK=r-package seems to be more pretty to see.

ha nope no limitation, happy to use r-package. r-pkg is a bad habit of mine from other projects.

why did you abandoned the idea to install the latest version from CRAN sources for Ubuntu

I followed the instructions from https://cran.r-project.org/bin/linux/ubuntu/README.html which SAY you should get 3.6.x, but still ended up with 3.4.x. I'm not sure why. We are using bionic (Ubuntu 18.04) on Travis, and it seems 3.4.x is the latest r-base release available. I think I see the problem though...found in this answer that there is one more step to see more versions from that server. I think in my latest commit, I have it successfully installing 3.6.1

jameslamb · 2019-11-06T02:31:50Z

Alright, I think this PR is finally ready for review. I've left some comments above calling out items that I think might be concerning. I expect we'll have a conversation about two more things:

the r-pkg stage takes 10-11 minutes to run. I'm unsure how to get that down. I don't think it's TOO bad given what we get in return in terms of quality control
I had to put a gross hack into install.libs.R to deal with a new check in R CMD check for R3.6+. Basically cmake was generating a Makefile with .NOTPARALLEL: in it, which R complains about as not portable. My hack was to have install.libs.R rewrite that Makefile without NOTPARALLEL before executing it to build lib_lightgbm. I am open to other ideas!

@StrikerRUS I know you don't have an R environment to test some of the changes in, but I would definitely like your input on the changes I'm making to .ci/test.sh at a minimum (and of course anything else you notice 😀 )

.ci/test.sh

Laurae2

Small reminder for contributors to R-package: next R release might be 4.0 (I guess if we test against R-devel, we will do it manually for the moment)

StrikerRUS

Please see below my comments about the CI part.

.ci/test.sh

.ci/test_r_package.sh

jameslamb · 2019-11-13T01:43:08Z

@StrikerRUS please see 64a46b5 and let me know what you think. Azure pipelines was failing because we are currently getting Ubuntu 14.04 (trusty) and I had hard-coded test_r_package.sh for Ubuntu 18.04 (bionic) since that's what we get on Travis.

I think the solution in that commit should work.

jameslamb · 2019-12-08T03:14:39Z

@jameslamb

That is also the first build using the new container on dev of https://github.com/guolinke/lightgbm-ci-docker and looks like it's working!

Very awesome! Please switch compilers back to their original values.

thanks! just switched them back in d98211f

jameslamb · 2019-12-08T04:29:16Z

All working! @StrikerRUS could you take one more look?

StrikerRUS · 2019-12-08T14:43:30Z

.vsts-ci.yml

@@ -11,7 +11,7 @@ variables:
 resources:
  containers:
  - container: ubuntu1404
-    image: lightgbm/vsts-agent:ubuntu-14.04
+    image: lightgbm/vsts-agent:ubuntu-14.04-dev


@jameslamb

All working! @StrikerRUS could you take one more look?

👍

guolinke/lightgbm-ci-docker#7 has been merged, so you can switch container back to the production version.

switched back in 9ab619c

StrikerRUS

LGTM! Thanks!

jameslamb · 2019-12-08T20:37:31Z

LGTM! Thanks!

Thanks so much! I think it's ok to merge with your approval + @Laurae2 's. Do you agree? This one touches a lot of stuff so want to be sure it's ok.

StrikerRUS · 2019-12-09T01:38:05Z

@jameslamb Sure! @Laurae2's review was outdated, so I re-requested a fresh one from him.

jameslamb · 2019-12-12T03:51:29Z

@jameslamb Sure! @Laurae2's review was outdated, so I re-requested a fresh one from him.

@Laurae2 could you take one more look?

guolinke

Lgtm

jameslamb · 2019-12-15T18:00:22Z

Thanks for the reviews and help along the way! I'm excited to merge this one 😀

jameslamb added the in progress label Oct 27, 2019