From b669a199f6c0231c04d2218f619029dfbc0a1d23 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 13 Apr 2026 11:36:20 -0500 Subject: [PATCH 1/5] test: capture Windows path regression in kaleido --- tests/testthat/test-kaleido.R | 45 +++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 tests/testthat/test-kaleido.R diff --git a/tests/testthat/test-kaleido.R b/tests/testthat/test-kaleido.R new file mode 100644 index 0000000000..fe401a08a8 --- /dev/null +++ b/tests/testthat/test-kaleido.R @@ -0,0 +1,45 @@ +test_that("newKaleidoScope does not inline Windows temp paths into Python code", { + skip_if_not_installed("reticulate") + skip_if_not(suppressWarnings(reticulate::py_available(TRUE))) + + win_path <- "C:\\users\\name\\AppData\\Local\\Temp\\Rtmp\\file.json" + py_calls <- character() + write_fig_args <- NULL + + testthat::local_mocked_bindings( + plotly_build = function(...) { + list(x = list(data = list(), layout = list(), config = list())) + }, + to_JSON = function(...) "{}", + plotlyMainBundlePath = function() "plotly.min.js", + .package = "plotly" + ) + + testthat::local_mocked_bindings( + tempfile = function(...) win_path, + writeLines = function(...) NULL, + .package = "base" + ) + + testthat::local_mocked_bindings( + py_run_string = function(code, ...) { + py_calls <<- c(py_calls, code) + py_obj <- get("py", envir = asNamespace("reticulate")) + py_obj$fig <- "fake-fig" + invisible(NULL) + }, + .package = "reticulate" + ) + + kaleido <- list(write_fig_sync = function(fig, file, opts, kopts) { + write_fig_args <<- list(fig = fig, file = file, opts = opts, kopts = kopts) + invisible(NULL) + }) + + scope <- newKaleidoScope(kaleido) + scope$transform(list(), "figure.png") + + expect_identical(py_calls, "import json; fig = json.load(open(tmp_json_path))") + expect_identical(write_fig_args$fig, "fake-fig") + expect_identical(write_fig_args$file, "figure.png") +}) From a3ba3f9c8586feb30374899a8db4bb68066a5a0d Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 13 Apr 2026 11:36:53 -0500 Subject: [PATCH 2/5] fix: avoid embedding Windows paths in kaleido Python code --- R/kaleido.R | 47 ++++++++++++++++++++++++++++++----- tests/testthat/test-kaleido.R | 3 ++- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/R/kaleido.R b/R/kaleido.R index 0f0224be14..f8a1884142 100644 --- a/R/kaleido.R +++ b/R/kaleido.R @@ -133,11 +133,10 @@ newKaleidoScope <- function(kaleido) { writeLines(fig, tmp_json) # Import it as a fig (dict) - load_json <- sprintf( - "import json; fig = json.load(open('%s'))", - tmp_json + .py_run_string_with_context( + "import json; fig = json.load(open(tmp_json_path))", + context = list(tmp_json_path = tmp_json) ) - reticulate::py_run_string(load_json) # Gather figure-level options opts <- list( @@ -193,8 +192,9 @@ legacyKaleidoScope <- function(kaleido) { ) # Write the base64 encoded string that transform() returns to disk # https://github.com/plotly/Kaleido/blame/master/README.md#L52 - reticulate::py_run_string( - sprintf("import sys; open('%s', 'wb').write(%s)", file, transform_cmd) + .py_run_string_with_context( + sprintf("import sys; open(output_file, 'wb').write(%s)", transform_cmd), + context = list(output_file = file) ) invisible(file) @@ -216,6 +216,41 @@ legacyKaleidoScope <- function(kaleido) { res } +.py_run_string_with_context <- function(code, context = list(), convert = TRUE) { + py <- reticulate::py + context_names <- names(context) + old_values <- vector("list", length(context)) + had_value <- logical(length(context)) + + if (length(context) > 0) { + if (is.null(context_names) || any(context_names == "")) { + rlang::abort("`context` must be a named list.") + } + + for (i in seq_along(context)) { + name <- context_names[[i]] + had_value[[i]] <- reticulate::py_has_attr(py, name) + if (had_value[[i]]) { + old_values[[i]] <- py[[name]] + } + py[[name]] <- context[[i]] + } + + on.exit({ + for (i in rev(seq_along(context))) { + name <- context_names[[i]] + if (had_value[[i]]) { + py[[name]] <- old_values[[i]] + } else { + reticulate::py_run_string(paste("del", name)) + } + } + }, add = TRUE) + } + + reticulate::py_run_string(code, convert = convert) +} + #' Print method for kaleido #' diff --git a/tests/testthat/test-kaleido.R b/tests/testthat/test-kaleido.R index fe401a08a8..d0c193972c 100644 --- a/tests/testthat/test-kaleido.R +++ b/tests/testthat/test-kaleido.R @@ -39,7 +39,8 @@ test_that("newKaleidoScope does not inline Windows temp paths into Python code", scope <- newKaleidoScope(kaleido) scope$transform(list(), "figure.png") - expect_identical(py_calls, "import json; fig = json.load(open(tmp_json_path))") + expect_identical(py_calls[[1]], "import json; fig = json.load(open(tmp_json_path))") + expect_identical(py_calls[[2]], "del tmp_json_path") expect_identical(write_fig_args$fig, "fake-fig") expect_identical(write_fig_args$file, "figure.png") }) From 1282fd4987672231b3b8cec11db0b85091c6c514 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 13 Apr 2026 12:02:16 -0500 Subject: [PATCH 3/5] docs: note Windows save_image fix in news --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index deae87ce4d..1876415b69 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # plotly (development version) +## Bug fixes + +* Closed #2483: `save_image()` no longer embeds Windows file paths directly into Python source passed to `reticulate`, fixing static image export with Kaleido on Windows. From cc992eaa3c41103388f008b31b8504aa6bf931f9 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 13 Apr 2026 14:08:48 -0500 Subject: [PATCH 4/5] test: tighten kaleido review follow-ups --- R/kaleido.R | 3 ++ tests/testthat/test-kaleido.R | 72 +++++++++++++++++++++-------------- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/R/kaleido.R b/R/kaleido.R index f8a1884142..13382f1a7b 100644 --- a/R/kaleido.R +++ b/R/kaleido.R @@ -226,6 +226,9 @@ legacyKaleidoScope <- function(kaleido) { if (is.null(context_names) || any(context_names == "")) { rlang::abort("`context` must be a named list.") } + if (any(!grepl("^[A-Za-z_][A-Za-z0-9_]*$", context_names))) { + rlang::abort("`context` names must be valid Python identifiers.") + } for (i in seq_along(context)) { name <- context_names[[i]] diff --git a/tests/testthat/test-kaleido.R b/tests/testthat/test-kaleido.R index d0c193972c..79631b06e9 100644 --- a/tests/testthat/test-kaleido.R +++ b/tests/testthat/test-kaleido.R @@ -6,41 +6,57 @@ test_that("newKaleidoScope does not inline Windows temp paths into Python code", py_calls <- character() write_fig_args <- NULL - testthat::local_mocked_bindings( - plotly_build = function(...) { - list(x = list(data = list(), layout = list(), config = list())) - }, - to_JSON = function(...) "{}", - plotlyMainBundlePath = function() "plotly.min.js", - .package = "plotly" - ) + local({ + testthat::local_mocked_bindings( + plotly_build = function(...) { + list(x = list(data = list(), layout = list(), config = list())) + }, + to_JSON = function(...) "{}", + plotlyMainBundlePath = function() "plotly.min.js", + .package = "plotly" + ) - testthat::local_mocked_bindings( - tempfile = function(...) win_path, - writeLines = function(...) NULL, - .package = "base" - ) + testthat::local_mocked_bindings( + tempfile = function(...) win_path, + writeLines = function(...) NULL, + unlink = function(...) 0, + .package = "base" + ) - testthat::local_mocked_bindings( - py_run_string = function(code, ...) { - py_calls <<- c(py_calls, code) - py_obj <- get("py", envir = asNamespace("reticulate")) - py_obj$fig <- "fake-fig" + testthat::local_mocked_bindings( + py_run_string = function(code, ...) { + py_calls <<- c(py_calls, code) + py_obj <- get("py", envir = asNamespace("reticulate")) + py_obj$fig <- "fake-fig" + invisible(NULL) + }, + .package = "reticulate" + ) + + kaleido <- list(write_fig_sync = function(fig, file, opts, kopts) { + write_fig_args <<- list(fig = fig, file = file, opts = opts, kopts = kopts) invisible(NULL) - }, - .package = "reticulate" - ) + }) - kaleido <- list(write_fig_sync = function(fig, file, opts, kopts) { - write_fig_args <<- list(fig = fig, file = file, opts = opts, kopts = kopts) - invisible(NULL) + scope <- plotly:::newKaleidoScope(kaleido) + scope$transform(list(), "figure.png") }) - scope <- newKaleidoScope(kaleido) - scope$transform(list(), "figure.png") - expect_identical(py_calls[[1]], "import json; fig = json.load(open(tmp_json_path))") - expect_identical(py_calls[[2]], "del tmp_json_path") + expect_true(!grepl(win_path, py_calls[[1]], fixed = TRUE)) + if (length(py_calls) >= 2) { + expect_identical(py_calls[[2]], "del tmp_json_path") + } expect_identical(write_fig_args$fig, "fake-fig") expect_identical(write_fig_args$file, "figure.png") }) + +test_that("py_run_string_with_context rejects invalid Python identifiers", { + expect_error( + plotly:::.py_run_string_with_context( + "value = 1", + context = list("bad name" = 1) + ), + "`context` names must be valid Python identifiers\\." + ) +}) From b90666a3b97fb8c3eea71f68b6dd090a90d49ab5 Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 13 Apr 2026 16:22:52 -0500 Subject: [PATCH 5/5] fix: harden kaleido context cleanup --- R/kaleido.R | 30 ++++++++++++----------- tests/testthat/test-kaleido.R | 45 +++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 14 deletions(-) diff --git a/R/kaleido.R b/R/kaleido.R index 13382f1a7b..03c0703a61 100644 --- a/R/kaleido.R +++ b/R/kaleido.R @@ -217,10 +217,10 @@ legacyKaleidoScope <- function(kaleido) { } .py_run_string_with_context <- function(code, context = list(), convert = TRUE) { - py <- reticulate::py context_names <- names(context) old_values <- vector("list", length(context)) had_value <- logical(length(context)) + was_set <- logical(length(context)) if (length(context) > 0) { if (is.null(context_names) || any(context_names == "")) { @@ -229,26 +229,28 @@ legacyKaleidoScope <- function(kaleido) { if (any(!grepl("^[A-Za-z_][A-Za-z0-9_]*$", context_names))) { rlang::abort("`context` names must be valid Python identifiers.") } - - for (i in seq_along(context)) { - name <- context_names[[i]] - had_value[[i]] <- reticulate::py_has_attr(py, name) - if (had_value[[i]]) { - old_values[[i]] <- py[[name]] - } - py[[name]] <- context[[i]] - } - + + py <- reticulate::py on.exit({ - for (i in rev(seq_along(context))) { + for (i in rev(which(was_set))) { name <- context_names[[i]] if (had_value[[i]]) { - py[[name]] <- old_values[[i]] + reticulate::py_set_attr(py, name, old_values[[i]]) } else { - reticulate::py_run_string(paste("del", name)) + reticulate::py_del_attr(py, name) } } }, add = TRUE) + + for (i in seq_along(context)) { + name <- context_names[[i]] + had_value[[i]] <- reticulate::py_has_attr(py, name) + if (had_value[[i]]) { + old_values[[i]] <- reticulate::py_get_attr(py, name) + } + reticulate::py_set_attr(py, name, context[[i]]) + was_set[[i]] <- TRUE + } } reticulate::py_run_string(code, convert = convert) diff --git a/tests/testthat/test-kaleido.R b/tests/testthat/test-kaleido.R index 79631b06e9..3c7c0eee62 100644 --- a/tests/testthat/test-kaleido.R +++ b/tests/testthat/test-kaleido.R @@ -1,6 +1,14 @@ test_that("newKaleidoScope does not inline Windows temp paths into Python code", { skip_if_not_installed("reticulate") skip_if_not(suppressWarnings(reticulate::py_available(TRUE))) + withr::defer({ + py <- reticulate::py + for (name in c("fig", "tmp_json_path")) { + if (reticulate::py_has_attr(py, name)) { + reticulate::py_del_attr(py, name) + } + } + }) win_path <- "C:\\users\\name\\AppData\\Local\\Temp\\Rtmp\\file.json" py_calls <- character() @@ -52,6 +60,8 @@ test_that("newKaleidoScope does not inline Windows temp paths into Python code", }) test_that("py_run_string_with_context rejects invalid Python identifiers", { + skip_if_not_installed("reticulate") + expect_error( plotly:::.py_run_string_with_context( "value = 1", @@ -60,3 +70,38 @@ test_that("py_run_string_with_context rejects invalid Python identifiers", { "`context` names must be valid Python identifiers\\." ) }) + +test_that("py_run_string_with_context cleans up partial assignments", { + skip_if_not_installed("reticulate") + skip_if_not(suppressWarnings(reticulate::py_available(TRUE))) + + deleted <- character() + + testthat::local_mocked_bindings( + py_has_attr = function(x, name) FALSE, + py_get_attr = function(x, name, silent = FALSE) stop("unexpected py_get_attr call"), + py_set_attr = function(x, name, value) { + if (identical(name, "second")) { + stop("boom") + } + invisible(NULL) + }, + py_del_attr = function(x, name) { + deleted <<- c(deleted, name) + invisible(NULL) + }, + py_run_string = function(code, local = FALSE, convert = TRUE) { + stop("unexpected py_run_string call") + }, + .package = "reticulate" + ) + + expect_error( + plotly:::.py_run_string_with_context( + "value = 1", + context = list(first = 1, second = 2) + ), + "boom" + ) + expect_identical(deleted, "first") +})