Skip to content

Commit 3fad53b

Browse files
jameslambjameslamb
authored
[R-package] make finalize() in R6 classes private (#6833)
1 parent 604e2c5 commit 3fad53b

8 files changed

+57
-62
lines changed

‎R-package/DESCRIPTION

+1-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ Suggests:
5353
Depends:
5454
R (>= 3.5)
5555
Imports:
56-
R6 (>= 2.0),
56+
R6 (>= 2.4.0),
5757
data.table (>= 1.9.6),
5858
graphics,
5959
jsonlite (>= 1.0),

‎R-package/R/lgb.Booster.R

+11-10
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,6 @@ Booster <- R6::R6Class(
1111
record_evals = list(),
1212
data_processor = NULL,
1313

14-
# Finalize will free up the handles
15-
finalize = function() {
16-
.Call(
17-
LGBM_BoosterFree_R
18-
, private$handle
19-
)
20-
private$handle <- NULL
21-
return(invisible(NULL))
22-
},
23-
2414
# Initialize will create a starter booster
2515
initialize = function(params = list(),
2616
train_set = NULL,
@@ -711,6 +701,17 @@ Booster <- R6::R6Class(
711701
set_objective_to_none = FALSE,
712702
train_set_version = 0L,
713703
fast_predict_config = list(),
704+
705+
# finalize() will free up the handles
706+
finalize = function() {
707+
.Call(
708+
LGBM_BoosterFree_R
709+
, private$handle
710+
)
711+
private$handle <- NULL
712+
return(invisible(NULL))
713+
},
714+
714715
# Predict data
715716
inner_predict = function(idx) {
716717

‎R-package/R/lgb.Dataset.R

+15-15
Original file line numberDiff line numberDiff line change
@@ -30,16 +30,6 @@ Dataset <- R6::R6Class(
3030
cloneable = FALSE,
3131
public = list(
3232

33-
# Finalize will free up the handles
34-
finalize = function() {
35-
.Call(
36-
LGBM_DatasetFree_R
37-
, private$handle
38-
)
39-
private$handle <- NULL
40-
return(invisible(NULL))
41-
},
42-
4333
# Initialize will create a starter dataset
4434
initialize = function(data,
4535
params = list(),
@@ -210,7 +200,7 @@ Dataset <- R6::R6Class(
210200
if (is.null(private$raw_data)) {
211201
stop(paste0(
212202
"Attempting to create a Dataset without any raw data. "
213-
, "This can happen if you have called Dataset$finalize() or if this Dataset was saved with saveRDS(). "
203+
, "This can happen if the Dataset's finalizer was called or if this Dataset was saved with saveRDS(). "
214204
, "To avoid this error in the future, use lgb.Dataset.save() or "
215205
, "Dataset$save_binary() to save lightgbm Datasets."
216206
))
@@ -608,7 +598,7 @@ Dataset <- R6::R6Class(
608598
# If updating failed but raw data is available, modify the params
609599
# on the R side and re-set ("deconstruct") the Dataset
610600
private$params <- new_params
611-
self$finalize()
601+
private$finalize()
612602
})
613603
}
614604
return(invisible(self))
@@ -649,7 +639,7 @@ Dataset <- R6::R6Class(
649639
private$categorical_feature <- categorical_feature
650640

651641
# Finalize and return self
652-
self$finalize()
642+
private$finalize()
653643
return(invisible(self))
654644

655645
},
@@ -681,7 +671,7 @@ Dataset <- R6::R6Class(
681671
private$reference <- reference
682672

683673
# Finalize and return self
684-
self$finalize()
674+
private$finalize()
685675
return(invisible(self))
686676

687677
},
@@ -713,6 +703,16 @@ Dataset <- R6::R6Class(
713703
info = NULL,
714704
version = 0L,
715705

706+
# finalize() will free up the handles
707+
finalize = function() {
708+
.Call(
709+
LGBM_DatasetFree_R
710+
, private$handle
711+
)
712+
private$handle <- NULL
713+
return(invisible(NULL))
714+
},
715+
716716
get_handle = function() {
717717

718718
# Get handle and construct if needed
@@ -749,7 +749,7 @@ Dataset <- R6::R6Class(
749749
private$predictor <- predictor
750750

751751
# Finalize and return self
752-
self$finalize()
752+
private$finalize()
753753
return(invisible(self))
754754

755755
}

‎R-package/R/lgb.Predictor.R

+12-18
Original file line numberDiff line numberDiff line change
@@ -8,24 +8,6 @@ Predictor <- R6::R6Class(
88
cloneable = FALSE,
99
public = list(
1010

11-
# Finalize will free up the handles
12-
finalize = function() {
13-
14-
# Check the need for freeing handle
15-
if (private$need_free_handle) {
16-
17-
.Call(
18-
LGBM_BoosterFree_R
19-
, private$handle
20-
)
21-
private$handle <- NULL
22-
23-
}
24-
25-
return(invisible(NULL))
26-
27-
},
28-
2911
# Initialize will create a starter model
3012
initialize = function(modelfile, params = list(), fast_predict_config = list()) {
3113
private$params <- .params2str(params = params)
@@ -531,5 +513,17 @@ Predictor <- R6::R6Class(
531513
.equal_or_both_null(private$fast_predict_config$num_iteration, num_iteration)
532514
)
533515
}
516+
517+
# finalize() will free up the handles
518+
, finalize = function() {
519+
if (private$need_free_handle) {
520+
.Call(
521+
LGBM_BoosterFree_R
522+
, private$handle
523+
)
524+
private$handle <- NULL
525+
}
526+
return(invisible(NULL))
527+
}
534528
)
535529
)

‎R-package/src/lightgbm_R.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ void _DatasetFinalizer(SEXP handle) {
270270
SEXP LGBM_NullBoosterHandleError_R() {
271271
Rf_error(
272272
"Attempting to use a Booster which no longer exists and/or cannot be restored. "
273-
"This can happen if you have called Booster$finalize() "
273+
"This can happen if the Booster's finalizer was called "
274274
"or if this Booster was saved through saveRDS() using 'serializable=FALSE'.");
275275
return R_NilValue;
276276
}
@@ -285,7 +285,7 @@ void _AssertDatasetHandleNotNull(SEXP handle) {
285285
if (Rf_isNull(handle) || !R_ExternalPtrAddr(handle)) {
286286
Rf_error(
287287
"Attempting to use a Dataset which no longer exists. "
288-
"This can happen if you have called Dataset$finalize() or if this Dataset was saved with saveRDS(). "
288+
"This can happen if the Dataset's finalizer was called or if this Dataset was saved with saveRDS(). "
289289
"To avoid this error in the future, use lgb.Dataset.save() or Dataset$save_binary() to save lightgbm Datasets.");
290290
}
291291
}

‎R-package/tests/testthat/test_Predictor.R

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
library(Matrix)
22

3-
test_that("Predictor$finalize() should not fail", {
3+
test_that("Predictor's finalizer should not fail", {
44
X <- as.matrix(as.integer(iris[, "Species"]), ncol = 1L)
55
y <- iris[["Sepal.Length"]]
66
dtrain <- lgb.Dataset(X, label = y)
@@ -21,11 +21,11 @@ test_that("Predictor$finalize() should not fail", {
2121

2222
expect_false(.is_null_handle(predictor$.__enclos_env__$private$handle))
2323

24-
predictor$finalize()
24+
predictor$.__enclos_env__$private$finalize()
2525
expect_true(.is_null_handle(predictor$.__enclos_env__$private$handle))
2626

2727
# calling finalize() a second time shouldn't cause any issues
28-
predictor$finalize()
28+
predictor$.__enclos_env__$private$finalize()
2929
expect_true(.is_null_handle(predictor$.__enclos_env__$private$handle))
3030
})
3131

‎R-package/tests/testthat/test_dataset.R

+3-3
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ test_that("Dataset$update_params() works correctly for recognized Dataset parame
351351
}
352352
})
353353

354-
test_that("Dataset$finalize() should not fail on an already-finalized Dataset", {
354+
test_that("Dataset's finalizer should not fail on an already-finalized Dataset", {
355355
dtest <- lgb.Dataset(
356356
data = test_data
357357
, label = test_label
@@ -361,11 +361,11 @@ test_that("Dataset$finalize() should not fail on an already-finalized Dataset",
361361
dtest$construct()
362362
expect_false(.is_null_handle(dtest$.__enclos_env__$private$handle))
363363

364-
dtest$finalize()
364+
dtest$.__enclos_env__$private$finalize()
365365
expect_true(.is_null_handle(dtest$.__enclos_env__$private$handle))
366366

367367
# calling finalize() a second time shouldn't cause any issues
368-
dtest$finalize()
368+
dtest$.__enclos_env__$private$finalize()
369369
expect_true(.is_null_handle(dtest$.__enclos_env__$private$handle))
370370
})
371371

‎R-package/tests/testthat/test_lgb.Booster.R

+10-10
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
test_that("Booster$finalize() should not fail", {
1+
test_that("Booster's finalizer should not fail", {
22
X <- as.matrix(as.integer(iris[, "Species"]), ncol = 1L)
33
y <- iris[["Sepal.Length"]]
44
dtrain <- lgb.Dataset(X, label = y)
@@ -15,11 +15,11 @@ test_that("Booster$finalize() should not fail", {
1515

1616
expect_false(.is_null_handle(bst$.__enclos_env__$private$handle))
1717

18-
bst$finalize()
18+
bst$.__enclos_env__$private$finalize()
1919
expect_true(.is_null_handle(bst$.__enclos_env__$private$handle))
2020

2121
# calling finalize() a second time shouldn't cause any issues
22-
bst$finalize()
22+
bst$.__enclos_env__$private$finalize()
2323
expect_true(.is_null_handle(bst$.__enclos_env__$private$handle))
2424
})
2525

@@ -195,7 +195,7 @@ test_that("Loading a Booster from a text file works", {
195195
lgb.save(bst, model_file)
196196

197197
# finalize the booster and destroy it so you know we aren't cheating
198-
bst$finalize()
198+
bst$.__enclos_env__$private$finalize()
199199
expect_null(bst$.__enclos_env__$private$handle)
200200
rm(bst)
201201

@@ -238,7 +238,7 @@ test_that("boosters with linear models at leaves can be written to text file and
238238
preds <- predict(bst, X)
239239
model_file <- tempfile(fileext = ".model")
240240
lgb.save(bst, model_file)
241-
bst$finalize()
241+
bst$.__enclos_env__$private$finalize()
242242
expect_null(bst$.__enclos_env__$private$handle)
243243
rm(bst)
244244

@@ -275,7 +275,7 @@ test_that("Loading a Booster from a string works", {
275275
model_string <- bst$save_model_to_string()
276276

277277
# finalize the booster and destroy it so you know we aren't cheating
278-
bst$finalize()
278+
bst$.__enclos_env__$private$finalize()
279279
expect_null(bst$.__enclos_env__$private$handle)
280280
rm(bst)
281281

@@ -313,7 +313,7 @@ test_that("Saving a large model to string should work", {
313313
expect_gt(nchar(model_string), 1024L * 1024L)
314314

315315
# finalize the booster and destroy it so you know we aren't cheating
316-
bst$finalize()
316+
bst$.__enclos_env__$private$finalize()
317317
expect_null(bst$.__enclos_env__$private$handle)
318318
rm(bst)
319319

@@ -383,7 +383,7 @@ test_that("If a string and a file are both passed to lgb.load() the file is used
383383
lgb.save(bst, model_file)
384384

385385
# finalize the booster and destroy it so you know we aren't cheating
386-
bst$finalize()
386+
bst$.__enclos_env__$private$finalize()
387387
expect_null(bst$.__enclos_env__$private$handle)
388388
rm(bst)
389389

@@ -1508,7 +1508,7 @@ test_that("boosters with linear models at leaves can be written to RDS and re-lo
15081508
preds <- predict(bst, X)
15091509
model_file <- tempfile(fileext = ".rds")
15101510
saveRDS(bst, file = model_file)
1511-
bst$finalize()
1511+
bst$.__enclos_env__$private$finalize()
15121512
expect_null(bst$.__enclos_env__$private$handle)
15131513
rm(bst)
15141514

@@ -1562,7 +1562,7 @@ test_that("Booster's print, show, and summary work correctly", {
15621562
.has_expected_content_for_fitted_model(log_txt)
15631563

15641564
#--- should not fail for finalized models ---#
1565-
model$finalize()
1565+
model$.__enclos_env__$private$finalize()
15661566

15671567
# print()
15681568
log_txt <- capture.output({

0 commit comments

Comments
 (0)