[R-package] enable saving Booster with saveRDS() and loading it with readRDS() (fixes #4296) #4685
Conversation
There was a problem hiding this comment.
Thanks very much for working on this! This is super useful feature, and I'm really excited to add it to the R package 🚀 🚀 🚀
I left a few comments below. Will provide a more thorough review some time in the next few days. I have some travel planned soon, so I apologize if responses are a bit slow over the next week.
For now, I can say for sure that you'll need to regenerate the NAMESPACE file (since you're proposing removing two exported functions).
You can do that by running the following:
sh build-cran-package.sh
R CMD INSTALL --with-keep.source lightgbm_*.tar.gz
# (optional, only necessary if you don't have {roxygen2} installed)
Rscript -e "install.packages('roxygen2', repos = 'https://cran.r-project.org')"
# regenerate docs
cd R-package
Rscript -e "roxygen2::roxygenize(load = 'installed')"I'm mentioning #4296 in this review and adding (fixes #4296) to the description, as it looks like this PR is directly intended to add that feature.
In this project, try to make an effort to reference relevant issues, pull requests, or other links whenever they're directly related to a conversation in this project, so it's easier for reviewers (and people getting here from search engines) to understand why the changes are being made. If there are other relevant links that you think someone reading this PR should see, please add them to the description.
|
@jameslamb The linter is complaining about the python code, which has not been touched in this PR. |
R-package/R/lgb.Booster.R
Outdated
| @@ -784,6 +829,7 @@ predict.lgb.Booster <- function(object, | |||
| if (!lgb.is.Booster(x = object)) { | |||
| stop("predict.lgb.Booster: object should be an ", sQuote("lgb.Booster")) | |||
| } | |||
| object$restore_handle() | |||
There was a problem hiding this comment.
Could this be moved inside Booster$predict()?
That way, it'll be guaranteed to run regardless of whether someone uses predict(bst, data) or bst$predict().
There was a problem hiding this comment.
If it's placed earlier it can throw an error before doing any other long operations with the data. I also assume that since the R6 methods are not documented, they are meant for internal usage, and a user trying to call them directly would likely need to examine the code in any case.
There was a problem hiding this comment.
If it's placed earlier it can throw an error before doing any other long operations with the data.
The only code between this call and Booster$predict() is conversion of ... to a list and possibly raising a deprecation warning.
I'd prefer to concentrate these $restore_handle() calls in the Booster object as much as possible, to minimize how many places in the package's code need to know about managing the raw model object.
I also assume that since the R6 methods are not documented, they are meant for internal usage
The fact that those methods are not documented is a gap that should be filled (not in this PR, please).
However, all of the Booster's public methods except initialize() are treated as part of the public API of the R package. We treat them that way because other exported functions can return Booster instance. For example, lgb.train() returns a Booster instance, and then user code can call any public methods on that instance without needing to use ::: or reach into $.__enclos_env__$private.
In the most recent run of the
That is why the build failed. If you are talking about errors in those logs like |
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
Co-authored-by: James Lamb <jaylamb20@gmail.com>
|
@jameslamb Some R checks say that it failed, but there's no error message. |
In the future, it would be very helpful if you could include links in messages like this, so reviewers can go directly to what you're looking at. I see job https://github.com/microsoft/LightGBM/pull/4685/checks?check_run_id=3908084481, for example, that has the following at the end of its logs.
Scrolling up in the log, I see
My best guess is that that's coming from the new examples you've added in this PR which call Please try adding a rule to https://github.com/microsoft/LightGBM/blob/master/R-package/.Rbuildignore to ignore |
But that's not an error, that's a |
Adding here so that this is findable from search in the future, the specific error is:
Yeah, unfortunately I've found that cloning Eigen from GitLab fails much more often than cloning other submodules hosted on GitHub. Like maybe as often as once a month in this project's CI. I'll re-run these jobs. |
|
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: failure ❌. |
|
/gha run r-solaris Workflow Solaris CRAN check has been triggered! 🚀 solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.3.1.99.tar.gz-5138007eafa6423f8a2ab136fc2447c1 |
|
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: failure ❌. |
|
@jameslamb : that valgrind issue is most definitely not caused by the changes in this PR - probably the issue had not arised before because the code path was not executed in the tests. I'll probably take a look at it during next weekend. |
To make it clear for other reviewers, this test is failing (click here for build logs) because of the following error from valgrind.
Just looking through recent commits since the v3.3.1 release on October 27, I see the following pull requests that touched C/C++ code in this project:
For what it's worth, #4782 did pass the valgrind tests shortly before being merged (#4782 (comment)). I'll try on a separate PR and see if I can reproduce those errors, just to confirm that the issue is not related to this PR. |
|
Looks like thevalgrind job is passing on |
|
@jameslamb : are you able to identify from the logs which is the specific R test that is triggering the valgrind errors? |
I think so! I searched through the logs for the phrase "invalid read", and found the following. valgrind logs (click me)The last line I see before that that looks like a
So I think the errors came from tests somewhere between these two points: In case it helps...there were some changes to loading Boosters from text in #4782. Maybe there is an issue in the interaction between those changes and the changes in this PR. I can try to look later and see if I find anything. |
|
@jameslamb : turns out it was an issue with null terminators being lost in conversions. Should be fixed now. |
|
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
|
thank you so much! Changes look good to me! I'll check back in a bit to see what valgrind says. |
|
/gha run r-solaris Workflow Solaris CRAN check has been triggered! 🚀 solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.3.1.99.tar.gz-0e1b5e71a33b4d42808c0351cd8a6262 |
There was a problem hiding this comment.
Given @StrikerRUS 's support for the general idea of this PR (#4685 (review)), the fact that all CI jobs (including valgrind and Solaris) are passing, and the fact that all my review comments have been addressed thoroughly, I'm approving this PR.
Thanks for the AWESOME work @david-cortes ! This is a really big improvement in usability for the R package 🚀
jameslamb
changed the title
|
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. |
Fixes #4296
This PR adds changes as necessary for model objects to be serializable through functions such as
saveRDSandqsaveby keeping raw bytes in the model object, from which a C++ object can be reconstructed when such object is de-serialized and the pointer reset to null.