Fix decorator problems after a good first run #1515
Conversation
Unit Test Performance Difference
Additional test case details
Results for commit 2b15f07 ♻️ This comment has been updated with latest results. |
|
Code Coverage SummaryDiff against mainResults for commit: 925f581 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 25 suites 2m 25s ⏱️ Results for commit 925f581. ♻️ This comment has been updated with latest results. |
|
📑 Journal (rolling update):
Note: UI changes about placement and what should show where should be discussed #1509 Latest screenshot
Older screenshots_(nothing to see here yet)_ |
| ) | ||
|
|
||
| summary_table <- srv_data_summary("data_summary", module_teal_data) | ||
| summary_table <- srv_data_summary("data_summary", summary_data) # Only updates on success |
There was a problem hiding this comment.
I think this should not be in error state or hidden when transformators introduce an error.
The alternative is to use module_teal_data and remove the (newly) introduced summary_data as below.
Q: WDYT?
There was a problem hiding this comment.
I like that there is not data summary as it cannot be provided, but maybe it was hidden because this makes the different order pop up the error message more for the users. But I prefer to not have hidding elements messing with the layout of the app and leave the section even if empty.
There was a problem hiding this comment.
I left a couple of comments as I couldn't run yet the branch. The most important is to make it easier to pull the branch. I had messed my git config but I fixed it.
It would also great if the example app at the beginning of the PR could be updated to really open an app (to avoid me messing it and test different things)
| ) | ||
|
|
||
| summary_table <- srv_data_summary("data_summary", module_teal_data) | ||
| summary_table <- srv_data_summary("data_summary", summary_data) # Only updates on success |
There was a problem hiding this comment.
I like that there is not data summary as it cannot be provided, but maybe it was hidden because this makes the different order pop up the error message more for the users. But I prefer to not have hidding elements messing with the layout of the app and leave the section even if empty.
There was a problem hiding this comment.
Code looks good I left a couple of comments but I think this resolves the issue:
2025-05-23_15-00-44.mp4
I think the code on "example app" is not the one that generates the leading screenshots. But if there is further testing to be done let me know, the checks locally (and remotely) run well
| !inherits(try(transformed_teal_data(), silent = TRUE), "teal_data") && | ||
| inherits(filtered_teal_data(), "teal_data") |
There was a problem hiding this comment.
As transformed_teal_data depends on the output of filtered_teal_data it can be omitted and I think we would get the same result (at least on the app on the issue it works as before).
| !inherits(try(transformed_teal_data(), silent = TRUE), "teal_data") && | |
| inherits(filtered_teal_data(), "teal_data") | |
| !inherits(try(transformed_teal_data(), silent = TRUE), "teal_data") |
| # Disable all elements if original data is not yet a teal_data | ||
| observeEvent(data_original_handled(), { | ||
| shinyjs::toggleState( | ||
| "wrapper_panel", |
There was a problem hiding this comment.
I think there is a div with id = "wrapper" for modules tabs, but where does this _panel come from? Would it work on teal modules used directly on shiny? Just thinking aloud after resolving an issue coming from an id not matching between different parts.
| data_handled() | ||
| data_original_handled() |
There was a problem hiding this comment.
Nice trick, never thought about adding two expressions as a single event. Would be the same as to have only data_handled()? Or in reversed order?
|
@llrs-roche this is not forgotten, @gogonzo wants to add some functionality on top after we get an understanding of |
|
I was checking the board and I don't know were we stay on this PR/feature. The coments I left on the review are minor and at the time I thought it could be merged. Do we merge this @gogonzo ? |
PR was not enough as reason of failure lays deeper. I'd expect making a solution which can standardize error handling and would rather remove lines than add. There was one PR at that time I raised but I'm sceptical if anybody will review that anytime soon. I don't have capacity to handle this topic now :/ |
Sorry @llrs-roche I confused this PR with #1509 teal-app-failuresoptions(
teal.log_level = "ERROR",
teal.show_js_log = TRUE,
# teal.bs_theme = bslib::bs_theme(version = 5),
shiny.bookmarkStore = "server"
)
pkgload::load_all("teal")
tm_decorated_plot <- function(label = "module", transformators = list(), decorators = list(), datanames = "all") {
checkmate::assert_list(decorators, "teal_transform_module")
module(
label = label,
ui = function(id, decorators) {
ns <- NS(id)
div(
selectInput(ns("dataname"), label = "select dataname", choices = NULL, multiple = TRUE),
selectInput(ns("x"), label = "select x", choices = NULL, multiple = TRUE),
selectInput(ns("y"), label = "select y", choices = NULL, multiple = TRUE),
ui_transform_teal_data(ns("decorate"), transformators = decorators),
plotOutput(ns("plot")),
verbatimTextOutput(ns("text"))
)
},
server = function(id, data, decorators) {
moduleServer(id, function(input, output, session) {
observeEvent(data(), {
dataname <- if (length(input$dataname)) input$dataname else names(data())[1]
updateSelectInput(inputId = "dataname", choices = names(data()), selected = dataname)
})
observeEvent(input$dataname, {
req(input$dataname)
updateSelectInput(inputId = "x", choices = colnames(data()[[input$dataname]]))
updateSelectInput(inputId = "y", choices = colnames(data()[[input$dataname]]))
})
dataname <- reactive(req(input$dataname))
x <- reactive({
req(input$x, input$x %in% colnames(data()[[dataname()]]))
input$x
})
y <- reactive({
req(input$y, input$y %in% colnames(data()[[dataname()]]))
input$y
})
plot_data <- reactive({
# todo: make sure it triggers once on init
# and once on change of its input and once on change in previous stages
req(dataname(), x(), y())
Sys.sleep(5) # to mimic relatively long computation
within(data(),
{
plot <- ggplot2::ggplot(dataname, ggplot2::aes(x = x, y = y)) +
ggplot2::geom_point()
},
dataname = as.name(dataname()),
x = as.name(x()),
y = as.name(y())
)
})
plot_data_decorated_no_print <- srv_transform_teal_data(
"decorate",
data = plot_data,
transformators = decorators
)
plot_data_decorated <- reactive(
within(req(plot_data_decorated_no_print()), expr = plot)
)
plot_r <- reactive({
plot_data_decorated()[["plot"]]
})
output$plot <- renderPlot(plot_r())
output$text <- renderText({
teal.code::get_code(req(plot_data_decorated()))
})
})
},
ui_args = list(decorators = decorators),
server_args = list(decorators = decorators),
datanames = datanames,
transformators = transformators
)
}
make_data <- function(datanames = c("ADSL", "ADTTE")) {
data_obj <- teal.data::teal_data()
if ("ADSL" %in% datanames) {
data_obj <- within(data_obj, ADSL <- teal.data::rADSL)
}
if ("ADTTE" %in% datanames) {
data_obj <- within(data_obj, ADTTE <- teal.data::rADTTE)
}
join_keys(data_obj) <- default_cdisc_join_keys[datanames]
data_obj
}
data <- teal_data_module(
once = FALSE,
ui = function(id) {
ns <- NS(id)
tagList(
selectizeInput(
ns("errortype"),
label = "Error Type",
choices = c(
"ok", "insufficient datasets", "no data",
"qenv.error", "error in reactive", "validate error", "silent.shiny.error", "not a reactive"
)
)
)
},
server = function(id, ...) {
moduleServer(id, function(input, output, session) {
logger::log_trace("example_module_transform2 initializing.")
reactive({
switch(input$errortype,
ok = make_data(),
`insufficient datasets` = make_data(datanames = "ADSL"),
`no data` = teal_data(),
qenv.error = within(data(), stop("\nthis is qenv.error in teal_data_module\n")),
`error in reactive` = stop("\nerror in a reactive in teal_data_module\n"),
`validate error` = validate(need(FALSE, "\nvalidate error in teal_data_module\n")),
`silent.shiny.error` = req(FALSE)
)
})
})
}
)
trans <- teal_transform_module(
ui = function(id) {
ns <- NS(id)
tagList(
selectizeInput(
ns("errortype"),
label = "Error Type",
choices = c(
"ok", "insufficient datasets", "no data",
"qenv.error", "error in reactive", "validate error", "silent.shiny.error", "not a reactive"
)
)
)
},
server = function(id, data) {
moduleServer(id, function(input, output, session) {
logger::log_trace("example_module_transform2 initializing.")
# to check if data with error causes problems
data2 <- reactive(data())
data3 <- eventReactive(data2(), data2())
observeEvent(data3(), {
# do nothing
})
reactive({
# notes: make sure it:
# - triggers once on init
# - once on change of its input
# - once on change in data input
new_data <- switch(input$errortype,
ok = data3(),
`insufficient datasets` = data3()["ADSL"],
`no data` = teal_data(),
qenv.error = within(teal_data(), stop("\nthis is qenv.error in teal_transform_module\n")),
`error in reactive` = stop("\nerror in a reactive in teal_transform_module\n"),
`validate error` = validate(need(FALSE, "\nvalidate error in teal_transform_module\n")),
`silent.shiny.error` = req(FALSE)
)
new_data
})
})
}
)
trans_empty <- teal_transform_module(
server = function(id, data) {
moduleServer(id, function(input, output, session) {
reactive({
validate(need(nrow(data()$ADSL) > 250, "ADSL needs 250 rows"))
data()
})
})
}
)
decor <- teal_transform_module(
label = "X-axis decorator",
ui = function(id) {
ns <- NS(id)
tagList(
selectizeInput(
ns("action"),
label = "Action type",
choices = c(
"nothing", "decorate", "no data",
"qenv.error", "error in reactive", "validate error", "silent.shiny.error", "not a reactive"
)
)
)
},
server = function(id, data) {
moduleServer(id, function(input, output, session) {
logger::log_trace("example_module_transform2 initializing.")
reactive({
switch(input$action,
nothing = data(),
`decorate` = data() |> within(plot <- plot + ggplot2::ggtitle("Decorated Title")),
`no data` = teal_data(),
qenv.error = within(teal_data(), stop("\nthis is qenv.error in teal_transform_module\n")),
`error in reactive` = stop("\nerror in a reactive in teal_transform_module\n"),
`validate error` = validate(need(FALSE, "\nvalidate error in teal_transform_module\n")),
`silent.shiny.error` = req(FALSE)
)
})
})
}
)
app <- teal::init(
data = data,
modules = modules(
modules(
label = "first tab",
tm_decorated_plot(
"mod-2",
transformators = list(trans, trans, trans_empty),
decorators = list(decor, decor),
datanames = c("ADSL", "ADTTE")
),
example_module()
)
),
filter = teal_slices(
teal_slice("ADSL", "SEX"),
teal_slice("ADSL", "AGE", selected = c(18L, 65L)),
teal_slice("ADTTE", "PARAMCD", selected = "CRSD"),
include_varnames = list(
ADSL = c("SEX", "AGE")
)
)
)
runApp(app)
|
|
Thanks to the app, I reviewed this again: Data summary is present even when there is an error on qenv: Screenshot
Errors are duplicated for each transform module: Screenshot
Error messages are different on the UI between the transform Data panel and the main panel: Screenshot
Initial error pops a modal and then the summary data is different from what the error says: Screenshots
In conclusion. it fixes the second issue (Last state is shown, be it an error or a plot) but fails to provide "consistent error messages in decorators". |
|
Summary of the meeting with @llrs-roche
It makes sense, the summary table should be reactive Action: grey out the "no datasets to show" in summary table
Plan: only show in transformators if the error occurred there
The error should also appear in the main module UI note from Lluis: see if namespace is wrong
Bug when there is no UI for the transfomators and it enters an error state Expected result is for the pseudo-UI to be visible with the error showing Extra mile would be to modify the notification when filters are removed due to incompatible datanames.
|
Pull Request
Fixes #1511
Changes description
teal_transform_dataon initializationdecorators🤖 Example app