From 228624b1d26bd243ab340f8b4083565b424dcd6d Mon Sep 17 00:00:00 2001
From: Timothy Mastny <tim.mastny@gmail.com>
Date: Sun, 25 Feb 2018 20:09:16 -0600
Subject: [PATCH 1/5] added unit tests that cover knitr #1505

---
 .../resources/eng-reticulate-cache-test.Rmd   | 22 ++++++++++
 tests/testthat/test-python-cache-engine.R     | 44 +++++++++++++++++++
 tests/testthat/test-python-dill.R             | 37 ++++++++++++++++
 tests/testthat/test-python-globals.R          | 27 ++++++++++++
 4 files changed, 130 insertions(+)
 create mode 100644 tests/testthat/resources/eng-reticulate-cache-test.Rmd
 create mode 100644 tests/testthat/test-python-cache-engine.R
 create mode 100644 tests/testthat/test-python-dill.R
 create mode 100644 tests/testthat/test-python-globals.R

diff --git a/tests/testthat/resources/eng-reticulate-cache-test.Rmd b/tests/testthat/resources/eng-reticulate-cache-test.Rmd
new file mode 100644
index 000000000..a878f8b98
--- /dev/null
+++ b/tests/testthat/resources/eng-reticulate-cache-test.Rmd
@@ -0,0 +1,22 @@
+---
+title: "Using reticulate's Python Engine with knitr"
+---
+
+```{r setup, include = FALSE}
+library(reticulate)
+knitr::opts_chunk$set(cache=TRUE)
+knitr::knit_engines$set(python = eng_python)
+knitr::cache_engines$set(python = cache_eng_python)
+```
+
+Cache can handle changes to second chunk:
+
+```{python}
+x = 1
+```
+
+```{python}
+print(x + 1)
+```
+
+
diff --git a/tests/testthat/test-python-cache-engine.R b/tests/testthat/test-python-cache-engine.R
new file mode 100644
index 000000000..2c4caa6e0
--- /dev/null
+++ b/tests/testthat/test-python-cache-engine.R
@@ -0,0 +1,44 @@
+context("knitr-cache")
+
+test_that("An R Markdown document can be rendered with cache using reticulate", {
+  
+  skip_on_cran()
+  skip_if_not_installed("rmarkdown")
+  skip_if_not_installed("callr")
+  
+  path <- callr::r(
+    function() {
+      rmarkdown::render("resources/eng-reticulate-cache-test.Rmd", quiet = TRUE, envir = new.env())
+    })
+  expect_true(file.exists(path))
+  on.exit(unlink(path), add = TRUE)
+})
+
+test_that("An R Markdown document builds if a cache is modified", {
+  
+  skip_on_cran()
+  skip_if_not_installed("rmarkdown")
+  skip_if_not_installed("callr")
+  
+  old_var <- "1"
+  new_var <- "0"
+  mutate_chunk <- function(x) {
+    print_line <- 19
+    file_text <- readLines("resources/eng-reticulate-cache-test.Rmd")
+    file_text[print_line] <- paste("print(x + ", x, ")", sep = "")
+    writeLines(file_text, "resources/eng-reticulate-cache-test.Rmd")
+  }
+  mutate_chunk(old_var)
+  mutate_chunk(new_var)
+  path <- callr::r(
+    function() {
+      rmarkdown::render("resources/eng-reticulate-cache-test.Rmd", quiet = TRUE, envir = new.env())
+    })
+  mutate_chunk(old_var)
+  expect_true(file.exists(path))
+  on.exit(unlink(path), add = TRUE)
+  on.exit(unlink("resources/eng-reticulate-cache-test_cache/", recursive = TRUE), add = TRUE)
+})
+
+
+
diff --git a/tests/testthat/test-python-dill.R b/tests/testthat/test-python-dill.R
new file mode 100644
index 000000000..56efa9d99
--- /dev/null
+++ b/tests/testthat/test-python-dill.R
@@ -0,0 +1,37 @@
+context("dill")
+
+source("utils.R")
+
+test_that("Interpreter sessions can be saved and loaded with dill", {
+  skip_if_no_python()
+  skip_if_not_installed("callr")
+  
+  session_one_vars <- callr::r(
+    function() {
+      module_load <- tryCatch(
+        dill <- reticulate::import("dill"),
+        error = function(c) {
+          py_error <- reticulate::py_last_error()
+          if(py_error$type == "ImportError" && py_error$value == "No module named dill") {
+            "No dill"
+          }})
+      if (module_load == "No dill") return(module_load)
+      main <- reticulate::py_run_string("x = 1")
+      reticulate::py_run_string("y = x + 1")
+      dill$dump_session(filename = "x.dill", byref = TRUE)
+      c(main$x, main$y)
+    })
+  if (session_one_vars[1] == "No dill")
+    skip("The dill Python module is not installed")
+  
+  session_two_vars <- callr::r(
+    function() {
+      dill <- reticulate::import("dill")
+      dill$load_session(filename = "x.dill")
+      main <- reticulate::py_run_string("pass")
+      c(main$x, main$y)
+    })
+  on.exit(unlink("x.dill"), add = TRUE)
+  expect_equal(session_one_vars, session_two_vars)
+})
+
diff --git a/tests/testthat/test-python-globals.R b/tests/testthat/test-python-globals.R
new file mode 100644
index 000000000..d9beb36ab
--- /dev/null
+++ b/tests/testthat/test-python-globals.R
@@ -0,0 +1,27 @@
+context("globals")
+
+source("utils.R")
+
+test_that("Interpreter sessions can be saved and loaded with dill", {
+  skip_if_no_python()
+  
+  py_run_string("x = 1")
+  py_run_string("y = 1")
+  py_run_string("[globals().pop(i) for i in ['x', 'y']]")
+  
+  test_x <- tryCatch(
+    py_run_string("x = x + 1"),
+    error = function(e) {
+      py_last_error()$value
+    }
+  )
+  test_y <- tryCatch(
+    py_run_string("y = y + 1"),
+    error = function(e) {
+      py_last_error()$value
+    }
+  )
+  expect_equal(test_x, "name 'x' is not defined")
+  expect_equal(test_y, "name 'y' is not defined")
+})
+

From b52ecaa4f25c24ab9915c3d2e172daa08e3be381 Mon Sep 17 00:00:00 2001
From: Timothy Mastny <tim.mastny@gmail.com>
Date: Sun, 25 Feb 2018 20:10:17 -0600
Subject: [PATCH 2/5] added cache_eng_python to add Python session caching
 between chunks. Addresses knitr #1505

---
 NAMESPACE               |  1 +
 R/knitr-engine.R        | 67 +++++++++++++++++++++++++++++++++++++++--
 man/cache_eng_python.Rd | 24 +++++++++++++++
 3 files changed, 89 insertions(+), 3 deletions(-)
 create mode 100644 man/cache_eng_python.Rd

diff --git a/NAMESPACE b/NAMESPACE
index 512e13a52..01e133f27 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -66,6 +66,7 @@ S3method(summary,python.builtin.object)
 S3method(with,python.builtin.object)
 export("%as%")
 export(array_reshape)
+export(cache_eng_python)
 export(conda_binary)
 export(conda_create)
 export(conda_install)
diff --git a/R/knitr-engine.R b/R/knitr-engine.R
index a3e4b22e9..f4db73d31 100644
--- a/R/knitr-engine.R
+++ b/R/knitr-engine.R
@@ -175,7 +175,9 @@ eng_python <- function(options) {
   }
   
   eng_python_synchronize_after()
-  
+  if(options$cache > 0) {
+    save_python_session(options$hash)
+  }
   wrap <- getOption("reticulate.engine.wrap", eng_python_wrap)
   wrap(outputs, options)
   
@@ -186,8 +188,18 @@ eng_python_initialize <- function(options, context, envir) {
   if (is.character(options$engine.path))
     use_python(options$engine.path[[1]])
   
-  ensure_python_initialized()
-  
+  if (options$cache > 0) {
+    load_module <- tryCatch(
+      import("dill"),
+      error = function(c) {
+        py_error <- py_last_error()
+        if(py_error$type == "ImportError" && py_error$value == "No module named dill") {
+          warning("The Python module dill was not found. This module is needed for full cache functionality.")
+          "No dill"
+        }})
+    if (load_module != "No dill") 
+      py_run_string("import dill")
+  } 
   eng_python_initialize_matplotlib(options, context, envir)
 }
 
@@ -278,3 +290,52 @@ eng_python_wrap <- function(outputs, options) {
   wrap <- yoink("knitr", "wrap")
   wrap(outputs, options)
 }
+
+save_python_session <- function(cache_path) {
+  dill <- tryCatch(
+    import("dill"),
+    error = function(c) {
+      py_error <- py_last_error()
+      if(py_error$type == "ImportError" && py_error$value == "No module named dill") {
+        "No dill"
+      }})
+  if (dill == "No dill") return()
+
+  py_run_string("globals().pop('r')")
+  py_run_string("globals().pop('R')")
+  dill$dump_session(filename = paste0(cache_path, ".pkl"), byref = TRUE)
+}
+
+#' A reticulate cache engine for Knitr
+#' 
+#' This provides a `reticulate` cache engine for `knitr`. The cache engine
+#' allows `knitr` to save and load Python sessions between cached chunks. The
+#' cache engine depends on the `dill` Python module. Therefore, you must have
+#' `dill` installed in your Python environment.
+#' 
+#' The engine can be activated by setting (for example)
+#' 
+#' ```
+#' knitr::cache_engines$set(python = reticulate::cache_eng_python)
+#' ```
+#' 
+#' Typically, this will be set within a document's setup chunk, or by the
+#' environment requesting that Python chunks be processed by this engine.
+#' 
+#' @param cache_path
+#'   The path to save the chunk cache, as provided by `knitr` during chunk execution.
+#'   
+#' @export
+cache_eng_python <- function(cache_path) {
+  dill <- tryCatch(
+    import("dill"),
+    error = function(c) {
+      py_error <- py_last_error()
+      if(py_error$type == "ImportError" && py_error$value == "No module named dill") {
+        "No dill"
+      }})
+  if (dill == "No dill") return()
+  dill$load_session(filename = paste0(cache_path, ".pkl"))
+}
+
+
diff --git a/man/cache_eng_python.Rd b/man/cache_eng_python.Rd
new file mode 100644
index 000000000..d8fd64ef0
--- /dev/null
+++ b/man/cache_eng_python.Rd
@@ -0,0 +1,24 @@
+% Generated by roxygen2: do not edit by hand
+% Please edit documentation in R/knitr-engine.R
+\name{cache_eng_python}
+\alias{cache_eng_python}
+\title{A reticulate cache engine for Knitr}
+\usage{
+cache_eng_python(cache_path)
+}
+\arguments{
+\item{cache_path}{The path to save the chunk cache, as provided by \code{knitr} during chunk execution.}
+}
+\description{
+This provides a \code{reticulate} cache engine for \code{knitr}. The cache engine
+allows \code{knitr} to save and load Python sessions between cached chunks. The
+cache engine depends on the \code{dill} Python module. Therefore, you must have
+\code{dill} installed in your Python environment.
+}
+\details{
+The engine can be activated by setting (for example)\preformatted{knitr::cache_engines$set(python = reticulate::cache_eng_python)
+}
+
+Typically, this will be set within a document's setup chunk, or by the
+environment requesting that Python chunks be processed by this engine.
+}

From c345ce2fde6555f2f71138de18d219b9e78b59c1 Mon Sep 17 00:00:00 2001
From: Timothy Mastny <tim.mastny@gmail.com>
Date: Wed, 28 Feb 2018 17:14:11 -0600
Subject: [PATCH 3/5] dill caching engine for knitr, with tests

---
 R/knitr-engine.R                          | 60 +++++++++++------------
 R/python.R                                | 60 +++++++++++++++++++++++
 tests/testthat/test-python-cache-engine.R |  2 +
 3 files changed, 91 insertions(+), 31 deletions(-)

diff --git a/R/knitr-engine.R b/R/knitr-engine.R
index f4db73d31..c73d71554 100644
--- a/R/knitr-engine.R
+++ b/R/knitr-engine.R
@@ -189,16 +189,14 @@ eng_python_initialize <- function(options, context, envir) {
     use_python(options$engine.path[[1]])
   
   if (options$cache > 0) {
-    load_module <- tryCatch(
-      import("dill"),
-      error = function(c) {
-        py_error <- py_last_error()
-        if(py_error$type == "ImportError" && py_error$value == "No module named dill") {
-          warning("The Python module dill was not found. This module is needed for full cache functionality.")
-          "No dill"
-        }})
-    if (load_module != "No dill") 
-      py_run_string("import dill")
+    module <- tryCatch(import("dill"), error = identity)
+    if (inherits(module, "error")) {
+      if (module$message == "ImportError: No module named dill") {
+        warning("The Python module dill was not found. This module is needed for full cache functionality.")
+      } else {
+        stop(module$message)
+      }
+    }
   } 
   eng_python_initialize_matplotlib(options, context, envir)
 }
@@ -292,18 +290,20 @@ eng_python_wrap <- function(outputs, options) {
 }
 
 save_python_session <- function(cache_path) {
-  dill <- tryCatch(
-    import("dill"),
-    error = function(c) {
-      py_error <- py_last_error()
-      if(py_error$type == "ImportError" && py_error$value == "No module named dill") {
-        "No dill"
-      }})
-  if (dill == "No dill") return()
-
-  py_run_string("globals().pop('r')")
-  py_run_string("globals().pop('R')")
-  dill$dump_session(filename = paste0(cache_path, ".pkl"), byref = TRUE)
+  module <- tryCatch(import("dill"), error = identity)
+  if (inherits(module, "error")) {
+    if (module$message == "ImportError: No module named dill") return()
+    signalCondition(module$message)
+  }
+  
+  r_objs_exist <- "all(r_obj in globals() for r_obj in ('r', 'R'))"
+  r_is_R <- "isinstance(r, R)"
+  if (py_eval(r_objs_exist) && py_eval(r_is_R)) {
+    py_run_string("globals().pop('r')")
+    py_run_string("globals().pop('R')")
+  }
+  
+  module$dump_session(filename = paste0(cache_path, ".pkl"), byref = TRUE)
 }
 
 #' A reticulate cache engine for Knitr
