[R-package] Move error handling into C++ side (#4163)

* [R-package] raise errors from C++ side * working but a lot of warnings * more changes * simplify * cleanup * linting * fix valgrind issues * revert unnecessary change
microsoft · Apr 22, 2021 · 1e95cb0 · 1e95cb0
1 parent 0a847ef
commit 1e95cb0
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 22 deletions.
diff --git a/R-package/R/lgb.Dataset.R b/R-package/R/lgb.Dataset.R
@@ -545,34 +545,37 @@ Dataset <- R6::R6Class(
 
     },
 
-    # Update parameters
+    # [description] Update Dataset parameters. If it has not been constructed yet,
+    #               this operation just happens on the R side (updating private$params).
+    #               If it has been constructed, parameters will be updated on the C++ side.
     update_params = function(params) {
       if (length(params) == 0L) {
         return(invisible(self))
       }
       if (lgb.is.null.handle(x = private$handle)) {
         private$params <- modifyList(private$params, params)
       } else {
-        call_state <- 0L
-        call_state <- .Call(
-          "LGBM_DatasetUpdateParamChecking_R"
-          , lgb.params2str(params = private$params)
-          , lgb.params2str(params = params)
-          , call_state
-          , PACKAGE = "lib_lightgbm"
-        )
-        call_state <- as.integer(call_state)
-        if (call_state != 0L) {
-
-          # raise error if raw data is freed
+        tryCatch({
+          call_state <- 0L
+          .Call(
+            "LGBM_DatasetUpdateParamChecking_R"
+            , lgb.params2str(params = private$params)
+            , lgb.params2str(params = params)
+            , call_state
+            , PACKAGE = "lib_lightgbm"
+          )
+        }, error = function(e) {
+          # If updating failed but raw data is not available, raise an error because
+          # achieving what the user asked for is not possible
           if (is.null(private$raw_data)) {
-            lgb.last_error()
+            stop(e)
           }
 
-          # Overwrite paramms
+          # If updating failed but raw data is available, modify the params
+          # on the R side and re-set ("deconstruct") the Dataset
           private$params <- modifyList(private$params, params)
           self$finalize()
-        }
+        })
       }
       return(invisible(self))
 

diff --git a/R-package/R/utils.R b/R-package/R/utils.R
@@ -80,11 +80,6 @@ lgb.call <- function(fun_name, ret, ...) {
       , PACKAGE = "lib_lightgbm"
     )
   }
-  call_state <- as.integer(call_state)
-  # Check for call state value post call
-  if (call_state != 0L) {
-    lgb.last_error()
-  }
 
   return(ret)
 

diff --git a/R-package/src/lightgbm_R.cpp b/R-package/src/lightgbm_R.cpp
@@ -12,6 +12,10 @@
 
 #include <R_ext/Rdynload.h>
 
+#define R_NO_REMAP
+#define R_USE_C99_IN_CXX
+#include <R_ext/Error.h>
+
 #include <string>
 #include <cstdio>
 #include <cstring>
@@ -31,7 +35,7 @@
 
 #define CHECK_CALL(x) \
   if ((x) != 0) { \
-    R_INT_PTR(call_state)[0] = -1;\
+    Rf_error(LGBM_GetLastError()); \
     return call_state;\
   }
 

diff --git a/R-package/tests/testthat/test_dataset.R b/R-package/tests/testthat/test_dataset.R
@@ -89,6 +89,19 @@ test_that("lgb.Dataset: Dataset should be able to construct from matrix and retu
   handle <- NULL
 })
 
+test_that("cpp errors should be raised as proper R errors", {
+  data(agaricus.train, package = "lightgbm")
+  train <- agaricus.train
+  dtrain <- lgb.Dataset(
+    train$data
+    , label = train$label
+    , init_score = seq_len(10L)
+  )
+  expect_error({
+    dtrain$construct()
+  }, regexp = "Initial score size doesn't match data size")
+})
+
 test_that("lgb.Dataset$setinfo() should convert 'group' to integer", {
   ds <- lgb.Dataset(
     data = matrix(rnorm(100L), nrow = 50L, ncol = 2L)