Fix: py_copy_global_env() now deep copies Python environment (#729)

Co-authored-by: Garrick Aden-Buie <[email protected]>

.gitignore

@@ -4,6 +4,7 @@
 .Ruserdata
 .DS_Store
 .vscode
+__pycache__
 Untitled
 issues
 node_modules

R/exercise.R

@@ -1401,8 +1401,8 @@ render_exercise_post_stage_hook.default <- function(exercise, ...) {
 
 #' @export
 render_exercise_post_stage_hook.python <- function(exercise, stage, envir, ...) {
-  # Add copy of python environment into the prep/restult environment
-  assign(".__py__", duplicate_py_env(py_global_env()), envir = envir)
+  # Add copy of python environment into the prep/result environment
+  assign(".__py__", py_copy_global_env(), envir = envir)
   invisible()
 }
 

R/utils.R

@@ -58,40 +58,59 @@ py_global_env <- function() {
   reticulate::py
 }
 
-# Create a duplicate of a Python environment
-#
-# @examples
-# reticulate::py_run_string("x = 3")
-# new_py_envir <- duplicate_py_env(py)
-# new_py_envir$items()
-#
-# @param module the `py` module accessed via `reticulate`
-#
-# @return a Python `Dict` or dictionary that is not converted to an R data type
-# @keywords internal
-duplicate_py_env <- function(module) {
+py_global_dict <- function() {
+  # extract all objects of `reticulate::py` (the main module)
+  reticulate::py_get_attr(py_global_env(), "__dict__")
+}
+
+#' Create a duplicate of a Python environment
+#'
+#' @return a Python `Dict` or dictionary
+#'
+#' @keywords internal
+#'
+#' @examples
+#' \dontrun{
+#' reticulate::py_run_string("x = 3")
+#' new_py_envir <- py_copy_global_env()
+#' new_py_envir$items()
+#' }
+py_copy_global_env <- function() {
   rlang::check_installed("reticulate", "Python exercise support")
 
-  # extract all objects within this module
-  new_objs <- reticulate::py_get_attr(module, "__dict__")
-  # then create copy of the dictionary with all objects
-  copy <- reticulate::import("copy", convert = FALSE)
-  copy$copy(new_objs)
-}
-
-# This clears the Python environment `py`.
-#
-# It will keep important initial objects such as `py` (main module), 
-# `r` (reticulate interface to R), and the `builtins` module.
-#
-# @examples
-# reticulate::py_run_string("x = 3")
-# # this removes the `x`
-# clear_py_env()
-#
-# @return Nothing
-# @keywords internal
-clear_py_env <- function() {
+  py_utils <- py_learnr_utilities()
+
+  # Calling `py_utils$deep_copy` results in a hybrid R-Python object, but
+  # invoking via `py_call` returns a Python object without R conversion
+  reticulate::py_call(py_utils$deep_copy, py_global_dict())
+}
+
+py_learnr_utilities <- function() {
+  py_env_dict <- py_global_dict()
+  utilities <- py_env_dict[["__learnr__"]]
+  if (!is.null(utilities)) {
+    return(utilities)
+  }
+
+  learnr_py <- system.file("internals", "learnr.py", package = "learnr")
+  reticulate::py_run_file(learnr_py,convert = FALSE)[["__learnr__"]]
+}
+
+#' This clears the Python environment `py`.
+#'
+#' It will keep important initial objects such as `py` (main module),
+#' `r` (reticulate interface to R), and the `builtins` module.
+#'
+#' @keywords internal
+#' @return Nothing
+#'
+#' @examples
+#' \dontrun{
+#' reticulate::py_run_string("x = 3")
+#' # this removes the `x`
+#' py_clear_env()
+#' }
+py_clear_env <- function() {
   Map(names(py_global_env()), f = function(obj_name) {
     # prevent the "base" python objects from being removed
     if (!obj_name %in% c("r", "sys", "builtins")) {
@@ -101,6 +120,10 @@ clear_py_env <- function() {
   return(invisible())
 }
 
+local_py_env <- function(envir = parent.frame()) {
+  withr::defer(py_clear_env(), envir = envir)
+}
+
 # backport errorCondition for R < 3.6.0
 if (getRversion() < package_version("3.6.0")) {
   errorCondition <- function(msg, ..., class = NULL, call = NULL) {

inst/internals/learnr.py

@@ -0,0 +1,15 @@
+
+class __learnr__:
+  '''An internal class to provide Python utility functions'''
+
+  @staticmethod
+  def deep_copy(dict, deep=True):
+    import copy
+    from types import ModuleType
+    new_dict = {}
+    for k, v in dict.items():
+      if (k == "r" or isinstance(v, ModuleType)):
+        new_dict[k] = v
+      else:
+        new_dict[k] = copy.deepcopy(v) if deep else v
+    return new_dict

tests/testthat/helpers.R

@@ -32,6 +32,10 @@ skip_on_ci_if_not_pr <- function() {
   skip("Skipping on CI, tests run in PR checks only")
 }
 
+skip_if_not_py_available <- function() {
+  skip_if_not(reticulate::py_available(initialize = TRUE), "Python not available on this system")
+}
+
 expect_marked_as <- function(object, correct, messages = NULL) {
   if (is.null(messages)) {
     expect_equal(object, mark_as(correct))

tests/testthat/test-exercise.R

@@ -1361,8 +1361,8 @@ test_that("SQL exercises - with explicit `output.var`", {
 
 test_that("Python exercises - simple example", {
   skip_if_not_installed("reticulate")
-  skip_if_not(reticulate::py_available(), "Python not available on this system")
-  withr::defer(clear_py_env())
+  skip_if_not_py_available()
+  local_py_env()
 
   ex_py <- mock_exercise(
     user_code = "3 + 3",
@@ -1372,17 +1372,24 @@ test_that("Python exercises - simple example", {
 
   res <- withr::with_tempdir(render_exercise(ex_py, new.env()))
 
-  expect_equal(res$last_value, 6)
+  expect_equal(reticulate::py_to_r(res$last_value), 6)
   expect_null(res$evaluate_result)
   expect_match(as.character(res$html_output), "<code>6</code>")
   expect_true(exists('.__py__', res$envir_prep))
   expect_true(exists('.__py__', res$envir_result))
+
+  # envir_prep and envir_result should be different objects
+  envir_prep_py <- get0(".__py__", envir = res$envir_prep, ifnotfound = NULL)
+  envir_result_py <- get0(".__py__", envir = res$envir_result, ifnotfound = NULL)
+  expect_false(
+    identical(reticulate::py_id(envir_prep_py), reticulate::py_id(envir_result_py))
+  )
 })
 
 test_that("Python exercises - assignment example", {
   skip_if_not_installed("reticulate")
-  skip_if_not(reticulate::py_available(), "Python not available on this system")
-  withr::defer(clear_py_env())
+  skip_if_not_py_available()
+  local_py_env()
 
   ex_py <- mock_exercise(
     user_code = "x = 3 + 3",
@@ -1393,13 +1400,18 @@ test_that("Python exercises - assignment example", {
   res <- withr::with_tempdir(render_exercise(ex_py, new.env()))
 
   # TODO: invisible values should be more explicit
-  expect_equal(res$last_value, "__reticulate_placeholder__")
+  expect_equal(reticulate::py_to_r(res$last_value), "__reticulate_placeholder__")
   expect_null(res$evaluate_result)
   expect_true(exists('.__py__', res$envir_prep))
   expect_true(exists('.__py__', res$envir_result))
   result <- get0(".__py__", envir = res$envir_result, ifnotfound = NULL)
-  result <- reticulate::py_to_r(result)
   expect_equal(result$x, 6)
+
+  envir_prep_py <- get0(".__py__", envir = res$envir_prep, ifnotfound = NULL)
+  envir_result_py <- get0(".__py__", envir = res$envir_result, ifnotfound = NULL)
+  expect_false(
+    identical(reticulate::py_id(envir_prep_py), reticulate::py_id(envir_result_py))
+  )
 })
 
 # render_exercise_prepare() ------------------------------------------------------