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

[Bug]: DT table ajax error on wrong column being selected. #284

Closed
3 tasks done
kartikeyakirar opened this issue Jun 26, 2024 · 14 comments · Fixed by #312
Closed
3 tasks done

[Bug]: DT table ajax error on wrong column being selected. #284

kartikeyakirar opened this issue Jun 26, 2024 · 14 comments · Fixed by #312
Assignees
Labels
bug Something isn't working core

Comments

@kartikeyakirar
Copy link
Contributor

What happened?

In the correlation plot, when the x/y-axis biomarker is changed for the first time, an AJAX error occurs indicating the column is not found. This issue is related to brush_data. Upon investigation, the intermediate output appears to be correct, and the DataTable (DT) is being updated properly.
image

sessionInfo()

No response

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct.

Contribution Guidelines

  • I agree to follow this project's Contribution Guidelines.

Security Policy

  • I agree to follow this project's Security Policy.
@kartikeyakirar kartikeyakirar added bug Something isn't working core labels Jun 26, 2024
@npaszty
Copy link
Contributor

npaszty commented Jul 24, 2024

@gogonzo @m7pr

data tables dialog still pops up.

I changed x biomarker to F2974907 BASE and that didn't trigger it but when I changed y biomarker to AST AVAL it was triggered. when I dismissed the dialog then the plot was created and brushing did work.

the table does inlucde the expected variables from the new selection but the data table warning is referring to the default y biomarker value combination of Albumin and AVAL. so something is stuck in the reactivity chain or the environment still

@m7pr
Copy link
Contributor

m7pr commented Jul 25, 2024

I guess it's a bit random and I can trigger it from a time to time when I keep changing inputs very fast. Googled it and found this https://datatables.net/manual/tech-notes/7 but it's not very helpful. It says there is an internal error, so maybe processing is happening way too fast.

@m7pr
Copy link
Contributor

m7pr commented Jul 25, 2024

Just managed to reproduce it on open data. When changing from ARM->ARMACT where other inputs are like on the print screen below

IGA
BASE
IGA
CHG

image
App
library(teal.goshawk)
data <- teal_data()
data <- within(data, {
  library(dplyr)
  library(stringr)
  
  # use non-exported function from goshawk
  h_identify_loq_values <- getFromNamespace("h_identify_loq_values", "goshawk")
  
  # original ARM value = dose value
  arm_mapping <- list(
    "A: Drug X" = "150mg QD",
    "B: Placebo" = "Placebo",
    "C: Combination" = "Combination"
  )
  color_manual <- c("150mg QD" = "#000000", "Placebo" = "#3498DB", "Combination" = "#E74C3C")
  # assign LOQ flag symbols: circles for "N" and triangles for "Y", squares for "NA"
  shape_manual <- c("N" = 1, "Y" = 2, "NA" = 0)
  
  set.seed(1)
  ADSL <- rADSL
  ADLB <- rADLB
  var_labels <- lapply(ADLB, function(x) attributes(x)$label)
  ADLB <- ADLB %>%
    mutate(AVISITCD = case_when(
      AVISIT == "SCREENING" ~ "SCR",
      AVISIT == "BASELINE" ~ "BL",
      grepl("WEEK", AVISIT) ~
        paste(
          "W",
          trimws(
            substr(
              AVISIT,
              start = 6,
              stop = str_locate(AVISIT, "DAY") - 1
            )
          )
        ),
      TRUE ~ NA_character_
    )) %>%
    mutate(AVISITCDN = case_when(
      AVISITCD == "SCR" ~ -2,
      AVISITCD == "BL" ~ 0,
      grepl("W", AVISITCD) ~ as.numeric(gsub("[^0-9]*", "", AVISITCD)),
      TRUE ~ NA_real_
    )) %>%
    # use ARMCD values to order treatment in visualization legend
    mutate(TRTORD = ifelse(grepl("C", ARMCD), 1,
                           ifelse(grepl("B", ARMCD), 2,
                                  ifelse(grepl("A", ARMCD), 3, NA)
                           )
    )) %>%
    mutate(ARM = as.character(arm_mapping[match(ARM, names(arm_mapping))])) %>%
    mutate(ARM = factor(ARM) %>%
             reorder(TRTORD)) %>%
    mutate(
      ANRHI = case_when(
        PARAMCD == "ALT" ~ 60,
        PARAMCD == "CRP" ~ 70,
        PARAMCD == "IGA" ~ 80,
        TRUE ~ NA_real_
      ),
      ANRLO = case_when(
        PARAMCD == "ALT" ~ 20,
        PARAMCD == "CRP" ~ 30,
        PARAMCD == "IGA" ~ 40,
        TRUE ~ NA_real_
      )
    ) %>%
    rowwise() %>%
    group_by(PARAMCD) %>%
    mutate(LBSTRESC = ifelse(
      USUBJID %in% sample(USUBJID, 1, replace = TRUE),
      paste("<", round(runif(1, min = 25, max = 30))), LBSTRESC
    )) %>%
    mutate(LBSTRESC = ifelse(
      USUBJID %in% sample(USUBJID, 1, replace = TRUE),
      paste(">", round(runif(1, min = 70, max = 75))), LBSTRESC
    )) %>%
    ungroup()
  attr(ADLB[["ARM"]], "label") <- var_labels[["ARM"]]
  attr(ADLB[["ANRHI"]], "label") <- "Analysis Normal Range Upper Limit"
  attr(ADLB[["ANRLO"]], "label") <- "Analysis Normal Range Lower Limit"
  
  # add LLOQ and ULOQ variables
  ADLB_LOQS <- h_identify_loq_values(ADLB, "LOQFL")
  ADLB <- left_join(ADLB, ADLB_LOQS, by = "PARAM")
})

datanames <- c("ADSL", "ADLB")
datanames(data) <- datanames

join_keys(data) <- default_cdisc_join_keys[datanames]

app <- init(
  data = data,
  modules = modules(
    tm_g_gh_correlationplot(
      label = "Correlation Plot",
      dataname = "ADLB",
      param_var = "PARAMCD",
      xaxis_param = choices_selected(c("ALT", "CRP", "IGA"), "ALT"),
      yaxis_param = choices_selected(c("ALT", "CRP", "IGA"), "CRP"),
      xaxis_var = choices_selected(c("AVAL", "BASE", "CHG", "PCHG"), "BASE"),
      yaxis_var = choices_selected(c("AVAL", "BASE", "CHG", "PCHG"), "AVAL"),
      trt_group = choices_selected(c("ARM", "ACTARM"), "ARM"),
      color_manual = c(
        "Drug X 100mg" = "#000000",
        "Placebo" = "#3498DB",
        "Combination 100mg" = "#E74C3C"
      ),
      shape_manual = c("N" = 1, "Y" = 2, "NA" = 0),
      plot_height = c(500, 200, 2000),
      facet_ncol = 2,
      visit_facet = TRUE,
      reg_line = FALSE,
      loq_legend = TRUE,
      font_size = c(12, 8, 20),
      dot_size = c(1, 1, 12),
      reg_text_size = c(3, 3, 10),
      hline_arb = c(40, 50),
      hline_arb_label = "arb hori label",
      hline_arb_color = c("red", "blue"),
      hline_vars = c("ANRHI", "ANRLO", "ULOQN", "LLOQN"),
      hline_vars_colors = c("green", "blue", "purple", "cyan"),
      hline_vars_labels = c("ANRHI Label", "ANRLO Label", "ULOQN Label", "LLOQN Label"),
      vline_vars = c("ANRHI", "ANRLO", "ULOQN", "LLOQN"),
      vline_vars_colors = c("yellow", "orange", "brown", "gold"),
      vline_vars_labels = c("ANRHI Label", "ANRLO Label", "ULOQN Label", "LLOQN Label"),
      vline_arb = c(50, 70),
      vline_arb_label = "arb vert A",
      vline_arb_color = c("green", "orange")
    )
  )
)
if (interactive()) {
  shinyApp(app$ui, app$server)
}

</summary?

@m7pr
Copy link
Contributor

m7pr commented Jul 25, 2024

And again, only changed Select X-axis Biomarker from initial value to IGA, the app started to compute something and during those computations I went again to change Select Y-axis Variable and got this issue.

image

@m7pr
Copy link
Contributor

m7pr commented Jul 25, 2024

And again, changed multiple inputs when app was still calculating something
image

@m7pr
Copy link
Contributor

m7pr commented Jul 25, 2024

My hypothesis is that

  1. We keep changing inputs - let's say 7 changes in total
  2. Shiny or DataTable put a queue of tables to be computed - 7 in total
  3. When app is calculating the 3rd table, we already changed that many inputs that we are past the 7th change
  4. App wants to calculate the table with the inputs set on the 3rd change, but that is already changed so it reflects changes from the 7th change
  5. There is an issue that DataTable has some inputs, but data no longer have those columns
  6. When the error is accepted, the app goes to the calculation of the 7th table and is able to create the table as the inputs for the table correspond to the data

@npaszty
Copy link
Contributor

npaszty commented Jul 25, 2024

@m7pr

thanks for looking into this. I guess it's a gremlin that we can't address easily. I can add a comment about this in the user guide tab of our apps but no one reads the user guide anyway. 🤣

what do you want to do with this issue? close?

@npaszty npaszty removed the priority label Jul 25, 2024
@npaszty
Copy link
Contributor

npaszty commented Jul 25, 2024

@m7pr

removed priority since you think it's a random gremlin and google suggests internal error.

@donyunardi
Copy link
Contributor

donyunardi commented Aug 12, 2024

As @m7pr pointed out, I only see the dialog boxes when I keep changing inputs consecutively.
Also, when I clicked OK, the app is still running and producing what seems to be the right visualization.

Acceptance Criteria

  • Investigate and suggest if there's a way to prevent dialog box to show

@m7pr
Copy link
Contributor

m7pr commented Sep 19, 2024

I think the issue will remain as this is DataTable issue, but we can try to maybe set some options so that the error/warning is not displayed on the app when this happens. I guess that would be the ultimate solution. We can not do much with the gremlin, but we may at least hide it and don't display it (if possible!).

@m7pr m7pr self-assigned this Sep 30, 2024
@m7pr
Copy link
Contributor

m7pr commented Sep 30, 2024

Hey, had a chance to deeply investigate the issue today. CC @donyunardi let me know your thoughts on this.

History

The same issue has been reported for the last 10 years :) (see comments on shiny/rstudio from 2014)

The cause

This is a know issue of JavaScript DataTables (which is the source for DT::datatables). The originate of the issue is as I expected

I guess it is not harmful, and I do not encourage or discourage it. My current guess is that the tables they tried to render are pretty big, and if they change the data extremely frequently (e.g. check/uncheck the columns at https://gallery.shinyapps.io/012-datatables/), R may be slow to respond, then for some reason, the Ajax call may return a data object of 8 columns whereas the current table on the page still has 9 columns. DataTables will warn in this case with an alert box.

This means that the changes of the input are way too frequent for the DataTable and it throws a warning because it can not process that many data at the same time.

Solutions

There are few solutions that we can discuss, to get things sorted out.

Solution 1

You can hide this alert warning for a particular DT::datables by passing an extra callback argument

DT::datatable(iris,  callback = DT::JS("$.fn.dataTable.ext.errMode = 'none';"))

However this requires updates in all DT::datatable in our codebase.

Solution 2

We can introduce a global change by specifying the below snippet in teal::ui_teal

tags$head(
  tags$script(
  '$(document).ready(function() {
     $.fn.dataTable.ext.errMode = "none";
   })'
  )
)

none vs throw

We do not necessarily need to hide the alert with none mode. We can just redirect it from the popup, to the browser console log with throw mode, so it's still visible in the debug mode for developers.

throw_to_console

Then the snippet is as follows

tags$head(
  tags$script(
  '$(document).ready(function() {
     $.fn.dataTable.ext.errMode = "throw";
   })'
  )
)

@m7pr
Copy link
Contributor

m7pr commented Oct 1, 2024

Created a PR that only affects this package #310

@donyunardi
Copy link
Contributor

Thanks for looking into this @m7pr

Since hiding it might not be the most optimal solution, I’m hesitant to apply it in the teal framework, and I’m also unsure if it’s safe to do so.

From what I understand, this issue relates to processing time when generating complex tables or plots, where users interact with inputs before the calculation is complete. This tells me that the problem lies within the module itself when DT::datatable() is used. Fortunately, there are options that the module developer can use to mitigate this, as you demonstrated.

For now, I would prefer to limit the scope and address this issue only in the package where it has been reported. So, I would vote for solution 1.

However this requires updates in all DT::datatable in our codebase.

I guess we could go this route, or we could focus on addressing only the problematic module, specifically, the one that requires more time to process the output. We only have a handful for teal.goshawk so perhaps this is okay.

m7pr added a commit that referenced this issue Oct 2, 2024
Solution for #284 that do not pop-up AJAX warnings from JavaScript
DataTable when computations happens too often and rendering is behind
the state of the data.

This adds a callback behavior for `DT::datatable`. We also tested the
global setup of this behavior in teal but we were unable to include
javascript in a way that it is executed after the object is created.
Other modules in other packages don't really need that as they have
better reactivity handling. The modules in teal.goshawk have very much
reactivitiy and could be refactored one day but that's a bigger project.

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@m7pr
Copy link
Contributor

m7pr commented Oct 2, 2024

@vedhav created an alternative solution where we added a debounce mode to each datatable so it does not calculate that fast when the input is changed #312

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants