Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce the new DDL #957

Merged
merged 54 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from 41 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
c9887c9
approach 3
gogonzo Nov 1, 2023
e7a3526
[skip actions] Roxygen Man Pages Auto Update
dependabot-preview[bot] Nov 1, 2023
e9df325
ddl_login_password wrapper
gogonzo Nov 1, 2023
c2e84eb
[skip actions] Roxygen Man Pages Auto Update
dependabot-preview[bot] Nov 1, 2023
a8f85ca
remove redundant
gogonzo Nov 1, 2023
dc3d7e3
[skip actions] Roxygen Man Pages Auto Update
dependabot-preview[bot] Nov 1, 2023
08c7214
fixes
gogonzo Nov 2, 2023
221976a
:O
gogonzo Nov 2, 2023
55a5a80
modifying class to be defacto simpler version of `teal_module`
gogonzo Nov 3, 2023
51b3f79
[skip actions] Roxygen Man Pages Auto Update
dependabot-preview[bot] Nov 3, 2023
e43c76a
need to quote to avoid eval of language objects
gogonzo Nov 3, 2023
f0c1cc6
allow no server_args
gogonzo Nov 3, 2023
df885f3
change assertion to list(ui, server)!
gogonzo Nov 3, 2023
a324e21
tighten up asserts
gogonzo Nov 3, 2023
3ac84f9
fix asserts in teal with splash
gogonzo Nov 3, 2023
e11acd7
example ddl
gogonzo Nov 3, 2023
0bb2e70
[skip actions] Roxygen Man Pages Auto Update
dependabot-preview[bot] Nov 3, 2023
9244d33
review suggestions
gogonzo Nov 6, 2023
d6b8d2f
[skip actions] Roxygen Man Pages Auto Update
dependabot-preview[bot] Nov 6, 2023
0ce0eb5
fix notifications
gogonzo Nov 6, 2023
71c1501
[skip actions] Roxygen Man Pages Auto Update
dependabot-preview[bot] Nov 6, 2023
0c52e1a
removing delayed_data and ddl
gogonzo Nov 6, 2023
9b7de0f
add test for teal::init
gogonzo Nov 6, 2023
640c27b
- introduce `data_module`
gogonzo Nov 8, 2023
7a451d3
Merge 640c27bb8816d38081f1d2a5b6cd0adf1f406d6b into 60755551478f0f666…
gogonzo Nov 8, 2023
82fb25a
[skip actions] Restyle files
github-actions[bot] Nov 8, 2023
cfb27bc
add vignette
gogonzo Nov 8, 2023
c3adb74
Merge cfb27bcd90783cae615234f26b39879afc812579 into 60755551478f0f666…
gogonzo Nov 8, 2023
147c02f
[skip actions] Restyle files
github-actions[bot] Nov 8, 2023
cd292ea
empty
gogonzo Nov 8, 2023
e80c8ce
data_module -> teal_data_module
gogonzo Nov 8, 2023
724dcdb
WIP tests
gogonzo Nov 8, 2023
a98682d
- add asserts on datanames in teal::init
gogonzo Nov 9, 2023
507fbc3
lint
gogonzo Nov 9, 2023
0f7accd
edit vignette
Nov 9, 2023
0836745
Merge 0f7accd87e42a3bd6c4e0c041f8448806ac67029 into 60755551478f0f666…
gogonzo Nov 9, 2023
e511517
[skip actions] Restyle files
github-actions[bot] Nov 9, 2023
bbaff1f
update documentation for teal_data_module
Nov 9, 2023
2a38182
WIP review
gogonzo Nov 9, 2023
971c7d5
protect teal from teal_data_module errors
gogonzo Nov 10, 2023
72abf29
@kartikayakirar
gogonzo Nov 10, 2023
457ae2c
fix typo
chlebowa Nov 10, 2023
9d63d40
add NEWS entry and reorganize in proper sections
gogonzo Nov 10, 2023
f84ad0b
@chlebowa @ruckip review
gogonzo Nov 10, 2023
7d5ccc3
review
gogonzo Nov 10, 2023
6570474
fix error handling when simple error in teal_data_module
gogonzo Nov 10, 2023
8a0675f
change error message when teal_data_module doesn't return reactive
gogonzo Nov 10, 2023
422efa6
review
gogonzo Nov 10, 2023
8e0cc87
@ruckip review
gogonzo Nov 13, 2023
2c928b2
@ruckip review
gogonzo Nov 13, 2023
54f05ba
update snapshot manager documentation
Nov 13, 2023
0b12f7b
link to the vignette
gogonzo Nov 13, 2023
f00e181
adding link to vignette and fix error message
gogonzo Nov 13, 2023
019f26c
lintr
gogonzo Nov 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions DESCRIPTION
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ Collate:
'show_rcode_modal.R'
'tdata.R'
'teal.R'
'teal_data_module.R'
'teal_reporter.R'
'teal_slices-store.R'
'teal_slices.R'
Expand Down
1 change: 1 addition & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export(reporter_previewer_module)
export(show_rcode_modal)
export(srv_teal_with_splash)
export(tdata2env)
export(teal_data_module)
export(teal_slices)
export(ui_teal_with_splash)
export(validate_has_data)
Expand Down
15 changes: 13 additions & 2 deletions R/dummy_functions.R
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ example_datasets <- function() { # nolint
#'
#' @description `r lifecycle::badge("experimental")`
#' @inheritParams module
#' @param src (`logical(1)`) whether to display reproducible R code in the module.
#' @return A `teal` module which can be included in the `modules` argument to [teal::init()].
#' @examples
#' app <- init(
Expand All @@ -88,21 +89,31 @@ example_datasets <- function() { # nolint
#' shinyApp(app$ui, app$server)
#' }
#' @export
example_module <- function(label = "example teal module", datanames = "all") {
chlebowa marked this conversation as resolved.
Show resolved Hide resolved
example_module <- function(label = "example teal module", datanames = "all", src = TRUE) {
checkmate::assert_string(label)
module(
label,
server = function(id, data) {
checkmate::assert_class(data, "tdata")
moduleServer(id, function(input, output, session) {
output$text <- renderPrint(data[[input$dataname]]())
teal.widgets::verbatim_popup_srv(
id = "rcode",
verbatim_content = attr(data, "code")(),
title = "Association Plot"
)
})
},
ui = function(id, data) {
ns <- NS(id)
teal.widgets::standard_layout(
output = verbatimTextOutput(ns("text")),
encoding = selectInput(ns("dataname"), "Choose a dataset", choices = names(data))
encoding = div(
selectInput(ns("dataname"), "Choose a dataset", choices = names(data)),
if (src) {
teal.widgets::verbatim_popup_ui(ns("rcode"), "Show R code")
}
)
)
},
datanames = datanames
Expand Down
65 changes: 29 additions & 36 deletions R/init.R
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@
#' an end-user, don't use this function, but instead this module.
#'
#' @param data (`TealData` or `TealDataset` or `TealDatasetConnector` or `list` or `data.frame`
#' or `MultiAssayExperiment`, `teal_data`)\cr
#' or `MultiAssayExperiment`, `teal_data`, `teal_data_module`)\cr
gogonzo marked this conversation as resolved.
Show resolved Hide resolved
#' `R6` object as returned by [teal.data::cdisc_data()], [teal.data::teal_data()],
#' [teal.data::cdisc_dataset()], [teal.data::dataset()], [teal.data::dataset_connector()] or
#' [teal.data::cdisc_dataset_connector()] or a single `data.frame` or a `MultiAssayExperiment`
#' [teal.data::cdisc_dataset_connector()] or [teal::teal_data_module()] or a single `data.frame` or
#' a `MultiAssayExperiment`
#' or a list of the previous objects or function returning a named list.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to add:
or teal_data_module object created by teal_data_module()

(the latter with link)

#' NOTE: teal does not guarantee reproducibility of the code when names of the list elements
#' do not match the original object names. To ensure reproducibility please use [teal.data::teal_data()]
Expand Down Expand Up @@ -114,11 +115,11 @@ init <- function(data,
footer = tags$p(),
id = character(0)) {
logger::log_trace("init initializing teal app with: data ({ class(data)[1] }).")

if (!inherits(data, c("TealData", "teal_data"))) {
if (!inherits(data, c("TealData", "teal_data", "teal_data_module"))) {
data <- teal.data::to_relational_data(data = data)
}
checkmate::assert_multi_class(data, c("TealData", "teal_data"))

checkmate::assert_multi_class(data, c("TealData", "teal_data", "teal_data_module"))
checkmate::assert_multi_class(modules, c("teal_module", "list", "teal_modules"))
checkmate::assert_string(title, null.ok = TRUE)
checkmate::assert(
Expand All @@ -142,26 +143,13 @@ init <- function(data,
if (length(landing) > 1L) stop("Only one `landing_popup_module` can be used.")
modules <- drop_module(modules, "teal_module_landing")

# resolve modules datanames
datanames <- teal.data::get_dataname(data)
join_keys <- teal.data::get_join_keys(data)
modules <- resolve_modules_datanames(modules = modules, datanames = datanames, join_keys = join_keys)

if (!inherits(filter, "teal_slices")) {
checkmate::assert_subset(names(filter), choices = datanames)
# list_to_teal_slices is lifted from teal.slice package, see zzz.R
# This is a temporary measure and will be removed two release cycles from now (now meaning 0.13.0).
filter <- list_to_teal_slices(filter)
}
# convert teal.slice::teal_slices to teal::teal_slices
filter <- as.teal_slices(as.list(filter))

# Calculate app hash to ensure snapshot compatibility. See ?snapshot. Raw data must be extracted from environments.
# Calculate app hash to ensure snapshot compatibility.
# See ?snapshot. Raw data must be extracted from environments.
gogonzo marked this conversation as resolved.
Show resolved Hide resolved
hashables <- mget(c("data", "modules"))
hashables$data <- if (inherits(hashables$data, "teal_data")) {
as.list(hashables$data@env)
} else if (inherits(hashables$data, "ddl")) {
attr(hashables$data, "code")
} else if (inherits(data, "teal_data_module")) {
body(data$server)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get the idea behind hashables but it looks to be an unique identifier of the input data (e.g. you do calculate hash on env with data tables in it). Here you are extracting server function body and calculate hashes on this. Please note that the same server function might return different object (vide: runif() but more realisticaly here: module with file upload). I don't think this is what we expect in the next step.

(The same mistake below where we call hashables$data$get_code())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hashables are for different reason. Hashes are not used for reproducibility purposes, and they are not returned in the R code. This single hash is needed to identify an app. At this moment it is used as "id" for filter argument, to check whether filter is applied to the same app. This means:

  • if you have an app with dynamic data source you'd like to make sure that filters which you upload are done for this app.
  • filters done for other app (possibly for other modules and data) should not be possible to upload for this app

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the comment a few lines up to be more descriptive?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the comment a few lines up to be more descriptive?

Yes - that would be great for a person from the outside.

This single hash is needed to identify an app

So I was wrong with my interpretation but the problem stays the same. The way how you calculate the hash now identify whether the data loading module is the same. You can have multiple apps with the same data loading module.

I have other objections for other cases of that if but it's going beyond the scope of this PR so will keep quiet here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If app1 and app2 use the same data module, then app2 can use a filter snapshot created in app1.
Note that modules is also hashed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If app1 and app2 use the same data module, then app2 can use a filter snapshot created in app1. Note that modules is also hashed.

So then few more comments:

  1. Just to clarify as I wanted to understand this better.
    IMO those two sentences conflicting with each other. If app1 and app2 uses the same data module but different analysis modules then the has won't be identical (as we include modules as well) and we cannot re-use the filter. Am I missing something here?

  2. Also: are you sure?

Let's leave 1. and assume same data module and same analysis modules. Let's assume the following:

  • module X that allows to load multiple csv files and apply datanames according to file names
  • APP1: data module X that returns datasets A, B, C + module on "all" datanames
  • APP2: data module X that returns datasets K, L, M + module on "all" datanames

So the hash here will be the same and are you sure that we can re-apply filters from APP1 to APP2? Am I right here?

I have the impression that this becomes imperfect as we introduce the concept of teal_data_module in which the same module can return different datasets. I am just trying to find a good test case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ad 1. Sorry for the confusing phrasing. I did mean that in order to use an uploaded snapshot, it must come from an app that used the same data (module) and modules. You are right, those are not exactly different apps. Which is the point ot the hash.

Ad 2. For the data module to return different (or even differently named) datasets it would have to have either a) different server code or b) the same server code that modifies data (see the new vignette for examples), where different data is supplied. The former seems safe enough. The latter, not necessarily.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AD 1. Thanks. Now it's more clear.

AD 2. I am actually thinking about yet another possibility of dynamically named object -> things like: x <- foo_named_list_of_df(input$foo, input$bar); list2env(x, env = current_env()). The server code is the same but it can return different things based on user inputs. This essentially means that our identifier mechanism based on hashing of server fun body is imperfect (also taking into account use case b) you mentioned).

Copy link
Contributor

@chlebowa chlebowa Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. So in the particular case you describe snapshot compatibility is upheld. Data names may be different but the data objects themselves are the same. (wrong, datanames are used in teal_slices 🤦) If you decide to change column names, then you're in trouble.

The question is, is this a valid use case? Are dataset names (let alone variable names) ever left up to the app user? We don't even provide for the app user to be able to modify variable labels (yet).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is. To me, that's one of the greatest things about teal-data-module concept. It essentially unlocks total flexibility of data names, column names, column types and everything else. We even say in the docs that (the only) limitation is that the returned object has to be of teal_data class. How you achieve that - it's up to the module developer. And that's great.

This feature does not fit well with our concept implementation of filters re-applicability which I am highlighting here. Now I don't know if we should invest in finding appropriate way of implement this this (here) or revisit this as a whole.

} else if (hashables$data$is_pulled()) {
sapply(get_dataname(hashables$data), simplify = FALSE, function(dn) {
hashables$data$get_dataset(dn)$get_raw_data()
Expand All @@ -172,20 +160,8 @@ init <- function(data,

attr(filter, "app_id") <- rlang::hash(hashables)

# check teal_slices
for (i in seq_along(filter)) {
dataname_i <- shiny::isolate(filter[[i]]$dataname)
if (!dataname_i %in% datanames) {
stop(
sprintf(
"filter[[%s]] has a different dataname than available in a 'data':\n %s not in %s",
i,
dataname_i,
toString(datanames)
)
)
}
}
# convert teal.slice::teal_slices to teal::teal_slices
filter <- as.teal_slices(as.list(filter))

if (isTRUE(attr(filter, "module_specific"))) {
module_names <- unlist(c(module_labels(modules), "global_filters"))
Expand Down Expand Up @@ -213,6 +189,23 @@ init <- function(data,
}
}

if (inherits(data, "teal_data")) {
# in case of teal_data_module this check is postponed to the srv_teal_with_splash
is_modules_ok <- check_modules_datanames(modules, teal.data::datanames(data))
if (!isTRUE(is_modules_ok)) {
logger::log_error(is_modules_ok)
stop(is_modules_ok)
}

is_filter_ok <- check_filter_datanames(filter, teal.data::datanames(data))
if (!isTRUE(is_filter_ok)) {
logger::log_warn(is_filter_ok)
warning(is_filter_ok)
gogonzo marked this conversation as resolved.
Show resolved Hide resolved
# we allow app to continue if applied filters are outside
# of possible data range
}
}

# Note regarding case `id = character(0)`:
# rather than using `callModule` and creating a submodule of this module, we directly modify
# the `ui` and `server` with `id = character(0)` and calling the server function directly
Expand Down
9 changes: 6 additions & 3 deletions R/module_nested_tabs.R
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ ui_nested_tabs.teal_module <- function(id, modules, datasets, depth = 0L, is_mod
checkmate::assert_class(datasets, class = "FilteredData")
ns <- NS(id)

args <- isolate(teal.transform::resolve_delayed(modules$ui_args, datasets))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure?
if yes - is this the only place where we use this function?
if yes - shall we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, too early for this. I was testing something and pushed by mistake. Reverting.

FYI we will remove resolve_delay from ui functions anyway soon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Yes - I am expecting this to be removed but was surprised this happened so fast :P

args <- c(list(id = ns("module")), args)
args <- c(list(id = ns("module")), modules$ui_args)

if (is_arg_used(modules$ui, "datasets")) {
args <- c(args, datasets = datasets)
Expand Down Expand Up @@ -297,7 +296,11 @@ srv_nested_tabs.teal_module <- function(id, datasets, modules, is_module_specifi
checkmate::assert_class(datasets, "FilteredData")
checkmate::assert_class(trigger_data, "reactiveVal")

datanames <- if (is.null(module$datanames)) datasets$datanames() else module$datanames
datanames <- if (is.null(module$datanames) || identical(module$datanames, "all")) {
datasets$datanames()
} else {
unique(module$datanames) # todo: include parents! unique shouldn't be needed here!
}

# list of reactive filtered data
data <- sapply(
Expand Down
8 changes: 7 additions & 1 deletion R/module_tabs_with_filters.R
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,13 @@ srv_tabs_with_filters <- function(id,
)

if (!is_module_specific) {
active_datanames <- reactive(active_module()$datanames)
active_datanames <- reactive({
if (identical(active_module()$datanames, "all")) {
singleton$datanames()
} else {
active_module()$datanames
}
})
singleton <- unlist(datasets)[[1]]
singleton$srv_filter_panel("filter_panel", active_datanames = active_datanames)

Expand Down
39 changes: 22 additions & 17 deletions R/module_teal.R
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ srv_teal <- function(id, modules, raw_data, filter = teal_slices()) {
}
)

reporter <- teal.reporter::Reporter$new()
if (is_arg_used(modules, "reporter") && length(extract_module(modules, "teal_module_previewer")) == 0) {
modules <- append_module(modules, reporter_previewer_module())
}

env <- environment()
datasets_reactive <- eventReactive(raw_data(), {
env$progress <- shiny::Progress$new(session)
Expand All @@ -171,6 +176,7 @@ srv_teal <- function(id, modules, raw_data, filter = teal_slices()) {
# Singleton starts with only global filters active.
filter_global <- Filter(function(x) x$id %in% attr(filter, "mapping")$global_filters, filter)
datasets_singleton$set_filter_state(filter_global)

module_datasets <- function(modules) {
if (inherits(modules, "teal_modules")) {
datasets <- lapply(modules$children, module_datasets)
Expand All @@ -180,11 +186,16 @@ srv_teal <- function(id, modules, raw_data, filter = teal_slices()) {
} else if (isTRUE(attr(filter, "module_specific"))) {
# we should create FilteredData even if modules$datanames is null
# null controls a display of filter panel but data should be still passed
datanames <- if (is.null(modules$datanames)) teal.data::get_dataname(raw_data()) else modules$datanames
# todo: subset tdata object to datanames
datanames <- if (is.null(modules$datanames) || modules$datanames == "all") {
include_parent_datanames(raw_data()@datanames, raw_data()@join_keys) # todo: use methods instead
gogonzo marked this conversation as resolved.
Show resolved Hide resolved
} else {
modules$datanames
}
# todo: subset teal_data to datanames
datasets_module <- teal_data_to_filtered_data(raw_data())

# set initial filters
# - filtering filters for this module
slices <- Filter(x = filter, f = function(x) {
x$id %in% unique(unlist(attr(filter, "mapping")[c(modules$label, "global_filters")])) &&
x$dataname %in% datanames
Expand All @@ -199,29 +210,23 @@ srv_teal <- function(id, modules, raw_data, filter = teal_slices()) {
datasets_singleton
}
}
datasets <- module_datasets(modules)

logger::log_trace("srv_teal@4 Raw Data transferred to FilteredData.")
datasets
module_datasets(modules)
})

reporter <- teal.reporter::Reporter$new()
if (is_arg_used(modules, "reporter") && length(extract_module(modules, "teal_module_previewer")) == 0) {
modules <- append_module(modules, reporter_previewer_module())
}

# Replace splash / welcome screen once data is loaded ----
# ignoreNULL to not trigger at the beginning when data is NULL
# just handle it once because data obtained through delayed loading should
# usually not change afterwards
# if restored from bookmarked state, `filter` is ignored
observeEvent(datasets_reactive(), ignoreNULL = TRUE, once = TRUE, {
gogonzo marked this conversation as resolved.
Show resolved Hide resolved

observeEvent(datasets_reactive(), {
logger::log_trace("srv_teal@5 setting main ui after data was pulled")
env$progress$set(0.5, message = "Setting up main UI")
on.exit(env$progress$close())
# main_ui_container contains splash screen first and we remove it and replace it by the real UI
env$progress$set(0.5, message = "Setting up main UI")
datasets <- datasets_reactive()

removeUI(sprintf("#%s:first-child", session$ns("main_ui_container")))
# main_ui_container contains splash screen first and we remove it and replace it by the real UI
removeUI(sprintf("#%s > div:nth-child(1)", session$ns("main_ui_container")))
insertUI(
selector = paste0("#", session$ns("main_ui_container")),
where = "beforeEnd",
Expand All @@ -230,7 +235,7 @@ srv_teal <- function(id, modules, raw_data, filter = teal_slices()) {
ui = div(ui_tabs_with_filters(
session$ns("main_ui"),
modules = modules,
datasets = datasets_reactive(),
datasets = datasets,
filter = filter
)),
# needed so that the UI inputs are available and can be immediately updated, otherwise, updating may not
Expand All @@ -242,7 +247,7 @@ srv_teal <- function(id, modules, raw_data, filter = teal_slices()) {
# registered once (calling server functions twice would trigger observers twice each time)
active_module <- srv_tabs_with_filters(
id = "main_ui",
datasets = datasets_reactive(),
datasets = datasets,
modules = modules,
reporter = reporter,
filter = filter
Expand Down
Loading