@@ -327,15 +327,13 @@ save_python_session <- function(cache_path) {
 #'   
 #' @export
 cache_eng_python <- function(cache_path) {
-  dill <- tryCatch(
-    import("dill"),
-    error = function(c) {
-      py_error <- py_last_error()
-      if(py_error$type == "ImportError" && py_error$value == "No module named dill") {
-        "No dill"
-      }})
-  if (dill == "No dill") return()
-  dill$load_session(filename = paste0(cache_path, ".pkl"))
+  module <- tryCatch(import("dill"), error = identity)
+  if (inherits(module, "error")) {
+    if (module$message == "ImportError: No module named dill") return()
+    stop(module$message)
+  }
+
+  module$load_session(filename = paste0(cache_path, ".pkl"))
 }
 
 
diff --git a/R/python.R b/R/python.R
index d6bfeacf7..2440e1a5f 100644
--- a/R/python.R
+++ b/R/python.R
@@ -198,6 +198,66 @@ summary.python.builtin.object <- function(object, ...) {
   str(object)
 }
 
+
+#' Convert between Python and R objects
+#' 
+#' @inheritParams import
+#' @param x Object to convert
+#' 
+#' @return Converted object
+#' 
+#' @name r-py-conversion
+#' @export
+py_to_r <- function(x) {
+  
+  ensure_python_initialized()
+  
+  if (!inherits(x, "python.builtin.object"))
+    stop("Object to convert is not a Python object")
+  
+  # get the default wrapper
+  x <- py_ref_to_r(x)
+  
+  # allow customization of the wrapper
+  wrapper <- py_to_r_wrapper(x)
+  attributes(wrapper) <- attributes(x)
+  
+  # return the wrapper
+  wrapper 
+}
+
+#' R wrapper for Python objects 
+#' 
+#' S3 method to create a custom R wrapper for a Python object.
+#' The default wrapper is either an R environment or an R function
+#' (for callable python objects).
+#' 
+#' @param x Python object 
+#' 
+#' @export
+py_to_r_wrapper <- function(x) {
+  UseMethod("py_to_r_wrapper")
+}
+
+#' @export
+py_to_r_wrapper.default <- function(x) {
+  x
+}
+
+
+
+
+
+#' @rdname r-py-conversion
+#' @export
+r_to_py <- function(x, convert = FALSE) {
+  
+  ensure_python_initialized()
+  
+  r_to_py_impl(x, convert = convert)
+}
+
+
 #' @export
 `$.python.builtin.module` <- function(x, name) {
  
diff --git a/tests/testthat/test-python-cache-engine.R b/tests/testthat/test-python-cache-engine.R
index 2c4caa6e0..940d7016d 100644
--- a/tests/testthat/test-python-cache-engine.R
+++ b/tests/testthat/test-python-cache-engine.R
@@ -6,6 +6,8 @@ test_that("An R Markdown document can be rendered with cache using reticulate",
   skip_if_not_installed("rmarkdown")
   skip_if_not_installed("callr")
   
+  unlink("resources/eng-reticulate-cache-test_cache/", recursive = TRUE)
+  
   path <- callr::r(
     function() {
       rmarkdown::render("resources/eng-reticulate-cache-test.Rmd", quiet = TRUE, envir = new.env())

From ff39889bb09eae41f9c07aa1a6d12c6ac8ebc14f Mon Sep 17 00:00:00 2001
From: Timothy Mastny <tim.mastny@gmail.com>
Date: Wed, 18 Apr 2018 17:19:02 -0500
Subject: [PATCH 4/5] changes from feedback on knitr #1518 with updated tests

---
 R/knitr-engine.R                                       | 9 +++++----
 man/cache_eng_python.Rd                                | 5 +++--
 tests/testthat/resources/eng-reticulate-cache-test.Rmd | 2 --
 tests/testthat/test-python-cache-engine.R              | 4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/R/knitr-engine.R b/R/knitr-engine.R
index c73d71554..50cb5e1dc 100644
--- a/R/knitr-engine.R
+++ b/R/knitr-engine.R
@@ -322,18 +322,19 @@ save_python_session <- function(cache_path) {
 #' Typically, this will be set within a document's setup chunk, or by the
 #' environment requesting that Python chunks be processed by this engine.
 #' 
-#' @param cache_path
-#'   The path to save the chunk cache, as provided by `knitr` during chunk execution.
+#' @param options
+#'   List of chunk options provided by `knitr` during chunk execution. 
+#'   Contains the caching path.
 #'   
 #' @export
-cache_eng_python <- function(cache_path) {
+cache_eng_python <- function(options) {
   module <- tryCatch(import("dill"), error = identity)
   if (inherits(module, "error")) {
     if (module$message == "ImportError: No module named dill") return()
     stop(module$message)
   }
 
-  module$load_session(filename = paste0(cache_path, ".pkl"))
+  module$load_session(filename = paste0(options$hash, ".pkl"))
 }
 
 
diff --git a/man/cache_eng_python.Rd b/man/cache_eng_python.Rd
index d8fd64ef0..ff8067b7b 100644
--- a/man/cache_eng_python.Rd
+++ b/man/cache_eng_python.Rd
@@ -4,10 +4,11 @@
 \alias{cache_eng_python}
 \title{A reticulate cache engine for Knitr}
 \usage{
-cache_eng_python(cache_path)
+cache_eng_python(options)
 }
 \arguments{
-\item{cache_path}{The path to save the chunk cache, as provided by \code{knitr} during chunk execution.}
+\item{options}{List of chunk options provided by \code{knitr} during chunk execution.
+Contains the caching path.}
 }
 \description{
 This provides a \code{reticulate} cache engine for \code{knitr}. The cache engine
diff --git a/tests/testthat/resources/eng-reticulate-cache-test.Rmd b/tests/testthat/resources/eng-reticulate-cache-test.Rmd
index a878f8b98..2017d584a 100644
--- a/tests/testthat/resources/eng-reticulate-cache-test.Rmd
+++ b/tests/testthat/resources/eng-reticulate-cache-test.Rmd
@@ -5,8 +5,6 @@ title: "Using reticulate's Python Engine with knitr"
 ```{r setup, include = FALSE}
 library(reticulate)
 knitr::opts_chunk$set(cache=TRUE)
-knitr::knit_engines$set(python = eng_python)
-knitr::cache_engines$set(python = cache_eng_python)
 ```
 
 Cache can handle changes to second chunk:
diff --git a/tests/testthat/test-python-cache-engine.R b/tests/testthat/test-python-cache-engine.R
index 940d7016d..6d8f17422 100644
--- a/tests/testthat/test-python-cache-engine.R
+++ b/tests/testthat/test-python-cache-engine.R
@@ -25,9 +25,9 @@ test_that("An R Markdown document builds if a cache is modified", {
   old_var <- "1"
   new_var <- "0"
   mutate_chunk <- function(x) {
-    print_line <- 19
+    print_line <- 17
     file_text <- readLines("resources/eng-reticulate-cache-test.Rmd")
-    file_text[print_line] <- paste("print(x + ", x, ")", sep = "")
+    file_text[print_line] <- paste0("print(x + ", x, ")")
     writeLines(file_text, "resources/eng-reticulate-cache-test.Rmd")
   }
   mutate_chunk(old_var)

From 8e07779631d2089310e46cecd38cf5a1c66a79c4 Mon Sep 17 00:00:00 2001
From: Timothy Mastny <tim.mastny@gmail.com>
Date: Wed, 18 Apr 2018 19:18:23 -0500
Subject: [PATCH 5/5] fixed testing utils source in dill tests

---
 tests/testthat/test-python-dill.R | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/testthat/test-python-dill.R b/tests/testthat/test-python-dill.R
index 56efa9d99..e14b9c04d 100644
--- a/tests/testthat/test-python-dill.R
+++ b/tests/testthat/test-python-dill.R
@@ -1,6 +1,6 @@
 context("dill")
 
-source("utils.R")
+source("helper-utils.R")
 
 test_that("Interpreter sessions can be saved and loaded with dill", {
   skip_if_no_python()