-
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] Factor out custom R interface to lib_lightgbm #3016
Comments
Closing this in favor of being in #2302 , the place where we keep all feature requests. Anyone is welcome to contribute this! If you want to work on it, leave a comment here. |
I don't get the context in full:
|
This is related to the fact that we also offer CMake-based builds of the package (using a different toolchain from CRAN) for some features that are difficult to get to CRAN or prohibited by CRAN's policies, like a GPU-enabled version of the package and builds with MSVC. You can see https://github.com/microsoft/LightGBM/blob/master/build_r.R and https://github.com/microsoft/LightGBM/tree/master/R-package#install for reference.
When the LightGBM R package was first created, the original authors were concerned that pieces of LightGBM/R-package/src/R_object_helper.h Lines 8 to 11 in 13d0cee
That historical legal stuff is the only reason.
Right now
I don't understand this question, sorry. Would appreciate some specific links to code or documentation.
The handles created to data are doubles with 64-bit R and integers with 32-bit R. Lines 9 to 15 in 13d0cee
Does that answer your question?
When LightGBM/include/LightGBM/utils/log.h Lines 132 to 159 in 13d0cee
LightGBM/include/LightGBM/utils/log.h Lines 121 to 128 in 13d0cee
|
These are the destructors that I'm talking about: LightGBM/R-package/src/lightgbm_R.cpp Line 314 in 1e95cb0
LightGBM/R-package/src/lightgbm_R.cpp Line 196 in 1e95cb0
Which are put under R6 finalizers: LightGBM/R-package/R/lgb.Booster.R Line 13 in 1e95cb0
LightGBM/R-package/R/lgb.Dataset.R Line 10 in 1e95cb0
And BTW using them like this currently can lead to segmentation faults as their validity is not checked (#4208). Another thing: |
Oh interesting! It sounds like you're really knowledgeable about this. We'd really appreciate some contributions if you see opportunities to improve the way the package is set up. It doesn't have to be one large PR that completely solves this issue. For example, the point you made about using If you don't have the time or interest we'll understand and I will try to do what I can based on your detailed write-ups here. |
Would you be willing to add a dependency on either Rcpp or cpp11? That'd make things a lot easier. |
I'm opposed to adding a new dependency unless we really have to, and at this moment I'm not convinced that it's necessary.
|
@david-cortes are you interested in contributing fixes for this issue in the next few weeks? If you aren't, I'm going to make it my next priority in this project (using all the excellent insight you've given us in recent comments). Just want to know so you and I aren't working on the same thing at the same time. |
I don't have any plans to work on these issues right now, but feel free to ask if you need more suggestions. |
Ok great, thank you so much! I probably will be asking you. We really appreciate it!! I recognized you from |
…fixes #3016) (#4265) * started converting handles * more changes * sort of working for Dataset * yay all the tests are passing for Dataset handle changes * working for other handle types * remove debugging logging * remove unnecessary spaces * fix null logic * more NULL * updates * Apply suggestions from code review Co-authored-by: Nikita Titov <nekit94-08@mail.ru> * consolidate steps Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Summary
The R package calls a C++ library (
lib_lightgbm.so
) with the R function.Call()
. Doing this requires an interface to map R data types to C data types and back. Right now we do not use the SEXP ("S expression") interface that ships with R's built-in headers.So far our custom implementation has worked well, but there is a risk that future versions of R will not work with it.
To close this issue, remove
R_object_helper.h
, includingLGBM_SER
andLGBM_SE
. Replace them with the corresponding symbols fromR.h
,Rinternals.h
, and the other R built-in headers.It is likely possible to do this in pieces....it does not have to be a single pull request.
Motivation
This may not be strictly necessary to get to CRAN, but it will bring the R package into tighter compatibility with the R build tools, so should improve the chances we'll be able to support new versions of R with no changes.
I suspect, though I don't know for sure, that this would also resolve these warnings we're getting from some compilers:
See #3009 (comment) and
The text was updated successfully, but these errors were encountered: