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

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

Open
rempsyc opened this issue Jul 4, 2024 · 7 comments

Comments

@rempsyc
Copy link
Member

rempsyc commented Jul 4, 2024

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
Copy link
Member Author

rempsyc commented Jul 4, 2024

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
Copy link
Member

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
Copy link
Member Author

rempsyc commented Sep 13, 2024

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
Copy link
Member Author

rempsyc commented Sep 21, 2024

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
Copy link
Member Author

rempsyc commented Oct 11, 2024

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
Copy link
Member

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
Copy link
Member Author

rempsyc commented Oct 14, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants