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

nicer way to call a teal module outside of teal #624

Closed
Tracked by #32
pawelru opened this issue Apr 29, 2022 · 4 comments
Closed
Tracked by #32

nicer way to call a teal module outside of teal #624

pawelru opened this issue Apr 29, 2022 · 4 comments
Labels
enhancement New feature or request

Comments

@pawelru
Copy link
Contributor

pawelru commented Apr 29, 2022

As a non-teal app developer
I would like use teal module created in teal.modules.xyz in my non-teal app

The business case is a DASHDIS app that wanted to use tmc but they found it pretty difficult to do.

Please see following code:

x1 <- teal.modules.clinical::foo(...)
x2 <- teal.modules.clinical::bar(...)

####
d1 <- teal.data::cdisc_dataset(...)
d2 <- teal.data::cdisc_dataset(...)
d_dm <- teal.data::cdisc_data(d1, d2)
datasets <- teal.slice:::filtered_data_new(d_dm)
teal.slice:::filtered_data_set(d_dm, datasets)
####

shinyApp(
    ui = dashboardPage(
        dashboardHeader(),
        dashboardSidebar(
            sidebarMenu(
                menuItem("m1", tabName = "m1"),
                menuItem("m2", tabName = "m2")
            )
        ),
        dashboardBody(
            tabItems(
                tabItem(
                    "m1",
                    uiOutput("x1_outer")
                ),
                tabItem(
                    "m2", 
                    uiOutput("x2_outer")
                )
            )
        )
    ),
    server = function(input, output) {
        output$x1_outer <- renderUI(teal:::ui_nested_tabs("x1", modules = x1, datasets = datasets))
        output$x2_outer <- renderUI(teal:::ui_nested_tabs("x2", modules = x2, datasets = datasets))

        observe({
            teal:::srv_nested_tabs("x1", datasets = datasets, modules = x1)
            teal:::srv_nested_tabs("x2", datasets = datasets, modules = x2)
        })
    }
)

The encapsulation of teal modules is not that great and it is still heavily depends on the teal (internal!) functions to be executed. There are also other not so nice things such as:

  • ui needs to be called from within a server (probably because of DDL). It should be possible to run it inside UI for non-DDL inputs.
  • srv part needs to be executed from within a reactive context
  • the order of ui and srv functions are reversed (modules, datasets vs datasets, modules)
  • the need of dataset object - I think it could be simplified into the reactive values (or list of reactive values) - that would be a refactor on teal.modules.xyz codes

I also think that a module could return something (reactive list) to the caller such as R code, or output. We need to agree on the structure (to be the same for all the modules). I am fine to keep it as a separate feature request.

@nikolas-burkoff
Copy link
Contributor

Nice task - I agree with all of your list of other not so nice things :)

the need of dataset object - I think it could be simplified into the reactive values (or list of reactive values) - that would be a refactor on teal.modules.xyz codes

I think this feeds into making the interface clear as to what datasets must allow (i.e. it should be an object - ideally reacitve to be honest, even though filteredData isn't which can use the S3 method get_data(datasets()) and get_call(datasets()) on etc. which returns a reactive dataset/expression etc. respectively).

Then you can use FilteredData (or some other output from teal.slice if we split FilteredData) which adheres to that interface or something that doesn't even use the filter panel in the app as above.

With insightsengineering/teal.reporter#5 we should think about other arguments teal::init is going to ask of the modules

@kpagacz
Copy link
Contributor

kpagacz commented May 13, 2022

I would go as far as to throw the whole FilteredData object out the window and be content with a list of data objects (Dataset objects? maybe new quosures or something? perhaps raw data.frames would be good enough? It is just data passed from the user anyway, right? - should be possible just to relay it to a module) and reactive filter states updated by the filter panel. In a module, it should be possible to apply the filters to data to get filtered data.

This would nicely split data from filters and tie well with decoupling and different specialisations of the filter panel - a filter panel for data.frames, for mae, for Dataset, for MAEDataset, for TealData, for a dm object? There is little reason not to work in the filter panel with whatever you can think of, even in cases where the initial code for creating the object isn't available. After all, it is better to have some code than no code at all.

Once filter panel works with just data.frames and data is passed smoothly and with minimal interruption from user input to the modules, there is little stopping teal from working with just a bunch of data.frames.

The last stop is to release the pressure of defining data_extract_specs either by making module developers always provide defaults or just by resigning from extract specs altogether. transform them into nice custom js widgets and let module developers call them by input$.

Once done with all of the above, out of the blue it's possible to do stuff like this:

app <- teal::app(iris, modules = teal.modules.general::tm_variable_browser)
shiny::shinyApp(app$ui, app$server)

where the majority of the code is package names XD

On the module developer front, I would do everything to make people just write shiny modules and let them put it into teal without any additional tinkering. Ideally, at first, they should be able to:

teal::app(iris, module = list(UI = my_custom_ui, srv = my_custom_srv))

and as long as my_custom_srv didn't throw before, it should not throw after piping it into teal. Then, I would give them tools to enhance their shiny modules: here's how you can add filtering, here's how you can add source code tracking, here's how you can write teal's UI elements and how you can use them to merge data together, here's how you do reporting. All of this should be opt-in. not required.

Arguably, some of it is already in. Source code is opt in. FilteredData is mandatory, the whole teal data model is mandatory, filter panel is mandatory because FilteredData. It's possible to get the raw data which is nice but it's wrapped around FilteredData. You don't have to use data extract specs which is nice, but the server function for some reason needs to have obscure arguments. It's a bit good, but a bit bad, would be nice if it's all gravy.

@gogonzo
Copy link
Contributor

gogonzo commented May 23, 2022

I'm copying here content of the duplicated issue
I agree with @kpagacz about having teal_module with basic data object with code (quosure or reproducible expression passed directly). We also don't need a whole FIlteredData in the module - it could be optional like reporter if somebody wants module to communicate with filter-panel.


  • Modules inputs should not be complicated teal.xxx class containing data, expression and unneeded methods.
  • Modules data should be a list of (reactive) objects, so they can be used in non-teal app. Modules should have following arguments:
tm_module(
  a b c <instructions for choices>
  data <list of reactives, dm or list of static data, optionally with expr and join_keys attr>, # 
  tbl <data transformation> # we still didn't concluded about this
  ... 
  # datasets - FilterPanel-api if the communication with filteredData needed
  # reporter - if reporter-api needed 
  # metadata - if needed
)

Currently following methods from FilteredData are used in the modules:

  • $get_data(filtered = TRUE)
  • $get_data(filtered = FALSE) ❌ see occurrences. This can be fixed by including unfiltered data to the module also.
  • $datanames() - this will be the names of the list
  • $get_metadata(), $get_keys() - should we pass using ...? Please find the optimal solution.
  • $get_varlabels - this can be extracted from the raw data.frame object

Alternatively datasets can be wrapped in the simple class object with limited number of methods.


Edit: followup issues. Part of the api simplification milestone

@gogonzo
Copy link
Contributor

gogonzo commented Jun 14, 2022

Closing in favour of followup issues

@gogonzo gogonzo closed this as completed Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants