FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix Windows path handling in save_image() (#2487) · plotly/plotly.R@b8f8d96 · GitHub

/ plotly.R Public

Commit b8f8d96

Browse files
cpsievert
authored
Fix Windows path handling in save_image() (#2487)
* test: capture Windows path regression in kaleido * fix: avoid embedding Windows paths in kaleido Python code * docs: note Windows save_image fix in news * test: tighten kaleido review follow-ups * fix: harden kaleido context cleanup
1 parent db95519 commit b8f8d96

3 files changed

Lines changed: 157 additions & 6 deletions

File tree

‎NEWS.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
* Removed the dependency on the `{lazyeval}` package.
44

5+
## Bug fixes
6+
7+
* 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.
8+
59

610
# plotly 4.12.0
711

‎R/kaleido.R‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -133,11 +133,10 @@ newKaleidoScope <- function(kaleido) {
133133
writeLines(fig, tmp_json)
134134

135135
# Import it as a fig (dict)
136-
load_json <- sprintf(
137-
"import json; fig = json.load(open('%s'))",
138-
tmp_json
136+
.py_run_string_with_context(
137+
"import json; fig = json.load(open(tmp_json_path))",
138+
context = list(tmp_json_path = tmp_json)
139139
)
140-
reticulate::py_run_string(load_json)
141140

142141
# Gather figure-level options
143142
opts <- list(
@@ -193,8 +192,9 @@ legacyKaleidoScope <- function(kaleido) {
193192
)
194193
# Write the base64 encoded string that transform() returns to disk
195194
# https://github.com/plotly/Kaleido/blame/master/README.md#L52
196-
reticulate::py_run_string(
197-
sprintf("import sys; open('%s', 'wb').write(%s)", file, transform_cmd)
195+
.py_run_string_with_context(
196+
sprintf("import sys; open(output_file, 'wb').write(%s)", transform_cmd),
197+
context = list(output_file = file)
198198
)
199199

200200
invisible(file)
@@ -216,6 +216,46 @@ legacyKaleidoScope <- function(kaleido) {
216216
res
217217
}
218218

219+
.py_run_string_with_context <- function(code, context = list(), convert = TRUE) {
220+
context_names <- names(context)
221+
old_values <- vector("list", length(context))
222+
had_value <- logical(length(context))
223+
was_set <- logical(length(context))
224+
225+
if (length(context) > 0) {
226+
if (is.null(context_names) || any(context_names == "")) {
227+
rlang::abort("`context` must be a named list.")
228+
}
229+
if (any(!grepl("^[A-Za-z_][A-Za-z0-9_]*$", context_names))) {
230+
rlang::abort("`context` names must be valid Python identifiers.")
231+
}
232+
233+
py <- reticulate::py
234+
on.exit({
235+
for (i in rev(which(was_set))) {
236+
name <- context_names[[i]]
237+
if (had_value[[i]]) {
238+
reticulate::py_set_attr(py, name, old_values[[i]])
239+
} else {
240+
reticulate::py_del_attr(py, name)
241+
}
242+
}
243+
}, add = TRUE)
244+
245+
for (i in seq_along(context)) {
246+
name <- context_names[[i]]
247+
had_value[[i]] <- reticulate::py_has_attr(py, name)
248+
if (had_value[[i]]) {
249+
old_values[[i]] <- reticulate::py_get_attr(py, name)
250+
}
251+
reticulate::py_set_attr(py, name, context[[i]])
252+
was_set[[i]] <- TRUE
253+
}
254+
}
255+
256+
reticulate::py_run_string(code, convert = convert)
257+
}
258+
219259

220260
#' Print method for kaleido
221261
#'

‎tests/testthat/test-kaleido.R‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
test_that("newKaleidoScope does not inline Windows temp paths into Python code", {
2+
skip_if_not_installed("reticulate")
3+
skip_if_not(suppressWarnings(reticulate::py_available(TRUE)))
4+
withr::defer({
5+
py <- reticulate::py
6+
for (name in c("fig", "tmp_json_path")) {
7+
if (reticulate::py_has_attr(py, name)) {
8+
reticulate::py_del_attr(py, name)
9+
}
10+
}
11+
})
12+
13+
win_path <- "C:\\users\\name\\AppData\\Local\\Temp\\Rtmp\\file.json"
14+
py_calls <- character()
15+
write_fig_args <- NULL
16+
17+
local({
18+
testthat::local_mocked_bindings(
19+
plotly_build = function(...) {
20+
list(x = list(data = list(), layout = list(), config = list()))
21+
},
22+
to_JSON = function(...) "{}",
23+
plotlyMainBundlePath = function() "plotly.min.js",
24+
.package = "plotly"
25+
)
26+
27+
testthat::local_mocked_bindings(
28+
tempfile = function(...) win_path,
29+
writeLines = function(...) NULL,
30+
unlink = function(...) 0,
31+
.package = "base"
32+
)
33+
34+
testthat::local_mocked_bindings(
35+
py_run_string = function(code, ...) {
36+
py_calls <<- c(py_calls, code)
37+
py_obj <- get("py", envir = asNamespace("reticulate"))
38+
py_obj$fig <- "fake-fig"
39+
invisible(NULL)
40+
},
41+
.package = "reticulate"
42+
)
43+
44+
kaleido <- list(write_fig_sync = function(fig, file, opts, kopts) {
45+
write_fig_args <<- list(fig = fig, file = file, opts = opts, kopts = kopts)
46+
invisible(NULL)
47+
})
48+
49+
scope <- plotly:::newKaleidoScope(kaleido)
50+
scope$transform(list(), "figure.png")
51+
})
52+
53+
expect_identical(py_calls[[1]], "import json; fig = json.load(open(tmp_json_path))")
54+
expect_true(!grepl(win_path, py_calls[[1]], fixed = TRUE))
55+
if (length(py_calls) >= 2) {
56+
expect_identical(py_calls[[2]], "del tmp_json_path")
57+
}
58+
expect_identical(write_fig_args$fig, "fake-fig")
59+
expect_identical(write_fig_args$file, "figure.png")
60+
})
61+
62+
test_that("py_run_string_with_context rejects invalid Python identifiers", {
63+
skip_if_not_installed("reticulate")
64+
65+
expect_error(
66+
plotly:::.py_run_string_with_context(
67+
"value = 1",
68+
context = list("bad name" = 1)
69+
),
70+
"`context` names must be valid Python identifiers\\."
71+
)
72+
})
73+
74+
test_that("py_run_string_with_context cleans up partial assignments", {
75+
skip_if_not_installed("reticulate")
76+
skip_if_not(suppressWarnings(reticulate::py_available(TRUE)))
77+
78+
deleted <- character()
79+
80+
testthat::local_mocked_bindings(
81+
py_has_attr = function(x, name) FALSE,
82+
py_get_attr = function(x, name, silent = FALSE) stop("unexpected py_get_attr call"),
83+
py_set_attr = function(x, name, value) {
84+
if (identical(name, "second")) {
85+
stop("boom")
86+
}
87+
invisible(NULL)
88+
},
89+
py_del_attr = function(x, name) {
90+
deleted <<- c(deleted, name)
91+
invisible(NULL)
92+
},
93+
py_run_string = function(code, local = FALSE, convert = TRUE) {
94+
stop("unexpected py_run_string call")
95+
},
96+
.package = "reticulate"
97+
)
98+
99+
expect_error(
100+
plotly:::.py_run_string_with_context(
101+
"value = 1",
102+
context = list(first = 1, second = 2)
103+
),
104+
"boom"
105+
)
106+
expect_identical(deleted, "first")
107+
})

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL