donyunardi
commented
3 weeks ago So this fails locally too when I run verdepcheck::max_deps_check(".")
Closed donyunardi closed 3 weeks ago
donyunardi
commented
3 weeks ago So this fails locally too when I run verdepcheck::max_deps_check(".")
m7pr
commented
3 weeks ago The difference between those strategies are teal dependency versions
max strategy uses
mirari 1.2.0.9025 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058830842#step:9:186renv 1.0.9.9000 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058830842#step:9:206callr 3.7.6 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058830842#step:9:87min_cohort uses
mirari 1.2.0 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058831013#step:9:163renv 1.0.9 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058831013#step:9:113callr 3.7.6 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058831013#step:9:126min_isolated uses
mirari 1.1.1 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058830714#step:9:151renv 1.0.7 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058830714#step:9:132callr 3.7.3 https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058830714#step:9:109When it comes to renv, 1.0.9.9000 was never release on CRAN. Only 1.0.7, 1.0.8 and 1.0.9 and now 1.0.10 (since 3 days) https://cran.r-project.org/src/contrib/Archive/renv/ were released.
When it comes to mirari 1.2.0 is the highest possible CRAN release https://cran.r-project.org/web/packages/mirai/index.html and 1.2.0.90** is the GitHub version. https://github.com/shikokuchuo/mirai/blob/main/DESCRIPTION#L4
I don't think we should pay attention to max dependency test as it takes development versions of packages. For example there is no mirai 1.2.0.9025 anymore as GitHub today states the latest version is 1.2.0.9030. Frankly the pace of changes in dependent packages on their GitHub repositories is that high that we are not able to stay up to date with all development versions. And there is no point in my opinion as they keep changing every day. IMHO we should stick to the released versions on CRAN. the max dependency strategy is only a nice to have, but not a must IMHO.
m7pr
commented
3 weeks ago Test do not fail locally under
> library(mirai);library(renv);library(callr)
> packageVersion('mirai');packageVersion('renv');packageVersion('callr')
[1] ‘1.2.0.9030’
[1] ‘1.0.7’
[1] ‘3.7.6’but fails for
> library(mirai);library(renv);library(callr)
> packageVersion('mirai');packageVersion('renv');packageVersion('callr')
[1] ‘1.2.0.9030’
[1] ‘1.0.10’
[1] ‘3.7.6’And I do see
[WARN] 2024-10-08 11:55:37.7643 pid:21244 token:[76c0d459] teal Lockfile creation failed.
In the logs for a simple teal app
app <- teal::init(
data = teal.data::teal_data(iris = iris),
modules = modules(example_module())
)
shinyApp(app$ui, app$server)It's the newest renv that breaks
> getwd()
[1] "C:/Rprojects/teal"
> renv::snapshot(
+ lockfile = 'teal_app.lock',
+ prompt = FALSE,
+ force = TRUE,
+ type = renv::settings$snapshot.type()
+ )
Error in if (package == "R") return() :
missing value where TRUE/FALSE needed
In addition: Warning message:
In node[[2L]] : subscript out of bounds
> packageVersion('renv')
[1] ‘1.0.10’For renv 1.0.9 it works fine
> packageVersion('renv')
[1] ‘1.0.9’
> renv::snapshot(
+ lockfile = 'teal_app.lock',
+ prompt = FALSE,
+ force = TRUE,
+ type = renv::settings$snapshot.type()
+ )
- Lockfile written to "teal_app.lock".Regular
renv::snapshot(
prompt = FALSE,
force = TRUE
)works in teal.osprey and in teal.modules.general but does not work in teal.
What is different between teal and teal.osprey or teal.modules.general is that renv::dependencies() returns more columns for teal than for other packages.
In tmg
In teal
m7pr
commented
3 weeks ago And there is some traceback entry for teal.slice
m7pr
commented
3 weeks ago It is something broken in our tests/ directory that prevents renv to make a snapshot. When this directory is deleted, the snapshot is performed regurarly.
> renv::snapshot()
Error in if (package == "R") return() :
missing value where TRUE/FALSE needed
In addition: Warning message:
In node[[2L]] : subscript out of bounds
> unlink('tests/', recursive = TRUE)
> renv::snapshot()
The following package(s) were installed from an unknown source:
- teal.logger [0.2.0.9008]
- teal.widgets [0.4.2.9020]
renv may be unable to restore these packages in the future.
Consider reinstalling these packages from a known source (e.g. CRAN).
Do you want to proceed? [Y/n]: nI managed to figure out which file causes the renv to break - it's tests/testthat/test-modules.R
> renv::snapshot()
Error in if (package == "R") return() :
missing value where TRUE/FALSE needed
In addition: Warning message:
In node[[2L]] : subscript out of bounds
> file.remove('tests/testthat/test-modules.R')
[1] TRUE
> renv::snapshot()
The following package(s) were installed from an unknown source:
- teal.logger [0.2.0.9008]
- teal.widgets [0.4.2.9020]
renv may be unable to restore these packages in the future.
Consider reinstalling these packages from a known source (e.g. CRAN).
Do you want to proceed? [Y/n]: nLong story short is that, for the new renv (1.1.0) that was released 3 days ago on CRAN, it is the function module() that, when used in tests/ without any extra parameters, breaks renv::snapshot() creation, which is used in the lockfile process. So, the lockfile test breaks because we cannot create the lockfile.
This is bizzare and some edge case of renv. To test this just run renv::snapshot() on main, and on below 2 PRs.
I provided 2 PR to solve this, but this is not an actual solution.
module() without any extra parameter
https://github.com/insightsengineering/teal/pull/1364module() so that they use module(label = "module")
https://github.com/insightsengineering/teal/pull/1365
averissimo
commented
3 weeks ago Thanks for this @m7pr I was flabergasted with renv failing on my teal package install.
I got it to snapshot when restarting the renv library from scratch.
It's weird that module is creating problems as it doesn't seem to be overwritten by testthat, renv and others that would be obvious
edit:
averissimo
commented
3 weeks ago I don't think we should pay attention to max dependency test as it takes development versions of packages. For example there is no mirai 1.2.0.9025 anymore as GitHub today states the latest version is 1.2.0.9030. Frankly the pace of changes in dependent packages on their GitHub repositories is that high that we are not able to stay up to date with all development versions. And there is no point in my opinion as they keep changing every day. IMHO we should stick to the released versions on CRAN. the max dependency strategy is only a nice to have, but not a must IMHO.
I agree that max strategy should not be a top priority, as I've seen many problems being resolved on their own.
On the other hand, we already have contributed to identify 3 or 4 upstream PRs (and in at least 2 occasions fix) that would've broken one of i. Package's functionality or; ii. tests
I think we should at least investigate the cause of the error and decide the priority case to case.
m7pr
commented
3 weeks ago module does not need to be renv function, it can be some whitelist keyword that they omit.
however renv uses {modules} package and this package has modules::module function.
When you seek for module phrase in renv it looks like they do something with it
https://github.com/search?q=repo%3Arstudio%2Frenv%20module&type=code
Maybe they have special treatment, as this is reserved for reticulate/python modules?
m7pr
commented
3 weeks ago I raised the issue on rstudio/renv but they have 160 opened issues : P https://github.com/rstudio/renv/issues/2007
donyunardi
commented
3 weeks ago This issue is starting to cause problem in our PR pipeline too.
Look at #1353 R CMD Check pipeline: https://github.com/insightsengineering/teal/actions/runs/11232478215/job/31224159187?pr=1353
m7pr
commented
3 weeks ago yes, becuase renv was published 3 days ago on CRAN and this new version has this bug
m7pr
commented
3 weeks ago @pawelru @donyunardi @gogonzo @averissimo @vedhav are you OK including this temporary change for now https://github.com/insightsengineering/teal/pull/1365 so it unblocks our CHECK and SCHEDULED job? Without this, we no longer can merge any PR. Each PR is now based on new version of renv that fails because we have an exported module function that is used in tests without parameters. When we pass default parameter, the test pass as renv::snapshot is able to be created, hence the test for lockfile creation passes.
Other alternative is to remove the test for lockfile temporarily.
pawelru
commented
3 weeks ago My concern is that that change won't be temporary and we won't fix it. Look, if you merge it as is then it would be difficult to isolate items to revert (assuming a fixed version of renv would be available). I would rather do skip() instead and provide informative message linking to here and renv issue. This way we would be able to isolate and revert in the future.
donyunardi
commented
3 weeks ago It looks like the bug has been addressed.
Thanks for the bug report, and especially for taking the time to provide a reproducible example! This should now be fixed up in the development version of renv; I'll try to get a patch release onto CRAN soon.
Let's skip the tests for now and revert it once the patch release is on CRAN.
m7pr
commented
3 weeks ago If this is already fixed in the upstream package (sick!), then let's put a requirement on a dev version of this package in DESCRIPTION
renv 1.0.10.9000
m7pr
commented
3 weeks ago @pawelru I made a PR where we increase the min version of renv https://github.com/insightsengineering/teal/pull/1372 and extend lookup_refs for the time it is dev on GitHub but not yet on CRAN
https://github.com/insightsengineering/teal/actions/runs/11172110749/job/31058830842
This is similar fail with our daily integration test and we have already increased the timeout here to 500 seconds.
To test this, I'm going to see if increasing the timeout will help resolve this.