[R-package] deprecate the use of 'info' in Dataset

[jameslamb]

Addresses the rest of #4543 (changes not covered by #4571), as part of #4310.

• deprecates keyword argument info in lgb.Dataset()
• add keyword arguments group, weight, init_score, and label to lgb.Dataset()

Notes for Reviewers

While working through this, I realized that today the only way to pass new parameters through to Dataset when using lgb.Dataset.create.valid() is to pass them through .... So this PR also proposes adding a new keyword params to that function (and the corresponding public method Dataset$create_valid()), so that ... can be eliminated in release 4.0.0 while still allowing customization of parameters within lgb.Dataset.create.valid() calls.

No existing unit tests need to be changed in this PR, since there are not any that initialize a list info and pass it in. All of the tests and demo examples in this project are already passing weight, group, label, and init_score directly as keyword arguments.

[StrikerRUS]

I'm not sure I'm qualified enough to provide a thoughtful review, but I have a few comments/questions below:

[StrikerRUS]

Looks like additional params like min_data are lost here before 4.0.0 release.

[jameslamb]

oh! I don't think so, but it's not obvious at all what I did, so let me share some links.

First, Dataset in the R package is not exported, so its constructor can be considered "private" in the package's API. That isn't true for any other public methods on this class, since they can be called by user code once a function like lgb.Dataset() returns a Dataset. So as long as all the exported methods / functions maintain backwards-compatibility, no user code will be broken.

Since this PR adds each of the elements of INFO_KEYS as keyword arguments in Dataset$new(), the only thing that could be left in ... is information you want to include in params.

As of this PR, exported entrypoints that create a Dataset will handle merging params and ... before calling Dataset$new().

Dataset$create_valid()

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

Lines 136 to 156 in c8212d8

	params <- modifyList(additional_params, params)
	# the Dataset's existing parameters should be overwritten by any passed in to this call
	params <- modifyList(private$params, params)
	# Create new dataset
	ret <- Dataset$new(
	data = data
	, params = params
	, reference = self
	, colnames = private$colnames
	, categorical_feature = private$categorical_feature
	, predictor = private$predictor
	, free_raw_data = private$free_raw_data
	, used_indices = NULL
	, info = info
	, label = label
	, weight = weight
	, group = group
	, init_score = init_score
	)

lgb.Dataset()

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

Lines 798 to 826 in c8212d8

	additional_params <- list(...)
	params <- modifyList(params, additional_params)
	if (length(additional_params) > 0L) {
	warning(paste0(
	"lgb.Dataset: Found the following passed through '...': "
	, paste(names(additional_params), collapse = ", ")
	, ". These will be used, but in future releases of lightgbm, this warning will become an error. "
	, "Add these to 'params' instead. See ?lgb.Dataset for documentation on how to call this function."
	))
	}
	# Create new dataset
	return(
	invisible(Dataset$new(
	data = data
	, params = params
	, reference = reference
	, colnames = colnames
	, categorical_feature = categorical_feature
	, predictor = NULL
	, free_raw_data = free_raw_data
	, used_indices = NULL
	, info = info
	, label = label
	, weight = weight
	, group = group
	, init_score = init_score
	))

=====

this made me realize that one place, Dataset$slice(), is missing the "combine params before calling Dataset$new() logic. I'll open a separate PR for that.

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

Lines 501 to 527 in 32445ab

	slice = function(idxset, ...) {
	additional_params <- list(...)
	if (length(additional_params) > 0L) {
	warning(paste0(
	"Dataset$slice(): Found the following passed through '...': "
	, paste(names(additional_params), collapse = ", ")
	, ". These are ignored and should be removed. "
	, "In future releases of lightgbm, this warning will become an error."
	))
	}
	# Perform slicing
	return(
	Dataset$new(
	data = NULL
	, params = private$params
	, reference = self
	, colnames = private$colnames
	, categorical_feature = private$categorical_feature
	, predictor = private$predictor
	, free_raw_data = private$free_raw_data
	, used_indices = sort(idxset, decreasing = FALSE)
	, info = NULL
	, ...
	)
	)

[StrikerRUS]

As of this PR, exported entrypoints that create a Dataset will handle merging params and ... before calling Dataset$new().

Ah, here is the key point! Thanks a lot for as always detailed explanation!

[StrikerRUS]

I don't have any other comments, thanks!

[jameslamb]

my mistake, this should have been self$get_params(), not private. Will fix in a local commit because I also might as well merge in latest master if this PR is going to get another CI run anyway.

[jameslamb]

I'm not sure I'm qualified enough to provide a thoughtful review

Thanks very much as always for the reviews, @StrikerRUS !

I'm confident in merging this without another reviewer's approval since it's an important step towards the next release, and since:

• @Laurae2 already approved the general approach in Slack and on [RFC] [R-package] Replace "info" interface in lgb.Dataset with keyword arguments #4543
• existing unit tests are passing without any changes

[github-actions]

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.