[R-package] remove lgb.unloader()

[jameslamb]

Proposes removing lgb.unloader() from the R package.

Reasons I support removing this:

• simplifies the public API of the R package, reducing the risk that users spend time investigating a function they don't need
• reduces the run time of the package's tests
• removes some tests that might break in future versions of R or {testthat} because of the unorthodox things this function does with environments
  • see, for example, [R-package] [docs] instructions for generating code coverage produce incorrect results #4919 (comment)
• I don't think it's used much, and that it's unnecessary as of [R-package] Factor out custom R interface to lib_lightgbm #3016
  • searching on GitHub (not perfect, I know), the only uses of this function I found were in forks of LightGBM
  • https://github.com/search?q=%22lgb.unloader%22&type=code

Notes for Reviewers

lgb.unloader() was introduced 5+ years ago, in #378. At that time, {lightgbm} was using a custom R-to-C interface, which could result in some situations where Dataset and Booster objects lifecycles' weren't handled in the standard way and users could experience session crashes or freezes when errors occurred (e.g. #2088, #3445, #4208).

As of the changes tied to #3016 (released in v3.3.0), {lightgbm}'s R-to-C interface is now the standard one described in https://cran.r-project.org/doc/manuals/r-release/R-exts.html, and we haven't received reports of such issues.

Now, when the next release will be a v4.0.0 (#5153) with breaking changes, seems like a good time to make this change.

How I tested this

In addition to running the package's unit tests, I also ran the one affected demo.

Rscript R-package/demo/weight_param.R

That ran without issue.

I also searched for other uses of lgb.unloader() like this:

git grep -E 'lgb\.unload'

[StrikerRUS]

Great simplification, thank you!

[jmoralez]

I agree with this. Just a minor suggestion on the R-package/demo/weight_param.R demo, I think we should change the lines a bit after the unloader part (76-84) to something like the following:

# We now create the dataset without specifying weights 
# which makes each sample weigh 1 and thus the sum of weights equal to 6513
dtrain <- lgb.Dataset(train$data, label = train$label)
dtest <- lgb.Dataset.create.valid(dtrain, test$data, label = test$label)
valids <- list(test = dtest)

Which makes me think also line 11 should be:
# - Run 3: sum of weights equal to 6513 with adjusted regularization (learning)
or
# - Run 3: sum of weights equal to 0.06513 (x 1e5) with adjusted regularization (learning)

[jameslamb]

I think we should change the lines a bit after the unloader part (76-84) to something like the following

Sure, I'd be happy to review a PR with proposed changes to the demos. But I'm going to merge this PR, as I don't think your suggestion is directly related to lgb.unloader().

[jmoralez]

They were kind of left over after removing the unloader, since they reload the library and the data

[jameslamb]

Ah ok, sorry. I thought that since you left a "comment" instead of "request changes" review, that meant you were just saying something else you noticed and not requesting changes for this specific PR.

[jmoralez]

They're not really necessary but make it a bit confusing without the unloader part. I can address that in a follow up PR

[jameslamb]

Ah I see ok. Yeah please do, thanks!

[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.