[ci] consolidate R package installs into a CI script #6808

jameslamb · 2025-01-31T04:12:00Z

Consolidates the various install.packages() calls across the project's CI into a single .ci/ script.

Notes for Reviewers

I'm intentionally not proposing updating user-facing docs like this:

Lines 429 to 431 in 425395d

    
           # install dependencies 
        
           RDscript${R_CUSTOMIZATION} \ 
        
             -e "install.packages(c('R6', 'data.table', 'jsonlite', 'knitr', 'markdown', 'Matrix', 'RhpcBLASctl', 'testthat'), repos = 'https://cran.r-project.org', Ncpus = parallel::detectCores())"

I'd like to keep this script limited to LightGBM's CI, and changeable at any time. I don't want to make it a part of the public interface of the project or encourage anyone else to use it.

…-scripts

jameslamb · 2025-02-03T02:28:55Z

.github/workflows/static_analysis.yml

@@ -65,7 +65,7 @@ jobs:
        shell: bash
        # yamllint disable rule:line-length
        run: |
-          Rscript -e "install.packages(c('R6', 'data.table', 'jsonlite', 'knitr', 'markdown', 'Matrix', 'RhpcBLASctl', 'roxygen2', 'testthat'), repos = 'https://cran.rstudio.com', Ncpus = parallel::detectCores())"
+          Rscript ./.ci/install-r-deps.R --build --roxygen


Notice... {testthat} and all its dependencies were being installed unnecessarily here! So this change should also make the static analysis job a little faster 🎉

jameslamb · 2025-02-03T02:29:41Z

.ci/test-r-package-windows.ps1

+# $command_args_patch = "commandArgs <- function(...){c('--build', '--test')};"
+# Invoke-R-Code-Redirect-Stderr "$command_args_patch source('install-r-deps.R')" ; Assert-Output $?


Suggested change

# $command_args_patch = "commandArgs <- function(...){c('--build', '--test')};"

# Invoke-R-Code-Redirect-Stderr "$command_args_patch source('install-r-deps.R')" ; Assert-Output $?

This seems to work ok without the more complicated Invoke-R-Code-Redirect-Stderr pattern 😁

StrikerRUS

Thank you so much for simplifying the codebase by writing this script!

I looked carefully through the diff and left some comments below:

.ci/test-r-package-valgrind.sh

StrikerRUS · 2025-02-03T15:08:24Z

.ci/test-r-package-windows.ps1

-    "lib = '$env:R_LIB_PATH', Ncpus = parallel::detectCores())"
-)
-Invoke-R-Code-Redirect-Stderr $params ; Assert-Output $?
+Rscript.exe --vanilla ".ci/install-r-deps.R" --build --test ; Assert-Output $?


Seem that processx was lost.

LightGBM/R-package/inst/make-r-def.R

Lines 40 to 42 in c9de57b

"Using system2() to run shell commands. Installing "

, "'processx' with install.packages('processx') might "

, "make this faster."

Fixed in dae117f.

Rscript.exe --vanilla ".ci/install-r-deps.R" --build --include=processx --test ; Assert-Output $?

.github/workflows/static_analysis.yml

.vsts-ci.yml

StrikerRUS · 2025-02-03T15:18:10Z

.vsts-ci.yml

@@ -420,7 +420,7 @@ jobs:
          R_LIB_PATH=~/Rlib
          export R_LIBS=${R_LIB_PATH}
          mkdir -p ${R_LIB_PATH}
-          RDscript -e "install.packages(c('R6', 'data.table', 'jsonlite', 'knitr', 'markdown', 'Matrix', 'RhpcBLASctl'),  lib = '${R_LIB_PATH}', dependencies = c('Depends', 'Imports', 'LinkingTo'), repos = 'https://cran.rstudio.com', Ncpus = parallel::detectCores())" || exit 1
+          RDscript .ci/install-r-deps.R --build --test || exit 1


Hmmm, I don't see testthat in master here...

Modified this in dae117f, to

RDscript .ci/install-r-deps.R --build --include=RhpcBLASctl || exit 1

StrikerRUS · 2025-02-03T15:27:40Z

.github/workflows/r_package.yml

-          R_LIBS="c('R6', 'data.table', 'jsonlite', 'knitr', 'markdown', 'Matrix', 'RhpcBLASctl')"
-          Rscript \
-            -e "install.packages(${R_LIBS}, repos = 'https://cran.rstudio.com', Ncpus = parallel::detectCores())"
+          Rscript ./.ci/install-r-deps.R --build --test


I don't see testthat in master here...

LightGBM/.github/workflows/r_package.yml

Lines 340 to 341 in c9de57b

# 'testthat' is not needed by 'rchk', so avoid installing it until here

Rscript -e "install.packages('testthat', repos = 'https://cran.rstudio.com', Ncpus = parallel::detectCores())"

Ah you are right. Adding --test here was to pull in {RhpcBLASctl}. I will rework this.

I modified this in dae117f, to this:

Rscript ./.ci/install-r-deps.R --build --test --exclude=testthat

{testthat} is installed later:

LightGBM/.github/workflows/r_package.yml

Lines 340 to 341 in 81922a7

# 'testthat' is not needed by 'rchk', so avoid installing it until here

Rscript -e "install.packages('testthat', repos = 'https://cran.rstudio.com', Ncpus = parallel::detectCores())"

StrikerRUS · 2025-02-03T15:29:56Z

.ci/install-r-deps.R

+# always use the same CRAN mirror
+CRAN_MIRROR <- Sys.getenv("CRAN_MIRROR", unset = "https://cran.r-project.org")
+
+# we always want these


Maybe we should allow installation of arbitrary R package by this script? For example, late testthat installation or processx.

Yeah I agree! I implemented this in dae117f

Proposing adding two arguments, --include and --exclude, which are applied to the list of dependencies calculated after processing coarser arguments like --all, --build, and --test.

Co-authored-by: Nikita Titov <[email protected]>

…o ci/r-deps-scripts

StrikerRUS · 2025-02-14T11:48:27Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/13328574983

Status: success ✔️.

StrikerRUS

Please consider checking my next round of comments. One of them about roxygen2 is important, I think.

.ci/install-r-deps.R

StrikerRUS · 2025-02-17T11:28:18Z

.github/workflows/static_analysis.yml

        run: |
-          Rscript -e "install.packages(c('R6', 'data.table', 'jsonlite', 'knitr', 'markdown', 'Matrix', 'RhpcBLASctl', 'roxygen2', 'testthat'), repos = 'https://cran.rstudio.com', Ncpus = parallel::detectCores())"
+          Rscript ./.ci/install-r-deps.R --build --include=roxygen


Suggested change

Rscript ./.ci/install-r-deps.R --build --include=roxygen

Rscript ./.ci/install-r-deps.R --build --include=roxygen2

Hmmm... Strange that check-docs job was failed but green.

Warning message: package ‘roxygen’ is not available for this version of R A version of this package for your version of R might be available elsewhere, see the ideas at https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages

Run Rscript --vanilla -e "roxygen2::roxygenize('R-package/', load = 'installed')" || exit 1 ✖ In topic 'lgb_predict_shared_params.Rd': Skipping; no name and/or title.

https://github.com/microsoft/LightGBM/actions/runs/13361788471/job/37312577620?pr=6808#step:6:22

🙈 oy, thank you for catching this.

Looks like we are getting{roxygen2} that comes pre-installed in the container image used for that job.

LightGBM/.github/workflows/static_analysis.yml

Line 54 in 3fad53b

container: rocker/verse

docker run \ --rm \ --platform linux/amd64 \ -it rocker/verse \ Rscript -e "library(roxygen2); print(sessionInfo())"

... other attached packages: [1] roxygen2_7.3.2 ...

That note about lgb_predict_shared_params comes from roxygen2::roxygenize(), so {roxygen2} must be installed. It's just a warning, so it wouldn't cause this to fail.

And then I guess the other reason this isn't failing is that install.packages() returns a 0 exit code for a non-existent package 🙃

Rscript -e "install.packages('doesnotexist', repos = 'https://cran.r-project.org')" # Warning message: # package ‘doesnotexist’ is not available for this version of R echo $? # 0

So many things together led to this silently passing. I'm very very glad you were so thorough and caught this bug that CI didn't, thank you!!

It's just a warning, so it wouldn't cause this to fail.

Ah, ok! Thanks for clarifying. A ✖ sign confused me and I thought that roxygenization was not run at all.

install.packages() returns a 0 exit code for a non-existent package

Thanks for checking this! Indeed, this seems to be an old annoying problem without any easy workaround.
https://stackoverflow.com/q/26244530
r-lib/remotes#755 (comment)

Co-authored-by: Nikita Titov <[email protected]>

StrikerRUS

LGTM, happy to merge this finally!

jameslamb added 2 commits January 30, 2025 22:05

[ci] consolidate R package installs into a CI script

5cfd192

use CRAN_MIRROR and R_LIB_PATH consistently

d033e2a

jameslamb added in progress maintenance labels Jan 31, 2025

jameslamb added 8 commits January 30, 2025 22:15

shorter lines and fix reference

142d699

fix type option

16c8ab4

add extension

fb2d4c7

fix invocations on Windows

5737073

more fixes

aaa228c

binary

27ff6f7

use package_type binary on Windows

ec43edf

Merge branch 'master' of github.com:microsoft/LightGBM into ci/r-deps…

ae10dca

…-scripts

jameslamb commented Feb 3, 2025

View reviewed changes

Update .ci/test-r-package-windows.ps1

061e69f

jameslamb changed the title ~~WIP: [ci] consolidate R package installs into a CI script~~ [ci] consolidate R package installs into a CI script Feb 3, 2025

jameslamb added awaiting review and removed in progress labels Feb 3, 2025

jameslamb marked this pull request as ready for review February 3, 2025 02:32

jameslamb requested review from guolinke, shiyu1994, jmoralez, borchero and StrikerRUS as code owners February 3, 2025 02:32

StrikerRUS reviewed Feb 3, 2025

View reviewed changes

jameslamb and others added 5 commits February 6, 2025 19:53

more fixes

ea988db

Merge branch 'master' into ci/r-deps-scripts

d0427d1

Apply suggestions from code review

21a607b

Co-authored-by: Nikita Titov <[email protected]>

Merge branch 'ci/r-deps-scripts' of github.com:microsoft/LightGBM int…

8cd2627

…o ci/r-deps-scripts

make install-r-deps more flexible

dae117f

jameslamb requested a review from StrikerRUS February 8, 2025 04:22

Merge branch 'master' into ci/r-deps-scripts

e06c709

Merge branch 'master' into ci/r-deps-scripts

66d0b3b

StrikerRUS reviewed Feb 17, 2025

View reviewed changes

jameslamb and others added 2 commits February 17, 2025 19:35

Apply suggestions from code review

0198dbc

Co-authored-by: Nikita Titov <[email protected]>

Merge branch 'master' into ci/r-deps-scripts

76afb30

jameslamb requested a review from StrikerRUS February 18, 2025 01:35

Merge branch 'master' into ci/r-deps-scripts

402bfbf

StrikerRUS approved these changes Feb 24, 2025

View reviewed changes

StrikerRUS removed the awaiting review label Feb 24, 2025

StrikerRUS merged commit 6b624fb into master Feb 24, 2025
50 checks passed

StrikerRUS deleted the ci/r-deps-scripts branch February 24, 2025 00:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ci] consolidate R package installs into a CI script #6808

[ci] consolidate R package installs into a CI script #6808

jameslamb commented Jan 31, 2025 •

edited

Loading

jameslamb Feb 3, 2025

jameslamb Feb 3, 2025

StrikerRUS left a comment

StrikerRUS Feb 3, 2025

jameslamb Feb 8, 2025

StrikerRUS Feb 3, 2025

jameslamb Feb 8, 2025

StrikerRUS Feb 3, 2025

jameslamb Feb 4, 2025

jameslamb Feb 8, 2025 •

edited

Loading

StrikerRUS Feb 3, 2025

jameslamb Feb 8, 2025

StrikerRUS commented Feb 14, 2025 •

edited by shiyu1994

Loading

StrikerRUS left a comment

StrikerRUS Feb 17, 2025

jameslamb Feb 18, 2025

StrikerRUS Feb 24, 2025

StrikerRUS left a comment

	# install dependencies
	RDscript${R_CUSTOMIZATION} \
	-e "install.packages(c('R6', 'data.table', 'jsonlite', 'knitr', 'markdown', 'Matrix', 'RhpcBLASctl', 'testthat'), repos = 'https://cran.r-project.org', Ncpus = parallel::detectCores())"

		# $command_args_patch = "commandArgs <- function(...){c('--build', '--test')};"
		# Invoke-R-Code-Redirect-Stderr "$command_args_patch source('install-r-deps.R')" ; Assert-Output $?

	"Using system2() to run shell commands. Installing "
	, "'processx' with install.packages('processx') might "
	, "make this faster."

	# 'testthat' is not needed by 'rchk', so avoid installing it until here
	Rscript -e "install.packages('testthat', repos = 'https://cran.rstudio.com', Ncpus = parallel::detectCores())"

	Rscript ./.ci/install-r-deps.R --build --include=roxygen
	Rscript ./.ci/install-r-deps.R --build --include=roxygen2

[ci] consolidate R package installs into a CI script #6808

[ci] consolidate R package installs into a CI script #6808

Conversation

jameslamb commented Jan 31, 2025 • edited Loading

Notes for Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrikerRUS 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrikerRUS commented Feb 14, 2025 • edited by shiyu1994 Loading

StrikerRUS 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

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Jan 31, 2025 •

edited

Loading

jameslamb Feb 8, 2025 •

edited

Loading

StrikerRUS commented Feb 14, 2025 •

edited by shiyu1994

Loading