Skip to content
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.

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

[R-package] [ci] Add option to skip installation in build_r.R #2957

Merged
merged 6 commits into from
Mar 31, 2020

Conversation

jameslamb
Copy link
Collaborator

Right now, Rscript build_r.R will build a source distribution of the R package and install it. In this PR, I propose an addition to the script to tell build_r.R to just build the package tarball but not install it.

This should allow us to cut a few minutes out of the r-package tasks in CI!

Right now we are doing this:

Rscript build_r.R
R CMD check lightgbm_*.tar.gz

That means we're building lib_lightgbm twice, without getting any extra testing benefit. In fact, I think that the scenario of checking a source tarball on a system where lightgbm hasn't been installed already is an even better test of the experience for our users!

Note that this PR won't change the existing behavior for anyone using build_r.R. The default will still be to both build and install.

@jameslamb jameslamb requested a review from StrikerRUS March 30, 2020 03:21
@jameslamb jameslamb requested a review from Laurae2 as a code owner March 30, 2020 03:21
@jameslamb jameslamb requested a review from guolinke March 30, 2020 20:49
@StrikerRUS
Copy link
Collaborator

Linking #2441 for example of commandArgs usage.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM! Really huge difference it time! 🎉

build_r.R Outdated Show resolved Hide resolved
@jameslamb
Copy link
Collaborator Author

jameslamb commented Mar 30, 2020

Seeing this on the linux CI task in Azure DevOps and I am very confused

image

I don't understand how this could be failing on Azure DevOps but not Travis 🤔

My best guess is that some very new version of one of our dependencies previously relied on Matrix and just factored it out, and maybe we got a slightly newer version on Linux because we get source packages there and binary packages on Mac.

Either way, we probably should have been installing Matrix all along since it is listed in Imports, so I'll just add it.

EDIT: wait no we already install Matrix! I see this in the logs:

ERROR: dependency ‘lattice’ is not available for package ‘Matrix’

And further up:

image

I hope this was just transient. Pushing another small commit to test.

@jameslamb jameslamb requested a review from chivee as a code owner March 31, 2020 00:14
@jameslamb
Copy link
Collaborator Author

This PR is ready to merge, but I think we're facing a bug Travis faced a few days ago and thought they'd fixed.

Successful build: https://travis-ci.org/github/microsoft/LightGBM/builds/669022302

Details on the issue from https://www.traviscistatus.com/:

image

@jameslamb
Copy link
Collaborator Author

close-reopen to retrigger CI to try to get past transient issues

@jameslamb jameslamb reopened this Mar 31, 2020
@jameslamb jameslamb removed the request for review from chivee March 31, 2020 03:59
@jameslamb
Copy link
Collaborator Author

This seems to be working! Going to merge it so we can start getting the benefits in our CI 😀

@jameslamb jameslamb merged commit dbc3d05 into microsoft:master Mar 31, 2020
@@ -69,10 +69,10 @@ packages="c('data.table', 'jsonlite', 'Matrix', 'R6', 'testthat')"
if [[ $OS_NAME == "macos" ]]; then
packages+=", type = 'binary'"
fi
Rscript --vanilla -e "install.packages(${packages}, repos = '${CRAN_MIRROR}', lib = '${R_LIB_PATH}')" || exit -1
Rscript --vanilla -e "install.packages(${packages}, repos = '${CRAN_MIRROR}', lib = '${R_LIB_PATH}', dependencies = c('Depends', 'Imports', 'LinkingTo'))" || exit -1
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jameslamb
Just curious: what is the default set of options?

Copy link
Collaborator Author

@jameslamb jameslamb Mar 31, 2020

Choose a reason for hiding this comment

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

This set is the default set. From ?install.packages

The default, NA, means c("Depends", "Imports", "LinkingTo").

Those are the minimum ones needed to load a package. In case you're not familiar with them:

  • Depends: Has to be present to install your package. Will be loaded whenever your package is loaded.
  • Imports: Has to be present to install your package. Will only be loaded when your package calls code from them.
  • LinkingTo: Has to be present to install your package. These are usually header-only packages that your code links to, e.g. the BH package for Boost.

This commit just makes that explicit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, thanks a lot for the detailed explanation!

@jameslamb jameslamb deleted the ci/skip-r-install branch April 25, 2020 17:08
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants