Qenv use default environment not global environment by llrs-roche · Pull Request #303 · pharmaverse/teal.code

llrs-roche · 2026-11-10T14:43:05Z

Pull Request

Fixes #272

Replaces the base environment to an "empty", only those packages loaded by default. These are found via a new helper default_env() added. Note that the private function is used also on tests, but I am not sure if there is any work around that avoids duplicating it.

Check out if .eval_code() should be amended too, it is the only place where if I replace it something starts to fail, but I don't understand the evaluate framework enough to know if this is a mistake or not.

Also check the tests added. They should be skipped online but run locally. Checking with R CMD check --as-cran locally didn't trigger any Note about undeclared package.

Last not not least. This might create havoc across modules and apps, if we forgot to add a dependency inside qenv, once this is merged it will reveal itself.

To verify the fix run these lines that the last one should fail:

library("teal.code")
library("teal.modules.clinical")
within(qenv(), {ADSL <- tmc_ex_adsl})

In relation to #201, we might want to change some environment to the environment of the object before the evaluation.

github-actions · 2026-11-10T14:47:01Z

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -------------
R/qenv-c.R                          55       0  100.00%
R/qenv-class.R                      13       0  100.00%
R/qenv-concat.R                      7       0  100.00%
R/qenv-constructor.R                 1       0  100.00%
R/qenv-errors.R                      4       4  0.00%    6-9
R/qenv-eval_code.R                  63       1  98.41%   40
R/qenv-extract.R                    30       0  100.00%
R/qenv-get_code.R                   24       0  100.00%
R/qenv-get_env.R                     3       1  66.67%   27
R/qenv-get_messages.r                5       0  100.00%
R/qenv-get_outputs.R                 6       0  100.00%
R/qenv-get_var.R                    13       1  92.31%   13
R/qenv-get_warnings.R                5       0  100.00%
R/qenv-join.R                        1       1  0.00%    13
R/qenv-length.R                      2       1  50.00%   2
R/qenv-show.R                       29      29  0.00%    19-50
R/qenv-within.R                      8       0  100.00%
R/utils-get_code_dependency.R      258       3  98.84%   160, 258, 326
R/utils.R                           55       1  98.18%   108
TOTAL                              582      42  92.78%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  -------
R/utils.R       +13      +1  -1.82%
TOTAL           +13      +1  -0.01%

Results for commit: 8ce829c

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2026-11-10T14:47:10Z

Unit Test Performance Difference

Additional test case details

Test Suite	$Status$	Time on `main`	$±Time$	Test Case
qenv_constructor	👶		$+0.04$	parent_of_qenv_environment_is_the_parent_of_.GlobalEnv_but_doesn_t_mess_up_with_library_calls_inside_qenv
qenv_constructor	👶		$+0.00$	parent_of_qenv_environment_is_the_parent_of_.GlobalEnv_but_doesn_t_mess_up_with_library_calls_outside_qenv

Results for commit d536cc5

♻️ This comment has been updated with latest results.

github-actions · 2026-11-11T09:10:08Z

Unit Tests Summary

1 files 14 suites 4s ⏱️
192 tests 186 ✅ 5 💤 1 ❌
277 runs 271 ✅ 5 💤 1 ❌

For more details on these failures, see this check.

Results for commit 99a0f69.

♻️ This comment has been updated with latest results.

averissimo

Left a comment thread so we can discuss this

averissimo · 2026-11-11T09:09:40Z

+      if (!identical(parent.env(object@.xData), .GlobalEnv)) {
+        parent.env(object@.xData) <- .GlobalEnv


Why are you making the parent .GlobalEnv? shouldn't it be default_env, otherwise it will reset the purpose for sequential calls of within/eval_code

note: without running, I'm guessing the suggestion bellow will break the calls to library loaded during qenv() execution.

Suggested change

if (!identical(parent.env(object@.xData), .GlobalEnv)) {

parent.env(object@.xData) <- .GlobalEnv

if (!identical(parent.env(object@.xData), default_env())) {

parent.env(object@.xData) <- default_env()

Because this is injected with evaluate that uses the environment of the qenv and replacing it makes multiple tests to fail. But maybe instead of .GlobalEnv should be parent.env() to resolve on the right environment without relying on .GlobalEnv.

I have investigated a bit more this and it is still unclear to me the process. However here is the explanation I got so far by getting to print the name of the environments available before the injection, before the the assignment of the environment and after it as it is currently:

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 13 ]R_GlobalEnv, package:teal.code, devtools_shims, package:testthat, tools:rstudio Before evaluate , teal.code, imports:teal.code, base, R_GlobalEnv, package:teal.code, devtools_shims, package:testthat, tools:rstudio Inside evaluate inject , teal.code, imports:teal.code, base, R_GlobalEnv, package:teal.code, devtools_shims, package:testthat, tools:rstudio After messing with envs [ FAIL 0 | WARN 0 | SKIP 0 | PASS 14 ]R_GlobalEnv, package:teal.data, package:teal.code, devtools_shims, package:testthat, tools:rstudio Before evaluate , teal.code, imports:teal.code, base, R_GlobalEnv, package:dplyr, package:teal.data, package:teal.code, devtools_shims, package:testthat, tools:rstudio Inside evaluate inject , teal.code, imports:teal.code, base, R_GlobalEnv, package:dplyr, package:teal.data, package:teal.code, devtools_shims, package:testthat, tools:rstudio After messing with envs [ FAIL 1 | WARN 0 | SKIP 0 | PASS 14 ]R_GlobalEnv, package:dplyr, package:teal.data, package:teal.code, devtools_shims, package:testthat, tools:rstudio Before evaluate , teal.code, imports:teal.code, base, R_GlobalEnv, package:dplyr, package:teal.data, package:teal.code, devtools_shims, package:testthat, tools:rstudio Inside evaluate inject , teal.code, imports:teal.code, base, R_GlobalEnv, package:dplyr, package:teal.data, package:teal.code, devtools_shims, package:testthat, tools:rstudio After messing with envs [ FAIL 1 | WARN 0 | SKIP 0 | PASS 15 ]R_GlobalEnv, package:dplyr, package:teal.data, package:teal.code, devtools_shims, package:testthat, tools:rstudio Before evaluate [ FAIL 2 | WARN 0 | SKIP 0 | PASS 15 ]R_GlobalEnv, package:dplyr, package:teal.data, package:teal.code, devtools_shims, package:testthat, tools:rstudio

Note that initially there is R_GlobalEnv and no package between it an package:teal.code
Then teal.data is loaded right in between them because there is a test where inside the qenv the teal.data is called.
Then the dplyr package is loaded (I added the code in the discussion as a tests) and added even if it is loaded outside the qenv environment.
Last the teal.data environment is added after dplyr which makes the within( qenv(), {library(teal.data); t <- tibble()}) pass when it shouldn't!

I pushed the latest changes to verify that on the CI we see the same (without this debugging method).

I don't think we can fix all the cases without some major changes on teal.code as discussed below but I think this PR is another step in the right direction.

Current solution only solves it until there is a library() call on qenv(), which should not be the case in most scenarios, however our examples perform just this.

After a library call the parent env changes to global and effectively stays the same.

qenv() |> as.environment() |> rlang::env_parent() #> <environment: package:stats> #> attr(,"name") #> [1] "package:stats" #> attr(,"path") #> [1] "/usr/lib/R/library/stats" library(dplyr) (qenv() |> within({ 1+1 %>% sum() })) #> <qenv.error: could not find function "%>%" #> when evaluating qenv code: #> 1 + 1 %>% sum()> qenv() |> within({ library(teal.code) 1+1 %>% sum() }) |> as.environment() |> rlang::env_parent() #> <environment: R_GlobalEnv>

I'm torn here, this is a reproducibility package that is affected by the outside. This PR partially solves it, but may produce different experiences for 2 users.

I won't block this PR, but if we move on to merge this I would like to see a way to help users understand the problem.

If we analyse the globals after eval_code is run, we can:

throw error as certain object does not exist

(also) evaluate the globals of expression after error to try and inform user of how to solve it

caveat is that we need to evaluate whole code

library(globals) q <- qenv() |> within({ library(dplyr) 1 %>% sum() }) g <- globals::globalsOf(str2expression(teal.code::get_code(q)), method = "ordered") cleanup(g) #> Globals [0] library(dplyr) q2 <- qenv() |> within({ 1 %>% sum() }) g2 <- globals::globalsOf(str2expression(teal.code::get_code(q2)), method = "ordered") cleanup(g2) #> Globals [1] #> - ‘%>%’: <function> [dplyr]

(note: this runs with main branch to allow code execution anyway)

averissimo · 2026-11-11T09:26:40Z


      # insert (and overwrite) objects from y to x
-      x@.xData <- rlang::env_clone(x@.xData, parent = parent.env(.GlobalEnv))
+      x@.xData <- rlang::env_clone(x@.xData, parent = default_env())


[DISCUSSION]

I'm not sure we sure enforce this initial restriction, for 2 reasons:

Creating bottlenecks for app development

Not all developers understand reproducibility and your example not working will create some confusion as to why

When loading packages inside qenv we cannot enforce that it's loaded only on top of default_env(), see example below

this is the current reason why we always place the parent as the parent of the global environment

managing this would add a lot of complexity *IF* it can even be done

pkgload::load_all(".", export_all = FALSE) pkgload::load_all("./../teal.data", export_all = FALSE) library(dplyr) qenv() |> within({ library(teal.data) t <- tibble() }) #> FAILS q <- within(qenv(), { library(teal.data) ADSL <- rADSL }) q |> within(t <- tibble()) #> Succeeds

Alternative solution that I pursued some time ago might be to do some code analysis and warn against non-reproducible code

I believe it was using https://globals.futureverse.org/ to find globals in code

Yes, I think this could be problematic for users but it would help to detect issues on code where we assume some package are loaded while they might not be loaded.

I think this talks more about the PR solving this problem than not being possible to do it. I think the few .GlobalEnv might cause this to work.

using globals might be a good solution, at least the error/warning could become more informative.

…code into 272_valid_reproducible

… call

averissimo · 2026-11-13T11:44:57Z

+    expect_s3_class(within(qenv(), {
+      library(teal.data)
+      t <- tibble()
+    }), "qenv.error")


This fails on the CI because teal.data is not installed. I used it on snippets for ease of use. Here we should use one of the imports of the package

llrs-roche added 3 commits November 10, 2026 15:25

Add function to find default environment (with clean start)

786eeeb

Replace the environmnets for the default one when it makes sense

102fdaf

Add tests

49b83d1

llrs-roche requested a review from averissimo November 10, 2026 14:44

[skip style] [skip vbump] Restyle files

8ce829c

averissimo self-assigned this Nov 11, 2026

averissimo reviewed Nov 11, 2026

View reviewed changes

llrs-roche added 7 commits November 12, 2026 11:47

Clonse without setting parent environment

7963ad7

Add additional tests

031ac98

Remove some duplicate tests and avoid lintr issues

a8a858f

Merge branch '272_valid_reproducible' of github.com:pharmaverse/teal.…

b2b5f5d

…code into 272_valid_reproducible

Remove more usages or default_env as they might mess with the library…

016e3be

… call

Fix typo

62197da

Clean up

99a0f69

averissimo reviewed Nov 13, 2026

View reviewed changes

		if (!identical(parent.env(object@.xData), .GlobalEnv)) {
		parent.env(object@.xData) <- .GlobalEnv

Conversation

llrs-roche commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Uh oh!

github-actions Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Diff against main

Uh oh!

github-actions Bot commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Performance Difference

Uh oh!

github-actions Bot commented Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Tests Summary

Uh oh!

averissimo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llrs-roche Nov 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

averissimo Nov 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

llrs-roche commented Nov 10, 2026 •

edited

Loading

github-actions Bot commented Nov 10, 2026 •

edited

Loading

github-actions Bot commented Nov 10, 2026 •

edited

Loading

github-actions Bot commented Nov 11, 2026 •

edited

Loading

llrs-roche Nov 11, 2026 •

edited

Loading

averissimo Nov 13, 2026 •

edited

Loading