From f652ea1b0a6972b62d5be125a7a73fdd202710ab Mon Sep 17 00:00:00 2001
From: oath_nngithub <oath@novonordisk.com>
Date: Mon, 11 Nov 2024 16:11:14 +0100
Subject: [PATCH] feat: integrate python logging directly in functions

---
 R/session.R                            |  54 +++++++++---
 R/status.R                             |  33 +++++--
 inst/documents/dummy.qmd               |  53 ++++++++++--
 inst/documents/log.qmd                 | 114 +++++++++++++++----------
 inst/documents/python_modules.py       |  21 +++++
 inst/examples/simple/python_error.py   |   6 ++
 inst/examples/simple/python_success.py |   9 ++
 inst/examples/simple/python_warning.py |   3 +
 tests/testthat/test-internal_run.R     |   7 +-
 9 files changed, 228 insertions(+), 72 deletions(-)
 create mode 100644 inst/documents/python_modules.py
 create mode 100644 inst/examples/simple/python_error.py
 create mode 100644 inst/examples/simple/python_success.py
 create mode 100644 inst/examples/simple/python_warning.py

diff --git a/R/session.R b/R/session.R
index 920264a..1b3222f 100644
--- a/R/session.R
+++ b/R/session.R
@@ -1,10 +1,11 @@
 #' Get session info
 #'
 #' Retrieve session info and add quarto info if not already there
+#' Argument to also add python version and package info
 #'
 #' @noRd
 
-session_info <- function(approved_folder_pkgs, approved_url_pkgs) {
+session_info <- function(approved_folder_pkgs = NULL, approved_url_pkgs = NULL, python_packages = NULL) {
   info <- sessioninfo::session_info()
 
   if (!is.null(approved_folder_pkgs) |
@@ -30,7 +31,7 @@ session_info <- function(approved_folder_pkgs, approved_url_pkgs) {
   info$options <- info$options[!names(info$options) %in% "rl_word_breaks"]
   class(info$options) <- c("options_info", class(info$options))
 
-  # TODO: Extend to also cover external and python below in methods.
+  # TODO: Extend to also cover external.
   info[!names(info) %in% c("platform", "packages", "environment", "options")] <- NULL
 
   if (is.null(info$platform$quarto)) {
@@ -44,6 +45,19 @@ session_info <- function(approved_folder_pkgs, approved_url_pkgs) {
     }
   }
 
+  if (!is.null(python_packages)) {
+
+    # TODO: Get the same information as for R packages (not only name and version)
+    # TODO: Only show used, and not all installed, packages if possible
+
+    info$python_packages <- python_packages
+    class(info$python_packages) <- c("packages_info", class(info$python_packages))
+
+    quarto_python_path <- Sys.getenv("QUARTO_PYTHON")
+    quarto_python_version <- gsub(".*/([0-9]+\\.[0-9]+\\.[0-9]+)/.*", "\\1", quarto_python_path)
+    info$platform$python <- quarto_python_version
+  }
+
   class(info) <- c("whirl_session_info", class(info))
   for (i in seq_along(info)) {
     class(info[[i]]) <- c(paste0("whirl_", class(info[[i]])[[1]]), class(info[[i]]))
@@ -52,6 +66,21 @@ session_info <- function(approved_folder_pkgs, approved_url_pkgs) {
   return(info)
 }
 
+#' Get Python package info from json file
+#'
+#' @noRd
+
+python_package_info <- function(json) {
+
+  json |>
+    jsonlite::fromJSON() |>
+    tibble::enframe(name = "Package") |>
+    tidyr::unnest_wider(col = value) |>
+    dplyr::rename(
+      Version = version,
+      Path = installation_path
+    )
+}
 
 #' @noRd
 
@@ -81,13 +110,18 @@ knit_print.whirl_platform_info <- function(x, ...) {
 #' @noRd
 
 knit_print.whirl_packages_info <- function(x, ...) {
-  data.frame(
-    Package = x$package,
-    Version = x$loadedversion,
-    `Date (UTC)` = x$date,
-    Source = x$source,
-    check.names = FALSE
-  ) |>
+
+  if (!is.null(x$package)){
+    x <- data.frame(
+      Package = x$package,
+      Version = x$loadedversion,
+      `Date (UTC)` = x$date,
+      Source = x$source,
+      check.names = FALSE
+    )
+  }
+
+  x |>
     knitr::kable() |>
     kableExtra::kable_styling(
       bootstrap_options = "striped",
@@ -97,7 +131,7 @@ knit_print.whirl_packages_info <- function(x, ...) {
 }
 
 #' @noRd
-#'
+
 knit_print.whirl_approved_pkgs <- function(x, ...) {
   hold <- x |>
     data.frame(
diff --git a/R/status.R b/R/status.R
index 5c76427..2cd5aa6 100644
--- a/R/status.R
+++ b/R/status.R
@@ -11,20 +11,27 @@ get_status <- function(md) {
 
   x <- x[[1]]
 
+  add_python <- x |>
+    stringr::str_detect("\\{.python .cell-code") |>
+    any()
+
   # Errors
 
   errors <- x |>
     stringr::str_subset(pattern = "^ *\\{\\.cell-output \\.cell-output-error\\}") |>
     stringr::str_remove_all("\\{[^\\}]*\\}") |>
     stringr::str_squish()
-  
-  python_errors <- x |>
-    stringr::str_subset(pattern = "(?i).*Error.*") |>
-    stringr::str_remove_all("\\{[^\\}]*\\}") |>
-    stringr::str_squish()
-  
-  errors <- c(errors, python_errors)
-  
+
+  if (add_python) {
+    python_errors <- x |>
+      stringr::str_subset(pattern = "^ *\\{\\.cell-output") |>
+      stringr::str_remove_all("\\{[^\\}]*\\}") |>
+      stringr::str_squish() |>
+      stringr::str_subset("Error:")
+
+    errors <- c(errors, python_errors)
+  }
+
   # Warnings
 
   warnings <- x |>
@@ -33,6 +40,16 @@ get_status <- function(md) {
     stringr::str_squish() |>
     stringr::str_subset(pattern = "^(W|w)arning")
 
+  if (add_python) {
+    python_warnings <- x |>
+      stringr::str_subset(pattern = "^ *\\{\\.cell-output") |>
+      stringr::str_remove_all("\\{[^\\}]*\\}") |>
+      stringr::str_squish() |>
+      stringr::str_subset("Warning:")
+
+    warnings <- c(warnings, python_warnings)
+  }
+
   # Status
 
   if (length(errors)) {
diff --git a/inst/documents/dummy.qmd b/inst/documents/dummy.qmd
index 855385e..e393982 100644
--- a/inst/documents/dummy.qmd
+++ b/inst/documents/dummy.qmd
@@ -8,7 +8,10 @@ params:
   tmpdir: "."
 ---
 
-```{r setup, include=FALSE}
+```{r setup}
+#| label: Setup
+#| include: false
+
 knitr::opts_chunk$set(
   error = TRUE,
   warning = TRUE,
@@ -19,22 +22,56 @@ knitr::opts_chunk$set(
 .libPaths(c(character(), params$with_library_paths))
 ```
 
-```{r, echo=FALSE, eval=(grepl("\\.R$", params$script))}
+```{r}
+#| label: Log R
+#| eval: !expr grepl("\\.R$", params$script)
+#| echo: false
 knitr::spin_child(params$script)
 ```
 
-```{r, child = params$script, eval=(grepl("\\.qmd$", params$script))}
+```{r}
+#| label: Log Quarto
+#| child: !expr params$script
+#| eval: !expr grepl("\\.qmd$", params$script)
+#| include: !expr grepl("\\.qmd$", params$script)
+```
+
+```{r}
+#| label: Log Rmarkdown
+#| child: !expr params$script
+#| eval: !expr grepl("\\.Rmd$", params$script)
+#| include: !expr grepl("\\.Rmd$", params$script)
 ```
 
-```{r, child = params$script, eval=(grepl("\\.Rmd$", params$script))}
+```{python}
+#| label: Log Python
+#| file: !expr params$script
+#| eval: !expr grepl("\\.py$", params$script)
+#| include: !expr grepl("\\.py$", params$script)
 ```
 
-```{python, file=params$script, eval=(grepl("\\.py$", params$script))}
+```{python}
+#| label: Python modules
+#| file: !expr file.path(params$tmpdir, "python_modules.py")
+#| eval: !expr grepl("\\.py$", params$script)
+#| echo: false
 ```
 
-```{r sessioninfo, include=FALSE}
+```{r}
+#| label: Session info
+#| include: false
+
+python_packages <- NULL
+if (file.exists(file.path(params$tmpdir, "python_imports.json"))){
+  python_packages <- whirl:::python_package_info(file.path(params$tmpdir, "python_imports.json"))
+}
+
 saveRDS(
-  object = whirl:::session_info(params$check_approved_folder_pkgs, params$check_approved_url_pkgs),
+  object = whirl:::session_info(
+    approved_folder_pkgs = params$check_approved_folder_pkgs, 
+    approved_url_pkgs = params$check_approved_url_pkgs, 
+    python_packages = python_packages
+    ),
   file = file.path(params$tmpdir, "session_info.rds")
 )
 
@@ -44,6 +81,4 @@ if (is.character(params$renv) && params$renv == "yes" | is.logical(params$renv)
     file = file.path(params$tmpdir, "renv_status.rds")
   )
 }
-
 ```
-
diff --git a/inst/documents/log.qmd b/inst/documents/log.qmd
index a6f5242..f3dc745 100644
--- a/inst/documents/log.qmd
+++ b/inst/documents/log.qmd
@@ -19,8 +19,16 @@ format:
 
 # Summary
 
-```{r, echo=FALSE, eval=TRUE}
+```{r}
+#| label: Setup
+#| echo: false
 .libPaths(c(character(), params$with_library_paths))
+```
+
+
+```{r}
+#| label: Status
+#| echo: false
 
 status <- params$tmpdir |>
   file.path("doc.md") |> 
@@ -43,15 +51,19 @@ whirl:::quarto_callout(
 )
 ```
 
-```{r, echo=FALSE}
-if (file.exists(file.path(params$tmpdir, "renv_status.rds"))) {
-  params$tmpdir |> 
-    file.path("renv_status.rds") |> 
-    readRDS()
-}
+```{r}
+#| label: Renv
+#| echo: false
+#| eval: !expr file.exists(file.path(params$tmpdir, "renv_status.rds"))
+
+params$tmpdir |> 
+  file.path("renv_status.rds") |> 
+  readRDS()
 ```
 
-```{r, echo=FALSE}
+```{r}
+#| label: Track files
+#| echo: false
 log_info <- whirl:::read_from_log()
 use_log_info <- nrow(log_info) > 0
 
@@ -62,38 +74,57 @@ if (use_log_info) {
 }
 ```
 
-```{r, echo=FALSE, results='asis', eval=use_log_info}
+```{r}
+#| echo: false
+#| results: asis
+#| eval: !expr use_log_info
 cat("## Input", "\n")
 ```
 
-```{r, echo=FALSE, eval=use_log_info}
+```{r}
+#| echo: false
+#| eval: !expr use_log_info
 log_info$read
 ```
 
-```{r, echo=FALSE, results='asis', eval=use_log_info}
+```{r}
+#| echo: false
+#| results: asis
+#| eval: !expr use_log_info
 cat("## Output", "\n")
 ```
 
-```{r, echo=FALSE, eval=use_log_info}
+```{r}
+#| echo: false
+#| eval: !expr use_log_info
 log_info$write
 ```
 
-```{r, echo=FALSE, results='asis', eval=use_log_info}
+```{r}
+#| echo: false
+#| results: asis
+#| eval: !expr use_log_info
 cat("## Removed", "\n")
 ```
 
-```{r, echo=FALSE, eval=use_log_info}
+```{r}
+#| echo: false
+#| eval: !expr use_log_info
 log_info$delete
 ```
 
 # Script
 
-```{r, child = file.path(params$tmpdir, "doc.md")}
+```{r}
+#| child: !expr file.path(params$tmpdir, "doc.md")
 ```
 
 # Session info
 
-```{r, include=FALSE}
+```{r}
+#| label: Session info
+#| include: false
+
 knitr::opts_chunk$set(
   error = FALSE,
   warning = FALSE,
@@ -106,38 +137,28 @@ info <- params$tmpdir |>
     readRDS()
 ```
 
-```{r, results="asis", eval=(grepl("\\.py$", params$title))}
-
-quarto_python_path <- Sys.getenv("QUARTO_PYTHON")
-quarto_python_version <- gsub(".*/([0-9]+\\.[0-9]+\\.[0-9]+)/.*", "\\1", quarto_python_path)
-
-info$platform$python_version <- quarto_python_version
-```
-
-```{r, results="asis"}
+```{r}
+#| label: Platform
+#| results: asis
 cat("\n## Platform \n")
 info$platform
 cat("\n")
 ```
 
-```{r, results="asis", eval=(grepl("\\.py$", params$title)), fig.height = 0.5}
-library(knitr)
+```{r}
+#| label: Python
+#| results: asis
+#| eval: !expr grepl("\\.py$", params$title)
+#| fig.height: 0.5
 cat("\n## Python Packages\n")
 
-pip_list_output <- system("pip list", intern=TRUE)
-pip_list_df <- as.data.frame(do.call(rbind, strsplit(pip_list_output, "\\s{2,}")))
-pip_list_df <- pip_list_df[-c(1, 2), ]
-colnames(pip_list_df) <- c("Package", "Version")
-
-# Display the data frame as a knitr table
-
-row.names(pip_list_df) <- NULL
-pip_list_df |>
-  knitr::kable() |>
-  knitr::knit_print()
+info$python_packages
 ```
 
-```{r, results="asis", fig.height = 0.5}
+```{r}
+#| label: R
+#| results: asis
+#| fig.height: 0.5
 cat("\n## R Packages\n")
 
 if (!is.null(params$check_approved_folder_pkgs) |
@@ -148,23 +169,30 @@ if (!is.null(params$check_approved_folder_pkgs) |
 cat("\n")
 ```
 
-```{r, results="asis", fig.height = 0.5}
+```{r}
+#| results: asis
+#| fig.height: 0.5
 info$packages
 ```
 
-```{r, results="asis"}
+```{r}
+#| label: Environment
+#| results: asis
 cat("\n## Environment variables \n")
 info$environment
 cat("\n")
 ```
 
-```{r, results="asis"}
+```{r}
+#| label: Options
+#| results: asis
 cat("\n## Option settings\n")
 info$options
 cat("\n")
 ```
 
-```{r collect-objects}
+```{r}
+#| label: Output
 whirl_objects <- vector("list")
 
 if (exists("log_info")) {
diff --git a/inst/documents/python_modules.py b/inst/documents/python_modules.py
new file mode 100644
index 0000000..7d63501
--- /dev/null
+++ b/inst/documents/python_modules.py
@@ -0,0 +1,21 @@
+import json
+import sys
+import subprocess
+
+temp_dir = r.params['tmpdir']
+
+# Get a list of installed packages using pip list
+installed_packages = subprocess.check_output([sys.executable, '-m', 'pip', 'list', '-v']).decode('utf-8').strip().split('\n')[2:]
+installed_packages = {package.split()[0]: [package.split()[1], package.split()[2]] for package in installed_packages}
+
+imported_modules = {}
+for module_name, module in sys.modules.items():
+    if not (module_name.startswith('_') or module_name.startswith('.') or '.' in module_name):
+        if module_name in installed_packages:
+            imported_modules[module_name] = {
+                "version": installed_packages[module_name][0],
+                "installation_path": installed_packages[module_name][1]
+            }
+
+with open(f'{temp_dir}/python_imports.json', 'w') as json_file:
+    json.dump(imported_modules, json_file)
diff --git a/inst/examples/simple/python_error.py b/inst/examples/simple/python_error.py
new file mode 100644
index 0000000..66f6580
--- /dev/null
+++ b/inst/examples/simple/python_error.py
@@ -0,0 +1,6 @@
+
+1 + "a"
+
+raise TypeError("This is a type error for testing purposes")
+
+raise Exception("Error also for testing")
diff --git a/inst/examples/simple/python_success.py b/inst/examples/simple/python_success.py
new file mode 100644
index 0000000..b3cbbe6
--- /dev/null
+++ b/inst/examples/simple/python_success.py
@@ -0,0 +1,9 @@
+import pandas
+
+a = 2 + 2
+print(a)
+
+for i in range(10):
+    print(i)
+
+print("Hello, World!")
diff --git a/inst/examples/simple/python_warning.py b/inst/examples/simple/python_warning.py
new file mode 100644
index 0000000..e378c26
--- /dev/null
+++ b/inst/examples/simple/python_warning.py
@@ -0,0 +1,3 @@
+import warnings
+
+warnings.warn("test warning")
diff --git a/tests/testthat/test-internal_run.R b/tests/testthat/test-internal_run.R
index b407ebc..4362cea 100644
--- a/tests/testthat/test-internal_run.R
+++ b/tests/testthat/test-internal_run.R
@@ -3,17 +3,20 @@ test_that("testing internal_run()", {
   config_to_config <- system.file("examples/demo/config_to_config.yaml", package = "whirl")
 
   #A config file
-  withr::with_dir(tempdir(), {
+  withr::with_tempdir({
     queue <- whirl_queue$new()
     internal_run(input = file_config, steps = NULL, level = 1, queue = queue) |>
       expect_no_error()
   })
 
   #A config file
-  withr::with_dir(tempdir(), {
+  withr::with_tempdir({
     queue <- whirl_queue$new()
     internal_run(input = config_to_config, steps = NULL, level = 1, queue = queue) |>
       expect_no_error()
   })
 
 })
+
+withr::with_dir(tempdir(), {
+  queue <- whirl_queue$new()})