search

easystats / report

:scroll: :tada: Automated reporting of objects in R
https://easystats.github.io/report/
Other
695 stars 69 forks source link

Fix `check-test-warnings` workflow: "Global state has changed" in `test-report.brmsfit` #448

Open rempsyc opened 4 months ago

rempsyc commented 4 months ago

How to solve the "Global state has changed" warning in test-report.brmsfit? Would close #445

https://github.com/easystats/report/actions/runs/9797434007/job/27054023826?pr=447#step:5:196

── Warning (test-report.brmsfit.R:3:1): report.brms ────────────────────────────
Global state has changed:
     names(before[[4]])                | names(after[[4]])                      
[94] "PIPX_HOME"                       | "PIPX_HOME"                       [94] 
[95] "PKGCACHE_HTTP_VERSION"           | "PKGCACHE_HTTP_VERSION"           [95] 
[96] "PKGLOAD_PARENT_TEMPDIR"          | "PKGLOAD_PARENT_TEMPDIR"          [96] 
                                       - "PKG_CPPFLAGS"                    [97] 
                                       - "PKG_LIBS"                        [98] 
[97] "POWERSHELL_DISTRIBUTION_CHANNEL" | "POWERSHELL_DISTRIBUTION_CHANNEL" [99] 
[98] "PWD"                             | "PWD"                             [100]
[99] "RENV_CONFIG_REPOS_OVERRIDE"      | "RENV_CONFIG_REPOS_OVERRIDE"      [101]                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         
      names(before[[4]])        | names(after[[4]])              
[153] "TESTTHAT"                | "TESTTHAT"                [155]
[154] "TZ"                      | "TZ"                      [156]
[155] "USER"                    | "USER"                    [157]
                                - "USE_CXX17"               [158]
[156] "VCPKG_INSTALLATION_ROOT" | "VCPKG_INSTALLATION_ROOT" [159]
[157] "XDG_CONFIG_HOME"         | "XDG_CONFIG_HOME"         [160]
[158] "XDG_RUNTIME_DIR"         | "XDG_RUNTIME_DIR"         [161]                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   
before[[4]][94:99] vs after[[4]][94:101]
  PIPX_BIN_DIR"/opt/pipx_bin"
  PIPX_HOME"/opt/pipx"
  PKGCACHE_HTTP_VERSION"2"
  PKGLOAD_PARENT_TEMPDIR"/tmp/RtmpQHAqXH"
+ PKG_CPPFLAGS"  -I\"/home/runner/work/_temp/Library/Rcpp/include/\"  -I\"/home/runner/work/_temp/Library/RcppEigen/include/\"  -I\"/home/runner/work/_temp/Library/RcppEigen/include/unsupported\"  -I\"/home/runner/work/_temp/Library/BH/include\" -I\"/home/runner/work/_temp/Library/StanHeaders/include/src/\"  -I\"/home/runner/work/_temp/Library/StanHeaders/include/\"  -I\"/home/runner/work/_temp/Library/RcppParallel/include/\"  -I\"/home/runner/work/_temp/Library/rstan/include\" -DEIGEN_NO_DEBUG  -DBOOST_DISABLE_ASSERTS  -DBOOST_PENDING_INTEGER_LOG2_HPP  -DSTAN_THREADS  -DUSE_STANC3 -DSTRICT_R_HEADERS  -DBOOST_PHOENIX_NO_VARIADIC_EXPRESSION  -D_HAS_AUTO_PTR_ETC=0  -include '/home/runner/work/_temp/Library/StanHeaders/include/stan/math/prim/fun/Eigen.hpp'  -D_REENTRANT -DRCPP_PARALLEL_USE_TBB=1 "
+ PKG_LIBS" '/home/runner/work/_temp/Library/rstan/lib//libStanServices.a' -L'/home/runner/work/_temp/Library/StanHeaders/lib/' -lStanHeaders -L'/home/runner/work/_temp/Library/RcppParallel/lib/' -ltbb "
  POWERSHELL_DISTRIBUTION_CHANNEL"GitHub-Actions-ubuntu22"
  PWD"/home/runner/work/report/report"
  RENV_CONFIG_REPOS_OVERRIDE"https://packagemanager.posit.co/cran/__linux__/jammy/latest"
      before[[4]]              | after[[4]]                    
[153] "true"                   | "true"                   [155]
[154] "UTC"                    | "UTC"                    [156]
[155] "runner"                 | "runner"                 [157]
                               - "1"                      [158]
[156] "/usr/local/share/vcpkg" | "/usr/local/share/vcpkg" [159]
[157] "/home/runner/.config"   | "/home/runner/.config"   [160]
[158] "/run/user/1001"         | "/run/user/1001"         [161]
rempsyc commented 4 months ago

It seems this issue arises simply from calling the brms::brms() function. Just running the test with:

model <- suppressMessages(suppressWarnings(brms::brm(
    mpg ~ qsec + wt,data = mtcars, refresh = 0, iter = 1, seed = 333)))

Generates that "Global state has changed" warning. Any pointers here @IndrajeetPatil? Would you suggest opening an issue in the brms repo?

IndrajeetPatil commented 4 months ago

Sorry, missed this.

I see that you have already created an issue.

Let's see if there is any upstream change that takes care of this. If not, we need to think about an alternative strategy to figure out that our code is not changing global state anywhere. Because if just a namespaced function call changes state, there isn't much we can do about it.

Maybe we need to restrict this check only to tests that use easystats packages.

rempsyc commented 2 months ago

Since there were no reactions to my post for two months, I have opened a new issue on Stan Discourse: https://discourse.mc-stan.org/t/global-state-has-changed-warning/36591

rempsyc commented 2 months ago

Here's the response of Bob Carpenter, one of the core Stan developers, on Stan Discourse:

I looked up this error and it seems to arise when a test from testthat finds that global state has changed. I agree that it’s bad form to change global state in a test. Tracking this down is going to require finding where RStan or brms or something they call changes the global state.

In particular, this appears to arise from setting a few new flags (PKG_CPPFLAGS, PKG_LIBS and USE_CXX17). One way to deal with this would probably be to set them ahead of time. The other way to deal with it would be to write something that wraps the tests in something that removes warning messages.

Overall, testing for generic state changes like this is probably going to be brittle going forward if it’s not something that RStan or brms care about.

Another alternative might be to switch to the cmdstanr back end. Otherwise, someone needs to track through the code, figure out where those three flags get set, then wrap them safely so that they are guaranteed to be unset (I don’t know if R has exceptions and can support a try/catch/finally pattern).

rempsyc commented 1 month ago

Let's see if there is any upstream change that takes care of this. If not, we need to think about an alternative strategy to figure out that our code is not changing global state anywhere. Because if just a namespaced function call changes state, there isn't much we can do about it.

Maybe we need to restrict this check only to tests that use easystats packages.

Little update @IndrajeetPatil I asked what we could do if the Stan developers don't fix this and this is the reply I got:

Why not just turn off the check for warnings?

If no-warnings-in-tests is really a hard requirement for you, you might want to look at a package other than Stan, as I doubt we’ll ever get enough cycles to keep our tests warning-free.

So would you suggest following the path you suggested earlier of restricting this check to easystats packages tests then, at least for report?

IndrajeetPatil commented 1 month ago

So would you suggest following the path you suggested earlier of restricting this check to easystats packages tests then, at least for report?

Yes, absolutely! Thanks for sticking with it for so long 😅

rempsyc commented 1 month ago

ACTUALLY, we just found the source of the issue on stan. It is due of our use of testthat::set_state_inspector() in file tests/testthat/helper-state.R. Do we need this?