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

[jmoralez]

Fixes #6412

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]

@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]

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]

@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]

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]

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]

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]

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.

[jameslamb]

👀 I just noticed that all the Windows R-package jobs passed 11 hours ago, on this PR that has nothing to do with the R package:

• PR: Avoid copy on Refit #6478
• build: https://github.com/microsoft/LightGBM/actions/runs/9485380089/job/26153370408

Maybe we're gonna get lucky and the failing MSVC job here was just something temporary with GitHub Actions? I just restarted the failing job here, let's see.

[jameslamb]

changes look great, and thanks for the tests!

[jameslamb]

The job passed!

Ok I'm treating this as just something temporary, possibly fixed by the latest updates to the windows-2022 VM image (actions/runner-images#10041).