-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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] remove lgb.unloader() #5204
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great simplification, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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 |
They were kind of left over after removing the unloader, since they reload the library and the data |
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. |
They're not really necessary but make it a bit confusing without the unloader part. I can address that in a follow up PR |
Ah I see ok. Yeah please do, thanks! |
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. |
Proposes removing
lgb.unloader()
from the R package.Reasons I support removing this:
{testthat}
because of the unorthodox things this function does with environmentsNotes 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 whereDataset
andBooster
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.
That ran without issue.
I also searched for other uses of
lgb.unloader()
like this:git grep -E 'lgb\.unload'