[R-package] skip integer categorical feature check when building dataset subset (fixes #6412) #6442

jmoralez · 2024-05-04T04:41:32Z

When creating the folds for cv we slice the dataset, which just calls the constructor passing the same arguments and the used_indices. If free_raw_data = TRUE then by the time this constructor is called there isn't raw data anymore, however, we also don't need to check if the categorical feature indices are out of bounds because this has already been checked when constructing the original dataset (and the categorical features can't be changed once the dataset has been constructed).

This adds a check to see if we're building a subset and skips the validation. We can rely on that condition because it's checked afterwards

LightGBM/R-package/R/lgb.Dataset.R

Lines 202 to 212 in 6e78e69

    
           # not subsetting, constructing from raw data 
        
           if (is.null(private$used_indices)) { 
        
             if (is.null(private$raw_data)) { 
        
               stop(paste0( 
        
                 "Attempting to create a Dataset without any raw data. " 
        
                 , "This can happen if you have called Dataset$finalize() or if this Dataset was saved with saveRDS(). " 
        
                 , "To avoid this error in the future, use lgb.Dataset.save() or " 
        
                 , "Dataset$save_binary() to save lightgbm Datasets." 
        
               )) 
        
             }

jameslamb

Thanks! The fix looks great. I really appreciate the thorough description... makes perfect sense to me.

I just left some comments about testing.

Could you please also add one more test in test_dataset.R checking that the expected error message is actually raised under one of the conditions in that if statement being modified here?

Similar to this one:

LightGBM/R-package/tests/testthat/test_dataset.R

Lines 620 to 622 in 6e78e69

    
           expect_error({ 
        
             lgb.Dataset(raw_mat, categorical_feature = 2L)$construct() 
        
           }, regexp = "supplied a too large value in categorical_feature: 2 but only 1 features")

jameslamb · 2024-05-04T21:20:59Z

R-package/tests/testthat/test_basic.R

@@ -3652,6 +3652,28 @@ test_that("lgb.cv() only prints eval metrics when expected to", {
  )
 })

+test_that("lgb.cv() works with an already constructed dataset with integer categoricals", {


Since this is testing code in lgb.Dataset, would you consider moving it into test_dataset.R? I think it's similar to this one:

LightGBM/R-package/tests/testthat/test_dataset.R

Line 407 in 6e78e69

test_that("lgb.Dataset: should be able to run lgb.cv() immediately after using lgb.Dataset() on a file", {

jameslamb · 2024-05-04T21:24:02Z

R-package/tests/testthat/test_basic.R

+    , verbose = .LGB_VERBOSITY
+    , num_threads = .LGB_MAX_THREADS
+  )
+  lgb.cv(params = params, data = dtrain, nrounds = 1L)


could you add an assertion here that something happened? Just to give us a bit more confidence that this behavior didn't result in something like an early return with no training.

For example, maybe some of these checks that the returned objective contains metrics computed on the validation sets created by lgb.cv():

LightGBM/R-package/tests/testthat/test_basic.R

Lines 469 to 492 in 6e78e69

cv_bst <- lgb.cv(

data = dtrain

, nfold = 5L

, nrounds = nrounds

, params = list(

objective = "binary"

, metric = "auc,binary_error"

, learning_rate = 1.5

, num_leaves = 5L

, verbose = .LGB_VERBOSITY

, num_threads = .LGB_MAX_THREADS

)

)

expect_true(methods::is(cv_bst, "lgb.CVBooster"))

expect_named(

cv_bst$record_evals

, c("start_iter", "valid")

, ignore.order = FALSE

, ignore.case = FALSE

)

auc_scores <- unlist(cv_bst$record_evals[["valid"]][["auc"]][["eval"]])

expect_length(auc_scores, nrounds)

expect_identical(cv_bst$best_iter, which.max(auc_scores))

expect_identical(cv_bst$best_score, auc_scores[which.max(auc_scores)])

jameslamb · 2024-05-28T03:58:39Z

@jmoralez if you have time in the next week or so, could you update this to latest master and consider the review comments? I'd like to get this into v4.4.0 if we can.

jmoralez · 2024-06-10T18:50:38Z

The R-package 3.6 jobs are failing with this message (sample logs):

ERROR: dependency ‘evaluate’ is not available for package ‘knitr’
removing ‘/github/home/Rlib/knitr’
ERROR: dependency ‘evaluate’ is not available for package ‘testthat’
removing ‘/github/home/Rlib/testthat’

It seems evaluate 0.24.0 was published today which requires R >= 4.0.0

jmoralez · 2024-06-10T19:05:39Z

@jameslamb should I add something like Rscript --vanilla -e "install.packages('https://cran.r-project.org/src/contrib/Archive/evaluate/evaluate_0.23.tar.gz', repos = NULL, lib = '${R_LIB_PATH}') after this?

LightGBM/.ci/test_r_package.sh

Line 112 in 4401401

    
           Rscript --vanilla -e "install.packages('https://cran.r-project.org/src/contrib/Archive/lattice/lattice_0.20-41.tar.gz', repos = NULL, lib = '${R_LIB_PATH}')"

mayer79 · 2024-06-10T19:06:08Z

The R-package 3.6 jobs are failing with this message (sample logs):

ERROR: dependency ‘evaluate’ is not available for package ‘knitr’
removing ‘/github/home/Rlib/knitr’
ERROR: dependency ‘evaluate’ is not available for package ‘testthat’
removing ‘/github/home/Rlib/testthat’

It seems evaluate 0.24.0 was published today which requires R >= 4.0.0

Minor remark @jameslamb: Dropping support for R < 4.0.0 should not be a too big deal. The main branch of XGBoost even requires R >= 4.3.0.

jameslamb · 2024-06-10T19:46:52Z

Thanks for investigating it @jmoralez .

I think we should do what we can to preserve R 3.x support for this release (including manually installing more packages in CI if necessary).

The first release of R 4.x was only 4 years ago (https://cran.r-project.org/bin/windows/base/old/).

By comparison, the oldest version of Python LightGBM supports (3.7) was first released about 6 years ago. (https://devguide.python.org/versions/)

Could you try to get this PR working with R 3.6 if it's not too much effort, and write up an [RFC] issue (similar to #6436) where we can discuss and track the work dropping R 4 in the next release? I totally support dropping it in the next release, and including a note in v4.4.0's release notes like "this will be the last release to support R 3.x".

I do hear what you're saying @mayer79 , but I I've worked in organizations where a major-version upgrade of a language happens on a slightly longer timescale than 4 years. If the cost of keeping R 3.x support for the v4.4.0 release is "have to install a few more libraries manually in CI", I think it's worth it to do that.

jmoralez · 2024-06-10T22:05:52Z

It seems there's also a failure in the MSVC job when building the vignettes (this PR, #6467). Not sure how to debug that since the full error is just:

creating vignettes ... ERROR
--- re-building 'basic_walkthrough.Rmd' using knitr

jameslamb · 2024-06-11T02:51:38Z

I can look into the MSVC issues right now. I'll do that over in a separate PR (#6476), in case you want to use this PR to debug ... but if I find a fix, I'll push it here.

fix categorical features for dataset in cv

6305b8d

jmoralez changed the title ~~fix categorical features for dataset in cv~~ [R-package] fix categorical features for dataset in cv May 4, 2024

jmoralez changed the title ~~[R-package] fix categorical features for dataset in cv~~ [R-package] fix integer categorical features check for dataset in lgb.cv May 4, 2024

jmoralez added the fix label May 4, 2024

jmoralez changed the title ~~[R-package] fix integer categorical features check for dataset in lgb.cv~~ [R-package] fix integer categorical features check in lgb.cv May 4, 2024

lint

fe4be1d

jmoralez changed the title ~~[R-package] fix integer categorical features check in lgb.cv~~ [R-package] fix integer categorical features check in lgb.cv (fixes #6412) May 4, 2024

jmoralez marked this pull request as ready for review May 4, 2024 17:49

jmoralez requested review from guolinke, jameslamb, shiyu1994 and borchero as code owners May 4, 2024 17:49

jameslamb requested changes May 4, 2024

View reviewed changes

jmoralez added 6 commits June 10, 2024 12:00

apply suggestions

dcd780f

Merge branch 'master' into fix-cat-check-cv

170d99e

newline

b27028f

fixes

744e3d1

lint

dfb7c35

lint2

9a3341b

manually install evaluate for R<4.x

b4a56ab

jmoralez changed the title ~~[R-package] fix integer categorical features check in lgb.cv (fixes #6412)~~ [R-package] skip integer categorical feature check when building dataset subset (fixes #6412) Jun 10, 2024

jameslamb mentioned this pull request Jun 11, 2024

[DO NOT MERGE] [R-package] [ci] investigate R CI failures #6476

Draft

jameslamb mentioned this pull request Jun 12, 2024

release v4.4.0 #6439

Open

33 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] skip integer categorical feature check when building dataset subset (fixes #6412) #6442

[R-package] skip integer categorical feature check when building dataset subset (fixes #6412) #6442

jmoralez commented May 4, 2024

jameslamb left a comment

jameslamb May 4, 2024

jameslamb May 4, 2024

jameslamb commented May 28, 2024

jmoralez commented Jun 10, 2024

jmoralez commented Jun 10, 2024 •

edited

mayer79 commented Jun 10, 2024 •

edited

jameslamb commented Jun 10, 2024

jmoralez commented Jun 10, 2024

jameslamb commented Jun 11, 2024

	# not subsetting, constructing from raw data
	if (is.null(private$used_indices)) {

	if (is.null(private$raw_data)) {
	stop(paste0(
	"Attempting to create a Dataset without any raw data. "
	, "This can happen if you have called Dataset$finalize() or if this Dataset was saved with saveRDS(). "
	, "To avoid this error in the future, use lgb.Dataset.save() or "
	, "Dataset$save_binary() to save lightgbm Datasets."
	))
	}

	expect_error({
	lgb.Dataset(raw_mat, categorical_feature = 2L)$construct()
	}, regexp = "supplied a too large value in categorical_feature: 2 but only 1 features")

	cv_bst <- lgb.cv(
	data = dtrain
	, nfold = 5L
	, nrounds = nrounds
	, params = list(
	objective = "binary"
	, metric = "auc,binary_error"
	, learning_rate = 1.5
	, num_leaves = 5L
	, verbose = .LGB_VERBOSITY
	, num_threads = .LGB_MAX_THREADS
	)
	)
	expect_true(methods::is(cv_bst, "lgb.CVBooster"))
	expect_named(
	cv_bst$record_evals
	, c("start_iter", "valid")
	, ignore.order = FALSE
	, ignore.case = FALSE
	)
	auc_scores <- unlist(cv_bst$record_evals[["valid"]][["auc"]][["eval"]])
	expect_length(auc_scores, nrounds)
	expect_identical(cv_bst$best_iter, which.max(auc_scores))
	expect_identical(cv_bst$best_score, auc_scores[which.max(auc_scores)])

[R-package] skip integer categorical feature check when building dataset subset (fixes #6412) #6442

Are you sure you want to change the base?

[R-package] skip integer categorical feature check when building dataset subset (fixes #6412) #6442

Conversation

jmoralez commented May 4, 2024

jameslamb left a comment

Choose a reason for hiding this comment

jameslamb May 4, 2024

Choose a reason for hiding this comment

jameslamb May 4, 2024

Choose a reason for hiding this comment

jameslamb commented May 28, 2024

jmoralez commented Jun 10, 2024

jmoralez commented Jun 10, 2024 • edited

mayer79 commented Jun 10, 2024 • edited

jameslamb commented Jun 10, 2024

jmoralez commented Jun 10, 2024

jameslamb commented Jun 11, 2024

jmoralez commented Jun 10, 2024 •

edited

mayer79 commented Jun 10, 2024 •

edited