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

disable lockfile generation in callr, testthat::tests and R CMD CHECK #1346

Merged
merged 8 commits into from
Sep 19, 2024

Conversation

m7pr
Copy link
Contributor

@m7pr m7pr commented Sep 17, 2024

Part of #1263 and closes #1276

Local results of shinytest2 tests

══ Results ══════════════════════════════════════════════════════════════════════════
Duration: 540.8 s

── Skipped tests (6) ────────────────────────────────────────────────────────────────
• need a fix in a .slicesGlobal (1): test-module_teal.R:1139:11
• todo (5): test-module_teal.R:1404:7, test-module_teal.R:1411:5,
  test-module_teal.R:1414:5, test-module_teal.R:1673:5, test-module_teal.R:1731:5

[ FAIL 0 | WARN 0 | SKIP 6 | PASS 515 ]

@m7pr m7pr added the core label Sep 17, 2024
@m7pr m7pr mentioned this pull request Sep 17, 2024
@gogonzo
Copy link
Contributor

gogonzo commented Sep 17, 2024

It does seem to work. I've checked devtools::run_examples and pkgdown::build_site and I've not seen any lockfile created. Can you confirm that https://github.com/insightsengineering/tlg-catalog/ rendering doesn't create any lockfile?

@m7pr
Copy link
Contributor Author

m7pr commented Sep 17, 2024

Can you confirm that https://github.com/insightsengineering/tlg-catalog/ rendering doesn't create any lockfile?

Will try to build in a sec

It does seem to work. I've checked devtools::run_examples and pkgdown::build_site and I've not seen any lockfile created.

Does devtools::run_examples(fresh = TRUE) not show any lockfiles because we do have this if statement around all our apps in examples?

if (interactive()) {
  shinyApp(ui, server)
}

When I run devtools::run_examples(fresh = FALSE) it runs in my interactive() session and I do see lockfile being created

@m7pr
Copy link
Contributor Author

m7pr commented Sep 17, 2024

Sorry for the delay, I needed to install Quarto, webshot2, shinylive, quatro and render the book. Trying to render it locally

@gogonzo
Copy link
Contributor

gogonzo commented Sep 17, 2024

When I run devtools::run_examples(fresh = FALSE) it runs in my interactive() session and I do see lockfile being created

Then options(teal.lockfine.enable) is not set properly I guess. devtools::run_examples(fresh = TRUE) runs callr::r - theoretically lockfile shouldn't be triggered.

edit: sorry you run with fresh = FALSE so probably teal is just run outside of the callr, which is fine. But when fresh = TRUE the lockfile is still created and it is wrong I guess.

@m7pr
Copy link
Contributor Author

m7pr commented Sep 17, 2024

For devtools::run_examples(fresh = TRUE) I don't see lockfile being created (or maybe it happens too quickly and then it immediately gets deleted), even on 479_mirai_lockfile@main. Let's talk about it tomorrow for a quick moment after the daily.

I hope I will get quarto book rendered sorted out during this time. For now I am experiencing an issue with the installation of GitHub packages.

@m7pr
Copy link
Contributor Author

m7pr commented Sep 18, 2024

Ok, for devtools::run_examples(fresh = TRUE) we had a discussion in which we agreed that if we run it, and have all examples covered in if (interactive() ) then no teal session will be started, hence to teal lockfile generated.

@m7pr
Copy link
Contributor Author

m7pr commented Sep 18, 2024

Hey @pawelru and @cicdguy

In this PR we try to disable one of the functionalities of teal, which is lockfile creation. We listed out scenarios in which we would like to disable the option #1276 (comment)

We are trying to figure out what elements are covered in tlg-catalog Quarto book creation so that at some parts this feature can be disabled as well. I am trying to figure out the flow of Quarto (that uses webR and uses shinylive) for tlg-catalog rendering. Would you be able to help me explain the process?

Based on the example:

here is what I understand is happening in tlg-catalog.

Quarto is used to render the whole book/website.
Each page (in this case efficacy -> fstg01) contains of few tabs.

There are tabs that just have data preparations and plot generation - those have a preview section and webR section.
Preview is responsible for showing the code and the output, webR is responsible for running R live in browser.
Here no teal apps are created.
image

There is also a tab called teal app - this have a preview section and shinylive section.
Preview is responsible for showing the code and the screenshot of the app.
shinylive section is responsible for running teal app in it.
teal app is not started before you hit shinylive section, however in the preview section there is already some screenshot of the app.

preview
image

shinylive
image

So for the app that is started in shinylive section, we would like to show teal lockfile. However for the moment when the Quarto page is generated/rendered and the screenshot for the preview section is made, we would not like to create a lockfile. I am trying to figure out how the screenshot is made, and whether we needed to run the teal app to create the screenshot. I understand that screenshot is made with webshot2::webshot(app_url) but someone needed to somehow somewhere run the app so that app_url is exposed. I assume teal app is run during Quarto rendering.

If the teal app is run inside Quarto process just to create the screenshot, we can easily detect that Quarto is running by checking Sys.getenv("QUARTO_PROJECT_ROOT") variable and disabling lockfile creation of this is set. I am just trying to verify if I understand things right.

@pawelru
Copy link
Contributor

pawelru commented Sep 18, 2024

I share the same understanding. And yes - the app needs to be run to get the snapshot. Unfortunately I don't know ow the details of this. This is also not well documented and I even created a ticket to enhance the docs in the knitr repo

@m7pr
Copy link
Contributor Author

m7pr commented Sep 18, 2024

Ok, I think I was able to understand the process based on a small test Quarto file with teal app.

Quarto renders the file and starts teal app, once the chunk is finished it is possible to create a snapshot/webshot of the app that was run. You can see in the logs that the whole app was run.

So to omit teal lockfile creation during tlg-creation we need to disable it based on a Quarto running flag
Sys.getenv("QUARTO_PROJECT_ROOT")

EDIT THIS MESSAGE TO COPY THE CONTENT OF THE BELOW FILE


title: Test


knitr::opts_template$set(
  app = list(
    webshot = "webshot2",
    screenshot.force = TRUE,
    screenshot.opts = list(delay = 5),
    dev = "png",
    fig.width = 15,
    cache = FALSE,
    message = FALSE,
    R.options = list(
      teal.log_level = "TRACE"
    )
  )
)
#| echo: true
#| output: asis
cat(Sys.getenv("QUARTO_PROJECT_ROOT"))

teal App

library(teal.modules.clinical)

## Data reproducible code
data <- teal_data()
data <- within(data, {
  ADSL <- random.cdisc.data::cadsl
  ADRS <- random.cdisc.data::cadrs
})
datanames <- c("ADSL", "ADRS")
datanames(data) <- datanames
join_keys(data) <- default_cdisc_join_keys[datanames]

arm_ref_comp <- list(
  ARM = list(
    ref = "B: Placebo",
    comp = c("A: Drug X", "C: Combination")
  ),
  ARMCD = list(
    ref = "ARM B",
    comp = c("ARM A", "ARM C")
  )
)

## Reusable Configuration For Modules
ADSL <- data[["ADSL"]]
ADRS <- data[["ADRS"]]

## Setup App
app <- init(
  data = data,
  modules = modules(
    tm_g_forest_rsp(
      label = "Forest Response",
      dataname = "ADRS",
      arm_var = choices_selected(
        variable_choices(ADSL, c("ARM", "ARMCD")),
        "ARMCD"
      ),
      arm_ref_comp = arm_ref_comp,
      paramcd = choices_selected(
        value_choices(ADRS, "PARAMCD", "PARAM"),
        "BESRSPI"
      ),
      subgroup_var = choices_selected(
        variable_choices(ADSL, names(ADSL)),
        c("BMRKR2", "SEX")
      ),
      strata_var = choices_selected(
        variable_choices(ADSL, c("STRATA1", "STRATA2")),
        "STRATA2"
      ),
      plot_height = c(600L, 200L, 2000L),
      plot_width = c(1100L, 200L, 2000L)
    )
  )
)

shinyApp(app$ui, app$server)

@m7pr
Copy link
Contributor Author

m7pr commented Sep 18, 2024

Yeah, for Quarto you can see it was possible to disable lockfile with the last commit d54cb80

Without the flag it was running the lockfile

on_main

With the flag it is not running the lockfile

on_feature_branch_based_on_quarto_flag

R/zzz.R Show resolved Hide resolved
R/zzz.R Show resolved Hide resolved
R/zzz.R Outdated
lockfile_flag <-
!(
identical(Sys.getenv("CALLR_IS_RUNNING"), "true") || # inside callr process
identical(Sys.getenv("TESTTHAT"), "true") || # inside devtools::test
Copy link
Contributor

Choose a reason for hiding this comment

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

a question
what would happen if we do load_all() and then test()?
Do I understand correctly that the option would be set to true and then lockfile created inside the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check, good question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think that Sys.getenv("TESTTHAT") flag is set after the package is loaded during tests, and the initial value of getOption('teal.lockfile.enable') is set to TRUE during tests. Not as I would expect it to work

I added this snippet to check the behavior of this flag during checks.

cat('\n teal.lockfile.enable: ', getOption('teal.lockfile.enable'), '\n')

This made me think we actually need to disable the creation of the lockfile inside srv_teal_lockfile, if Sys.getenv("TESTTHAT") gets set later after the package is loaded

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sys.getenv("TESTTHAT") is set at testthat::test_that, not at devtools::test()

image

Copy link
Contributor

Choose a reason for hiding this comment

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

This made me think we actually need to disable the creation of the lockfile inside srv_teal_lockfile, if Sys.getenv("TESTTHAT") gets set later after the package is loaded

I got a similar idea. And I was thinking about this particular example in my previous comment here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this commit 1862043 I moved the functionality inside srv_teal_lockfile so the checks are made on lockfile execution.
If the option was set before the lockfile was called, then the option is superior to the circumstances check

@m7pr
Copy link
Contributor Author

m7pr commented Sep 18, 2024

Current state of test

══ Results ══════════════════════════════════════════════════════════════════════════════
Duration: 564.7 s

── Skipped tests (6) ────────────────────────────────────────────────────────────────────
• need a fix in a .slicesGlobal (1): test-module_teal.R:1139:11
• todo (5): test-module_teal.R:1404:7, test-module_teal.R:1411:5,
  test-module_teal.R:1414:5, test-module_teal.R:1673:5, test-module_teal.R:1731:5

[ FAIL 0 | WARN 0 | SKIP 6 | PASS 515 ]

@gogonzo
Copy link
Contributor

gogonzo commented Sep 19, 2024

I've tested using Rscript -e "Sys.setenv(TESTING_DEPTH=5)" and lockfile is not created in both mirai PR and this PR:

@Mirai @this
devtools::run_examples()
devtools::build_vignettes()
devtools::test()
devtools::check()
pkgdown::build_site()
roxy.shinylive of tmg (install docs render and view)

I didn't check tlg-catalog.

NOTE: In roxy.shinylive apps lockfile download is hidden, I assume renv is not available in shinylive (maybe is not compiled?).

@m7pr
Copy link
Contributor Author

m7pr commented Sep 19, 2024

I've tested manually Quarto and callr and it is improved with this PR

@Mirai @this
quarto render
callr

Looks like we are good to go

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

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

Let's continue on the feature branch

@m7pr
Copy link
Contributor Author

m7pr commented Sep 19, 2024

Alrighty, merging into feature branch

@m7pr m7pr merged commit 761dd68 into 479_mirai_lockfile@main Sep 19, 2024
1 check passed
@m7pr m7pr deleted the disable@479_mirai_lockfile@main branch September 19, 2024 08:04
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
@gogonzo gogonzo self-assigned this Oct 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants