736 Allow custom card functions in modules #737

chlebowa · 2024-04-19T14:41:54Z

Solves #736

Allows app developers to pass card functions to modules as arguments.

Demonstration

Example app demonstrating use of locally defined card function:

example app

rm(list = ls())

library(teal)
devtools::load_all("./teal.modules.general")
library(teal.widgets)

local_card_function <- function(comment, label) {
  card <- teal::report_card_template(
    title = "Distribution Plot",
    label = label,
    with_filter = FALSE,
    filter_panel_api = filter_panel_api
  )
  card$append_text("Dummy Card", "header2")
  card$append_text(paste(
    "This report card was generated by a card function",
    "defined in the global environment."
  ))
  if (!comment == "") {
    card$append_text("Comment", "header3")
    card$append_text(comment)
  }
  card
}

data <- teal_data() |> within({
  iris <- iris
  mtcars <- mtcars
  faithful <- faithful
})
datanames(data) <- c("iris", "mtcars", "faithful")
filter <- teal_slices(
  teal_slice("iris", "Petal.Length", selected = c(1, 5.1)),
  teal_slice("iris", "Species", selected = c("setosa", "virginica")),
  teal_slice("mtcars", "cyl", selected = 6L),
  teal_slice("faithful", "waiting", selected = c(76, 82)),
  module_specific = TRUE,
  mapping = list(
    # "Distribution Module" = "iris Petal.Length",
    "Example" = "faithful waiting",
    "global_filters" = "iris Species"
  )
)
app <- init(
  data = data,
  modules = modules(
    tm_g_distribution(
      label = "Default card function",
      dist_var = data_extract_spec(
        dataname = "iris",
        select = select_spec(variable_choices("iris"), "Petal.Length")
      ),
      ggplot2_args = ggplot2_args(
        labs = list(subtitle = "Plot generated by Distribution Module")
      )
    ),
    tm_g_distribution(
      label = "Local card function",
      dist_var = data_extract_spec(
        dataname = "iris",
        select = select_spec(variable_choices("iris"), "Petal.Length")
      ),
      ggplot2_args = ggplot2_args(
        labs = list(subtitle = "Plot generated by Distribution Module")
      ),
      card_function = local_card_function
    ),
    example_module("Example")
  ),
  filter = filter
)

runApp(app, launch.browser = TRUE)

Also works with card function defined in another package (not shown).

Rationale

Currently each module has a hard-coded card function so the module developer has full control over what is added to the report. Neither the app user, nor the app developer has any influence on this.

Being able to redefine the card function is a valid use case.

Scoping Consideration

Current card functions are defined within the module server functions because they rely heavily on bindings that exist in the execution environment of the server function. In order for a function defined elsewhere to be able to access those bindings, a function is introduced, hydrate_function. hydrate_function changes the enclosing environment of a function (here, a card function) to bring another frame into scope. It can also add other bindings to the funciton's enclosure.

Implementation

Card functions from all modules that support reporting are isolated as separate, unexported functions. The naming convention is <module_name>_card_function.
All modules that support reporting receive a card_function formal argument. If not provided, falls back to the corresponding card function for the module.
Within modules, the card function, whether one provided in the call or the fallback value, is passed through hydrate_function to extend its scope to include the module server function's execution environment. This call is identical in all modules:
card_function <- hydrate_function(card_function, with_filter, filter_panel_api).

NOTE

This is a proof of concept, changes have only been made to tm_g_distribution.

Progress

pawelru · 2024-04-22T14:12:10Z

Being able to redefine the card function is a valid use case.

I very much agree with this. I like the request and I do like the idea for implementation - that is: a card factory function. There are some details that I am still not quite sure yet and these could potentially require changes in teal.reporter itself (hopefully not but I cannot guarantee).

pawelru · 2024-04-22T14:27:48Z

R/tm_g_distribution.R

    }
    ###
  })
 }
+
+#' @keywords internal
+tm_g_distribution_card_function <- function(comment, label) { #nolint: object_length.


my main concern is that this is not a true list of required arguments. We are using with_filter and filter_panel_api. Moreover, we are using dist_r(), pws1 and many more object from the parent environment.
You have addressed this with hydrate_function but I think it would be better not to be in need of such functionality. I can think of a few ways how to do this:

extend function arguments

pass data as environment

this being a module?

(I really hope that there is no strict check for these and only these argument somewhere in the teal.reporter).

Each has its pros and cons and we probably need to think more which one would be best for this task. Glad you stopped early to allow for a discussion like this.

Looking at the changes - this is how it was written in the past so I definitely not blaming you for this. This PR is a great opportunity to make it right.

(I really hope that there is no strict check for these and only these argument somewhere in the teal.reporter).

There is one, actually. I considered just adding ... to the card function's formals but that is forbidden. I think it would work but I am not sure about resolving conflicts between the variables passed to the ellipsis and bindings in the card function's parent environment. If we can figure out a way for the card functions to be pure, this would not be a problem.

I tried to limit the proposed changes to the module packages because I assumed modifying teal.reporter is off the table.

Note that passing the caller environment is not sufficient, at least in this module (I assume in others as well). with_filter and filter_panel_api are also required but they exist in the caller's parent frame, not in the caller itself.

I will be happy to discuss a satisfactory solution.

Note also that with the proposed solution the if (with_reporter) chunk would be the same in every module, which means _all code added to the module to enable reporting _would be the same in every module. The only difference would be the default card function, which is separate.

This in turn opens another possibility: have a function called with_reporter that would modify any module object by adding the code required for reporting into the module functions. Then adding support for reporting would be limited to something like modules(tm_g_distribution(...) |> with_reporter(card_fun = foo)). But that's a separate conversation.

Ahh that's bad :( Then I would say that please feel free to modify this in teal.reporter. I feel that this is the true root-cause of the issue. I also think that this (relaxing this check on formals) would make creating card-factory-fun much simpler and also encapsulated so that it can be run without any additional steps - just provide what's required.
That's exactly what I'm looking for - more simplicity. I'm a little bit afraid of introducing a requirement of hydrating a card-factory-fun. I don't think that most of our users (in particular: not developers) would handle this.

Have a look at this alternative: #742

I don't think it is full proof because of the way teal.reporter checks and handles formal arguments of card_fun:
https://github.com/insightsengineering/teal.reporter/blob/ad7fd2041f0d7cac83d0d97dbbe3ed7914a197ea/R/AddCardModule.R#L166

Note that environment shenanigans are already built into teal.reporter:
https://github.com/insightsengineering/teal.reporter/blob/ad7fd2041f0d7cac83d0d97dbbe3ed7914a197ea/R/AddCardModule.R#L173

EDIT: I had got a little bit of tunnel vision, with an added env argument wrapping the card function is not necessary. The alternative is not that bad now. Card functions are pure, though it may be tedious to reference env$ for all added bindings.

Haven't yet checked the alternative, but in this case I would opt for relaxing teal.reporter checks to allow passing ellipsis which would simply the process and would not require the usage of hydrating in this case.

passing env instead of ... also works!

Signed-off-by: Aleksander Chlebowski <[email protected]>

github-actions · 2024-05-02T06:36:52Z

CLA Assistant Lite bot ✅ All contributors have signed the CLA

chlebowa · 2024-05-02T06:40:00Z

I have read the CLA Document and I hereby sign the CLA

Signed-off-by: Aleksander Chlebowski <[email protected]>

Signed-off-by: Marcin <[email protected]>

m7pr · 2024-07-01T07:55:56Z

DESCRIPTION

@@ -35,6 +35,8 @@ Imports:
    DT (>= 0.13),
    forcats (>= 1.0.0),
    grid,
+    logger (>= 0.3.0),


Suggested change

logger (>= 0.3.0),

m7pr · 2024-07-01T07:56:40Z

DESCRIPTION

@@ -82,7 +84,7 @@ VignetteBuilder:
 Config/Needs/verdepcheck: haleyjeppson/ggmosaic, tidyverse/ggplot2,
    rstudio/shiny, insightsengineering/teal,
    insightsengineering/teal.transform, mllg/checkmate, tidyverse/dplyr,
-    rstudio/DT, tidyverse/forcats, r-lib/scales, daattali/shinyjs,
+    rstudio/DT, tidyverse/forcats, r-lib/rlang, r-lib/scales, daattali/shinyjs,


As rlang was introduced as a dependency, it also needs to be added here and in .pre-commit-config.yaml

m7pr · 2024-07-01T07:59:13Z

R/utils.R

+#'
+#' Add bindings of an environment to a function's parent environment.
+#'
+#' This allows any funciton to use bindings present in any environment


Suggested change

#' This allows any funciton to use bindings present in any environment

#' This allows any function to use bindings present in any environment

m7pr · 2024-07-01T07:59:26Z

R/utils.R

+#' Add bindings of an environment to a function's parent environment.
+#'
+#' This allows any funciton to use bindings present in any environment
+#' as if the funciton were defined there.


Suggested change

#' as if the funciton were defined there.

#' as if the function were defined there.

m7pr · 2024-07-01T07:59:45Z

R/utils.R

+#' One may also want to add variables that are not bound in the caller
+#' but are accessible from the caller, e.g. they exist in the caller's parent frame.
+#' This may happen in `shiny` modules because `moduleServer` is called
+#' by the module server function so the server funciton's arguments are in scope


Suggested change

#' by the module server function so the server funciton's arguments are in scope

#' by the module server function so the server function's arguments are in scope

m7pr · 2024-07-10T09:55:09Z

R/tm_g_distribution.R

@@ -118,7 +118,8 @@ tm_g_distribution <- function(label = "Distribution Module",
                              plot_height = c(600, 200, 2000),
                              plot_width = NULL,
                              pre_output = NULL,
-                              post_output = NULL) {
+                              post_output = NULL,
+                              card_function) {


Suggested change

card_function) {

card_function = tm_g_distribution_card_function) {

I would expose this so that it's visible which function is used. However that would require exposing N reporting cards for N modules

m7pr · 2024-07-10T09:55:41Z

R/tm_g_distribution.R

+  if (missing(card_function)) {
+    card_function <- tm_g_distribution_card_function
+  } else {
+    checkmate::assert_function(card_function)
+  }


Then this can only be limited to an assert_function if we go with this approach https://github.com/insightsengineering/teal.modules.general/pull/737/files#r1671978213

chlebowa and others added 7 commits April 19, 2024 12:18

add hydrate_function

59da2e1

upgrade rlang dependency

55dc29e

isolate card function and add arguments in tm_g_distribution

e2a1a40

amend documentation

676b2af

amend NEWS

180be37

add missing roxygen tag

d13fb79

Merge branch 'main' into custom_card_functions@main

377da56

pawelru reviewed Apr 22, 2024

View reviewed changes

chlebowa and others added 2 commits April 22, 2024 17:24

Merge branch 'main' into custom_card_functions@main

e33a666

Signed-off-by: Aleksander Chlebowski <[email protected]>

improve argument name

e2f51bb

chlebowa mentioned this pull request Apr 23, 2024

736 Allow pure custom card functions in modules #742

Open

chlebowa and others added 3 commits April 23, 2024 19:14

Merge branch 'main' into custom_card_functions@main

ae91503

Merge branch 'main' into custom_card_functions@main

5282405

fix documentation

76c118d

chlebowa changed the title ~~Allow custom card functions in modules~~ 736 Allow custom card functions in modules Apr 24, 2024

chlebowa mentioned this pull request Apr 25, 2024

[Feature Request]: Allow app dev to supply card function. #736

Open

3 tasks

Merge branch 'main' into custom_card_functions@main

54b5e09

chlebowa and others added 2 commits May 27, 2024 10:28

Merge branch 'main' into custom_card_functions@main

3023fb2

Signed-off-by: Aleksander Chlebowski <[email protected]>

Merge branch 'main' into custom_card_functions@main

1df517c

Signed-off-by: Marcin <[email protected]>

m7pr reviewed Jul 1, 2024

View reviewed changes

m7pr reviewed Jul 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

736 Allow custom card functions in modules #737

736 Allow custom card functions in modules #737

chlebowa commented Apr 19, 2024

pawelru commented Apr 22, 2024

pawelru Apr 22, 2024

chlebowa Apr 22, 2024

pawelru Apr 23, 2024

chlebowa Apr 23, 2024 •

edited

Loading

m7pr Jul 10, 2024

m7pr Jul 10, 2024

github-actions bot commented May 2, 2024 •

edited

Loading

chlebowa commented May 2, 2024

m7pr Jul 1, 2024

m7pr Jul 1, 2024

m7pr Jul 1, 2024

m7pr Jul 1, 2024

m7pr Jul 1, 2024

m7pr Jul 10, 2024

m7pr Jul 10, 2024

	#' This allows any funciton to use bindings present in any environment
	#' This allows any function to use bindings present in any environment

	#' as if the funciton were defined there.
	#' as if the function were defined there.

	#' by the module server function so the server funciton's arguments are in scope
	#' by the module server function so the server function's arguments are in scope

	card_function) {
	card_function = tm_g_distribution_card_function) {

736 Allow custom card functions in modules #737

Are you sure you want to change the base?

736 Allow custom card functions in modules #737

Conversation

chlebowa commented Apr 19, 2024

Demonstration

Rationale

Scoping Consideration

Implementation

NOTE

Progress

pawelru commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlebowa Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2024 • edited Loading

chlebowa commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chlebowa Apr 23, 2024 •

edited

Loading

github-actions bot commented May 2, 2024 •

edited

Loading