From 34547a8f67813cfb7b2abac353a55f3bea37ed13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romain=20Fran=C3=A7ois?= Date: Thu, 7 Dec 2023 16:13:42 +0100 Subject: [PATCH] `execute()` R function don't always return success (#66) * execute return error_reply when error happens * execute also sending "execution_result" structures --- share/jupyter/kernels/xr/resources/execute.R | 23 ++++++++++----- share/jupyter/kernels/xr/resources/routines.R | 9 ------ src/routines.cpp | 29 ------------------- src/xinterpreter.cpp | 29 ++++++++++++++++++- test/test_xr_kernel.py | 2 +- 5 files changed, 44 insertions(+), 48 deletions(-) diff --git a/share/jupyter/kernels/xr/resources/execute.R b/share/jupyter/kernels/xr/resources/execute.R index 3f84c50..d63b71d 100644 --- a/share/jupyter/kernels/xr/resources/execute.R +++ b/share/jupyter/kernels/xr/resources/execute.R @@ -1,5 +1,6 @@ last_plot <- NULL last_visible <- TRUE +last_error <- NULL handle_message <- function(msg) { publish_stream("stderr", conditionMessage(msg)) @@ -55,7 +56,7 @@ handle_error <- function(e) { cli::col_red("--- Traceback"), format(e$trace) ) - publish_execution_error(ename = "ERROR", evalue = "", trace_back) + last_error <<- structure(list(ename = "ERROR", evalue = "", trace_back), class = "error_reply") } else { sys_calls <- sys.calls() sys_calls <- head(tail(sys_calls, -16), -3) @@ -69,7 +70,7 @@ handle_error <- function(e) { cli::col_red("--- Traceback (most recent call last)"), stack ) - publish_execution_error(ename = "ERROR", evalue = evalue, trace_back) + last_error <<- structure(list(ename = "ERROR", evalue = evalue, trace_back), class = "error_reply") } } @@ -125,16 +126,17 @@ send_plot <- function(plot) { } execute <- function(code, execution_counter, silent = FALSE) { + last_error <<- NULL + parsed <- tryCatch( parse(text = code), error = function(e) { msg <- paste(conditionMessage(e), collapse = "\n") - publish_execution_error("PARSE ERROR", msg) - e + last_error <<- structure(list(ename = "PARSE ERROR", evalue = msg), class = "error_reply") } ) - if (inherits(parsed, "error")) return() - + if (!is.null(last_error)) return(last_error) + output_handler <- if (silent) { evaluate::new_output_handler() } else { @@ -147,7 +149,7 @@ execute <- function(code, execution_counter, silent = FALSE) { value = handle_value(execution_counter) ) } - + last_plot <<- NULL last_visible <<- FALSE @@ -159,10 +161,12 @@ execute <- function(code, execution_counter, silent = FALSE) { stop_on_error = 1L, filename = filename ) + if (!is.null(last_error)) return(last_error) if (!silent && !is.null(last_plot)) { tryCatch(send_plot(last_plot), error = handle_error) } + if (!is.null(last_error)) return(last_error) if (isTRUE(last_visible)) { obj <- .Last.value @@ -175,7 +179,10 @@ execute <- function(code, execution_counter, silent = FALSE) { } bundle <- IRdisplay::prepare_mimebundle(obj, mimetypes = mimetypes) - publish_execution_result(execution_counter, bundle$data, bundle$metadata) + + structure(class = "execution_result", + list(toJSON(bundle$data), toJSON(bundle$metadata)) + ) } } diff --git a/share/jupyter/kernels/xr/resources/routines.R b/share/jupyter/kernels/xr/resources/routines.R index 35796f2..55d25e4 100644 --- a/share/jupyter/kernels/xr/resources/routines.R +++ b/share/jupyter/kernels/xr/resources/routines.R @@ -1,12 +1,3 @@ - -publish_execution_error <- function(ename, evalue, trace_back = character()) { - .Call("xeusr_publish_execution_error", ename, evalue, trace_back, PACKAGE = "(embedding)") -} - -publish_execution_result <- function(execution_count, data, metadata = NULL) { - .Call("xeusr_publish_execution_result", as.integer(execution_count), jsonlite::toJSON(data), jsonlite::toJSON(metadata), PACKAGE = "(embedding)") -} - publish_stream <- function(name, text) { .Call("xeusr_publish_stream", name, text, PACKAGE = "(embedding)") } diff --git a/src/routines.cpp b/src/routines.cpp index 0ab3193..6fadaf4 100644 --- a/src/routines.cpp +++ b/src/routines.cpp @@ -26,33 +26,6 @@ SEXP publish_stream(SEXP name_, SEXP text_) { return R_NilValue; } -SEXP publish_execution_error(SEXP ename_, SEXP evalue_, SEXP trace_back_) { - auto ename = CHAR(STRING_ELT(ename_, 0)); - auto evalue = CHAR(STRING_ELT(evalue_, 0)); - - auto n = XLENGTH(trace_back_); - std::vector trace_back(n); - for (decltype(n) i = 0; i < n; i++) { - trace_back[i] = CHAR(STRING_ELT(trace_back_, i)); - } - - xeus_r::get_interpreter()->publish_execution_error(ename, evalue, std::move(trace_back)); - - return R_NilValue; -} - -SEXP publish_execution_result(SEXP execution_count_, SEXP data_, SEXP metadata_) { - int execution_count = INTEGER_ELT(execution_count_, 0); - auto data = nl::json::parse(CHAR(STRING_ELT(data_, 0))); - auto metadata = nl::json::parse(CHAR(STRING_ELT(metadata_, 0))); - - xeus_r::get_interpreter()->publish_execution_result( - execution_count, std::move(data), std::move(metadata) - ); - - return R_NilValue; -} - SEXP display_data(SEXP js_data, SEXP js_metadata){ auto data = nl::json::parse(CHAR(STRING_ELT(js_data, 0))); auto metadata = nl::json::parse(CHAR(STRING_ELT(js_metadata, 0))); @@ -107,8 +80,6 @@ void register_r_routines() { static const R_CallMethodDef callMethods[] = { {"xeusr_kernel_info_request" , (DL_FUNC) &routines::kernel_info_request , 0}, {"xeusr_publish_stream" , (DL_FUNC) &routines::publish_stream , 2}, - {"xeusr_publish_execution_error" , (DL_FUNC) &routines::publish_execution_error , 3}, - {"xeusr_publish_execution_result", (DL_FUNC) &routines::publish_execution_result, 3}, {"xeusr_display_data" , (DL_FUNC) &routines::display_data , 2}, {"xeusr_update_display_data" , (DL_FUNC) &routines::update_display_data , 2}, {"xeusr_clear_output" , (DL_FUNC) &routines::clear_output , 1}, diff --git a/src/xinterpreter.cpp b/src/xinterpreter.cpp index 5bf8e95..15a961a 100644 --- a/src/xinterpreter.cpp +++ b/src/xinterpreter.cpp @@ -89,8 +89,35 @@ nl::json interpreter::execute_request_impl(int execution_counter, // Typicall SEXP silent_ = PROTECT(Rf_ScalarLogical(silent)); SEXP result = r::invoke_xeusr_fn("execute", code_, execution_counter_, silent_); - UNPROTECT(3); + + if (Rf_inherits(result, "error_reply")) { + std::string evalue = CHAR(STRING_ELT(VECTOR_ELT(result, 0), 0)); + std::string ename = CHAR(STRING_ELT(VECTOR_ELT(result, 1), 0)); + + std::vector trace_back; + if (XLENGTH(result) > 2) { + SEXP trace_back_ = VECTOR_ELT(result, 2); + auto n = XLENGTH(trace_back_); + for (decltype(n) i = 0; i < n; i++) { + trace_back.push_back(CHAR(STRING_ELT(trace_back_, i))); + } + } + + publish_execution_error(evalue, ename, trace_back); + + UNPROTECT(3); + return xeus::create_error_reply(evalue, ename, std::move(trace_back)); + } + + if (Rf_inherits(result, "execution_result")) { + SEXP data_ = VECTOR_ELT(result, 0); + SEXP metadata_ = VECTOR_ELT(result, 1); + auto data = nl::json::parse(CHAR(STRING_ELT(data_, 0))); + auto metadata = nl::json::parse(CHAR(STRING_ELT(metadata_, 0))); + publish_execution_result(execution_counter, data, metadata); + } + UNPROTECT(3); return xeus::create_successful_reply(/*payload, user_expressions*/); } diff --git a/test/test_xr_kernel.py b/test/test_xr_kernel.py index 4b95b7f..29edc3f 100644 --- a/test/test_xr_kernel.py +++ b/test/test_xr_kernel.py @@ -27,7 +27,7 @@ class KernelTests(jupyter_kernel_test.KernelTests): # code_page_something = "?cat" # code_clear_output = "clear_output()" - + code_generate_error = "stop('ouch')" code_inspect_sample = "print" complete_code_samples = ["fun()", "1 + 2", "a %>% b", "a |> b()", "a |> b(c = _)"]