[R-package] manage Dataset and Booster handles as R external pointers (fixes #3016) #4265

jameslamb · 2021-05-08T03:01:22Z

Contributes to #4208, might help with #4034.

Description

When the R package calls functions from the C++ side that create persistent objects in memory, like a Dataset or Booster, it stores a pointer to the memory address of such objects in a "handle". Today, these handles are stored in a double on 64-bit systems and as an int32 on 32-bit systems.

LightGBM/R-package/R/utils.R

Lines 9 to 15 in f831808

    
           lgb.null.handle <- function() { 
        
             if (.Machine$sizeof.pointer == 8L) { 
        
               return(NA_real_) 
        
             } else { 
        
               return(NA_integer_) 
        
             } 
        
           }

Today, the R package uses LightGBM's custom R-to-C interface to manage these handles. This can lead to issues like #4208, and generally exposes LightGBM to compatibility problems if new versions of R introduce changes that require corresponding changes in that custom interface.

This PR proposes replacing that custom management of "handles" with R's built-in pattern for such things. R's C API has a concept called "external pointers" designed for exactly this purpose.

From https://cran.r-project.org/doc/manuals/r-release/R-exts.html#External-pointers-and-weak-references

External pointer SEXPs are intended to handle references to C structures such as ‘handles’

Notes for Reviewers

This only fixes #3016 if is is merged after #4256.

The "References" section below contains links to the resources that helped me to understand these concepts better, and they might help reviewers to understand these changes.

References

jameslamb · 2021-05-08T03:47:24Z

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/822296181

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-e77325aba0e449bebe84249015a5240b
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-e494d83effcd4f10b747c25aa2cab9da
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

jameslamb · 2021-05-08T03:47:43Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/822296748

Status: success ✔️.

jameslamb · 2021-05-10T03:28:51Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/826655460

Status: success ✔️.

jameslamb · 2021-05-10T03:28:58Z

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/826655969

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-d68ab45fd2514e2982267336296db17b
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-7798e3815ca24329a8b4477895b98b35
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

StrikerRUS · 2021-05-10T14:20:47Z

Wonderful!
Unfortunately, this PR doesn't fix the following compilation warning

/home/runner/work/LightGBM/LightGBM/lightgbm.Rcheck/00_pkg_src/lightgbm/src/src/lightgbm_R.cpp:720:52: warning: cast between incompatible function types from ‘SEXPREC* (*)(SEXP)’ {aka ‘SEXPREC* (*)(SEXPREC*)’} to ‘DL_FUNC’ {aka ‘void* (*)()’} [-Wcast-function-type]

as it was suspected in #3016 (comment)

I suspect, though I don't know for sure, that this would also resolve these warnings we're getting from some compilers:

Any ideas why it doesn't and whether we should create a separate issue for these warnings as this PR is closing #3016?

jameslamb · 2021-05-10T16:53:16Z

Any ideas why it doesn't and whether we should create a separate issue for these warnings as this PR is closing #3016?

Please create a separate issue for it and I can investigate it (and document which specific combinations of operating system + R version + compiler + installation method it occurs for) in the future.

jameslamb · 2021-05-10T21:15:46Z

Please create a separate issue for it and I can investigate it

@StrikerRUS I'm back at a computer now so I can make this issue.

jameslamb · 2021-05-10T21:41:51Z

created #4273 to track the compiler warning

StrikerRUS · 2021-05-11T15:12:16Z

I'm back at a computer now so I can make this issue.

Thank you very much!

StrikerRUS

Very nice PR!
Please consider checking some my questions and cleanup suggestions below.

R-package/src/R_object_helper.h

R-package/src/lightgbm_R.cpp

R-package/src/lightgbm_R.h

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>

jameslamb · 2021-05-11T17:25:46Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/832600392

Status: failure ❌.

jameslamb · 2021-05-11T17:25:56Z

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/832601079

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-847f0335b7b8431fab8a6bd16e415eb6
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-ab08a050699a44bfbf8d33c119c97f5c
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

StrikerRUS · 2021-05-11T21:28:02Z

Hmmm, valgrind test is failing due to the time limit.

The job running on runner GitHub Actions 78 has exceeded the maximum execution time of 120 minutes.

Rerunning...

StrikerRUS · 2021-05-11T21:28:10Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/833251558

Status: success ✔️.

jameslamb · 2021-05-11T23:08:24Z

Hmmm, valgrind test is failing due to the time limit.

😬 I hope it was something temporary, but I could understand if it wasn't. As of this PR, R is taking on more responsibility for allocating and freeing memory, and we know that the version of R instrumented with valgrind runs significantly slower.

jameslamb · 2021-05-11T23:53:45Z

whew, seems that it succeeded.

Total duration of https://github.com/microsoft/LightGBM/actions/runs/833251558 was 1 hour, 50 minutes, 46 seconds.

I think that if we see timeout issues again in the future, we should increase the allowed time by another 30 minutes.

StrikerRUS · 2021-05-12T21:37:16Z

I think that if we see timeout issues again in the future, we should increase the allowed time by another 30 minutes.

Agree.

StrikerRUS

Woo-hoo! Thank you!

jameslamb · 2021-05-12T21:50:55Z

Excited about this one!!

github-actions · 2023-08-23T22:38:30Z

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.

jameslamb added 10 commits May 5, 2021 19:05

started converting handles

4a2a770

more changes

02e61c4

Merge branch 'master' into r/handles

9762ac3

sort of working for Dataset

0d59568

yay all the tests are passing for Dataset handle changes

53baecf

working for other handle types

8bbec8b

remove debugging logging

280d9d1

remove unnecessary spaces

a35c04b

fix null logic

4912037

more NULL

f4361c0

jameslamb added maintenance in progress labels May 8, 2021

jameslamb changed the title ~~WIP: [R-package] manage Dataset and Booster handles as R external pointers (fixes #3016)~~ [R-package] manage Dataset and Booster handles as R external pointers (fixes #3016) May 8, 2021

jameslamb marked this pull request as ready for review May 8, 2021 04:33

jameslamb requested a review from Laurae2 as a code owner May 8, 2021 04:33

jameslamb removed the in progress label May 8, 2021

jameslamb requested review from StrikerRUS and shiyu1994 May 8, 2021 04:34

jameslamb added 2 commits May 9, 2021 21:58

merge master

125ffb3

updates

7dd74d5

jameslamb mentioned this pull request May 10, 2021

[R-package] gcc warning on CMake-based builds: 'cast between incompatible function types' #4273

Closed

StrikerRUS requested changes May 11, 2021

View reviewed changes

jameslamb and others added 3 commits May 11, 2021 11:17

Apply suggestions from code review

e2d40d4

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>

Merge branch 'master' into r/handles

6f75fea

consolidate steps

1834681

jameslamb requested a review from StrikerRUS May 11, 2021 17:29

StrikerRUS approved these changes May 12, 2021

View reviewed changes

StrikerRUS merged commit 676c95f into microsoft:master May 12, 2021

jameslamb deleted the r/handles branch May 12, 2021 21:50

jameslamb mentioned this pull request May 20, 2021

[R-package] Very large l2 when training model #4305

Closed

jameslamb mentioned this pull request Jun 5, 2021

[R-package] resolve test warning about is.na() and handles #4341

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] manage Dataset and Booster handles as R external pointers (fixes #3016) #4265

[R-package] manage Dataset and Booster handles as R external pointers (fixes #3016) #4265

jameslamb commented May 8, 2021

jameslamb commented May 8, 2021 •

edited by guolinke

Loading

jameslamb commented May 8, 2021 •

edited by guolinke

Loading

jameslamb commented May 10, 2021 •

edited by guolinke

Loading

jameslamb commented May 10, 2021 •

edited by guolinke

Loading

StrikerRUS commented May 10, 2021

jameslamb commented May 10, 2021

jameslamb commented May 10, 2021

jameslamb commented May 10, 2021

StrikerRUS commented May 11, 2021

StrikerRUS left a comment

jameslamb commented May 11, 2021 •

edited by guolinke

Loading

jameslamb commented May 11, 2021 •

edited by guolinke

Loading

StrikerRUS commented May 11, 2021

StrikerRUS commented May 11, 2021 •

edited by guolinke

Loading

jameslamb commented May 11, 2021

jameslamb commented May 11, 2021

StrikerRUS commented May 12, 2021

StrikerRUS left a comment

jameslamb commented May 12, 2021

github-actions bot commented Aug 23, 2023

	lgb.null.handle <- function() {
	if (.Machine$sizeof.pointer == 8L) {
	return(NA_real_)
	} else {
	return(NA_integer_)
	}
	}

[R-package] manage Dataset and Booster handles as R external pointers (fixes #3016) #4265

[R-package] manage Dataset and Booster handles as R external pointers (fixes #3016) #4265

Conversation

jameslamb commented May 8, 2021

Description

Notes for Reviewers

References

jameslamb commented May 8, 2021 • edited by guolinke Loading

jameslamb commented May 8, 2021 • edited by guolinke Loading

jameslamb commented May 10, 2021 • edited by guolinke Loading

jameslamb commented May 10, 2021 • edited by guolinke Loading

StrikerRUS commented May 10, 2021

jameslamb commented May 10, 2021

jameslamb commented May 10, 2021

jameslamb commented May 10, 2021

StrikerRUS commented May 11, 2021

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented May 11, 2021 • edited by guolinke Loading

jameslamb commented May 11, 2021 • edited by guolinke Loading

StrikerRUS commented May 11, 2021

StrikerRUS commented May 11, 2021 • edited by guolinke Loading

jameslamb commented May 11, 2021

jameslamb commented May 11, 2021

StrikerRUS commented May 12, 2021

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented May 12, 2021

github-actions bot commented Aug 23, 2023

jameslamb commented May 8, 2021 •

edited by guolinke

Loading

jameslamb commented May 8, 2021 •

edited by guolinke

Loading

jameslamb commented May 10, 2021 •

edited by guolinke

Loading

jameslamb commented May 10, 2021 •

edited by guolinke

Loading

jameslamb commented May 11, 2021 •

edited by guolinke

Loading

jameslamb commented May 11, 2021 •

edited by guolinke

Loading

StrikerRUS commented May 11, 2021 •

edited by guolinke

Loading