m7pr commented 1 month ago

Fixes https://github.com/insightsengineering/coredev-tasks/issues/479

m7pr commented 1 month ago

This issue is basically using pak::create_lockfile that creates a file of all dependencies and their source addresses for future reproducibility. This process pulls and installs all packages during the creation of the lock file.

This process also does not recognize custom R repositories in which it should look for dependencies. It is based on options("repos") which provides the list of R .repositories. That's why I needed to use devtools::session_info() to detect the source address of repositories for custom packages. I would love to get rid of this with anything that comes from base, pak or deps (a dependency of pak).

Testing

You can test the functionality with a simple teal app

app <- init(
  data = teal_data(iris = iris),
  modules = example_module(label = "example teal module")
)
if (interactive()) {
  shinyApp(app$ui, app$server)
}

You can try including some packages installed from other places than CRAN.

teal.widgets from GitHub pak::pak("insightsengineering/teal.github@*release")
teal.reporter from a custom repository install.packages('teal.reporter', repos = c('https://pharmaverse.r-universe.dev', getOption('repos')))

github-actions[bot] commented 1 month ago

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  --------------------------------------------------------------------------------------------------------------------------------------------------
R/dummy_functions.R                  36      25  30.56%   21-37, 40-47
R/get_rcode_utils.R                  31       1  96.77%   50
R/include_css_js.R                   22      17  22.73%   12-38, 76-82
R/init.R                             86      31  63.95%   108-115, 161-162, 164, 176-197, 228-229, 231
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_bookmark_manager.R         158     125  20.89%   42-43, 57-59, 70-83, 93-143, 148-149, 189, 224-301
R/module_filter_manager.R            84      19  77.38%   38-42, 157, 162-175
R/module_nested_tabs.R              161      60  62.73%   39-112, 128, 180, 202, 224, 232, 236
R/module_snapshot_manager.R         241     178  26.14%   95-107, 136-139, 143-144, 159-169, 173-188, 190-198, 205-220, 224-228, 230-236, 239-252, 255-273, 282-298, 313-336, 339-350, 353-359, 373, 394-418
R/module_tabs_with_filters.R         76      33  56.58%   33-68, 100, 116
R/module_teal_with_splash.R         114      34  70.18%   60-95, 110, 131, 197-198
R/module_teal.R                     120      81  32.50%   52-121, 158-159, 165-168, 179, 192-223
R/module_wunder_bar.R                60      39  35.00%   23-41, 55-64, 68-77
R/modules.R                         159      26  83.65%   127-130, 147-151, 206-209, 291-292, 344, 356-364, 418-421
R/reporter_previewer_module.R        19       2  89.47%   30, 34
R/show_rcode_modal.R                 24      24  0.00%    17-42
R/tdata.R                            53       1  98.11%   154
R/teal_data_module-eval_code.R       27       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                  6       0  100.00%
R/teal_reporter.R                    68       6  91.18%   69, 77, 125-126, 129, 146
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      59      12  79.66%   137-150
R/TealAppDriver.R                   324     324  0.00%    43-671
R/utils.R                           187      15  91.98%   255, 397-418
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   110-377
R/zzz.R                              12       8  33.33%   3-15
TOTAL                              2278    1123  50.70%

Diff against main

Filename           Stmts    Miss  Cover
---------------  -------  ------  -------
R/module_teal.R       +7      +2  +2.41%
R/utils.R            +14     +14  -7.44%
TOTAL                +21     +16  -0.25%

Results for commit: 7d04235e202483cbbe172772d0f81fcfbfa1edd2

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

github-actions[bot] commented 1 month ago

Unit Tests Summary

1 files 30 suites 3m 21s :stopwatch: 240 tests 240 :white_check_mark: 0 :zzz: 0 :x: 505 runs 505 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 7d04235e.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 1 month ago

Unit Test Performance Difference

Test Suite	$Status$	Time on `main`	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
shinytest2-module_bookmark_manager	💔	$24.81$	$+1.19$	$0$	$0$	$0$	$0$

Results for commit 8c459b0c95bee5a48cecadd4b8ec0969a1978c26

♻️ This comment has been updated with latest results.

m7pr commented 1 month ago

Hey @gogonzo , hey @pawelru maybe it is safer or more robust to go as @vedhav proposed, which is: app developer pre-computes the .lockfile (with pak::lockfile_create(). We can create functions that support this creation. The content of the file is put through teal::init parameter called lockfile which is a path to the pre-computed .lockfile.

This way we do not need to figure out parallel long-lasting processes.

pawelru commented 1 month ago

Hey @gogonzo , hey @pawelru maybe it is safer or more robust to go as @vedhav proposed, which is: app developer pre-computes the .lockfile (with pak::lockfile_create(). We can create functions that support this creation. The content of the file is put through teal::init parameter called lockfile which is a path to the pre-computed .lockfile.

This way we do not need to figure out parallel long-lasting processes.

There are a few things I don't like about this idea:

I don't really understand why to do this. It does not provide anything new compared to what we already have - that is: code and session info. Look at this from that angle: everything app developer provides (arg values, objects, options etc.) brings something new we don't know or it's impossible to know from our side. This we know so no need to ask IMHO.
The lock file app developer provides might be incorrect / not line line with what's expected. The reproducibility is not guaranteed anymore.
This introduces some additional (!) complexity on the way how to build the app - it could be a new argument of init() you mentioned or some fixed path or something like that. I would prefer to eliminate the complexity if possible.

vedhav commented 1 month ago

The lock file app developer provides might be incorrect / not line line with what's expected. The reproducibility is not guaranteed anymore.

It would be a hard no if we cannot guarantee reproducibility. But, I think it is possible to do this. Especially if the teal app uses packages from private repos, This information is not easy to find without the repos information.

My biggest concern is that every app run should not compute something that is static. A reliable way of finding dependency during app runtime is also challenging without the app user's repos. And we'd have to test this solution for different deployment types as well.

m7pr commented 1 month ago

Hey, I created an alternative PR https://github.com/insightsengineering/teal/pull/1232 where pak::lockfile_create() is run in a parallel process to the shiny session using futures + shiny::ExtendedTask()

m7pr commented 1 month ago

Also summarized issues I see with our own support of the lockfile creation

https://github.com/insightsengineering/teal/pull/1232#issuecomment-2126867283

m7pr commented 1 month ago

It would be a hard no if we cannot guarantee reproducibility.

It's always hard to guarantee 100% reproducibility, even with pak, which states this in their own documentation https://github.com/insightsengineering/nestdevs-tasks/issues/55#issuecomment-2125661627

We do not guarantee reproducibility, we support the reproducibility!

gogonzo commented 1 month ago

We discussed a matter with @pawelru and support idea of having an automated way of creating pak lockfile in teal. There are several findings:

We don't need to show lockfile to copy-paste, but rather to download.
Given above, we propose that file is kept in the tempdir (where it cound be downloaded from) instead of readLines and passing character.
Process should be triggered in srv_teal as some modules can have some require calls which loads extra packages.
Because some packages could be loaded on require by some modules, it means that session might constantly change (more and more packages will be added), we recommend then a mechanism to update a lockfile when needed.

m7pr commented 1 month ago

@gogonzo 1,2,3 this is the part that looks like we already figured out in the comments. I think the biggest question is if we really use future and future::plan in our codebase and whether we use pak::lockfile_create or renv::snapshot() to create the lockfile. I don't think we have a way currently to create a lockfile with pak::lockfile_create and I personally think it takes too much time to create so I frankly think it's not that important to discuss whether it should be a file to be downloaded or a text that you copy-paste as this is minor in comparison to what is the current bottleneck

gogonzo commented 1 month ago

if we really use future and future::plan

I think it is beneficial to make a lockfile asynchronously.

whether we use pak::lockfile_create or renv::snapshot()

I guess pak is better in a long term - https://github.com/rstudio/renv/issues/1210

m7pr commented 1 month ago

I see that @pawelru provided his thoughts about on how to make use of pak https://github.com/insightsengineering/teal/pull/1232#discussion_r1611901282

m7pr commented 1 month ago

I guess pak is better in a long term - https://github.com/rstudio/renv/issues/1210

My personal opinion is that pak is better for installation, when a lockfile exists, but renv is better for lockfile creation. pak works on lockfiles or projects with DESCRIPTION file.

gogonzo commented 1 month ago

I guess pak is better in a long term - rstudio/renv#1210

My personal opinion is that pak is better for installation, when a lockfile exists, but renv is better for lockfile creation. pak works on lockfiles or projects with DESCRIPTION file.

My opinion is based on naive insights so I'm not pushing towards anything.

m7pr commented 1 month ago

Because some packages could be loaded on require by some modules, it means that session might constantly change (more and more packages will be added), we recommend then a mechanism to update a lockfile when needed.

This is not needed as renv::snapshot() for the default type = "implicit" takes all packages from the files in current project directory. For the type = "explicit", it takes only the specs from DESCRIPTION file (the same as pak::lockfile_create() does) so it's not working on an actual R session but on a list of files, that do not change.

m7pr commented 1 month ago

Closing in favour of https://github.com/insightsengineering/teal/pull/1232

insightsengineering / teal

479 add `pak::create_lockfile` for `.lock` file download #1224

Testing

Code Coverage Summary

Diff against main

Unit Tests Summary

Unit Test Performance Difference