Skip to content

Commit

Permalink
check StanHeaders version number
Browse files Browse the repository at this point in the history
  • Loading branch information
@bgoodri
bgoodri committed Oct 15, 2023
1 parent 2cd45b0 commit 350ef0d
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 6 deletions.
2 changes: 1 addition & 1 deletion rstan/rstan/DESCRIPTION
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Package: rstan
Type: Package
Title: R Interface to Stan
Version: 2.32.2
Version: 2.32.3
Authors@R: c(person("Jiqiang", "Guo", email = "[email protected]", role = "aut"),
person("Jonah", "Gabry", email = "[email protected]", role = "aut"),
person("Ben", "Goodrich", email = "[email protected]", role = c("cre", "aut")),
Expand Down
9 changes: 4 additions & 5 deletions rstan/rstan/R/zzz.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,11 @@ OUT <- 0
if (requireNamespace("V8", quietly = TRUE)) {
assign("stanc_ctx", V8::v8(), envir = topenv())
} else assign("stanc_ctx", QuickJSR::JSContext$new(stack_size = 4 * 1024 * 1024), envir = topenv())
stanc_js <- system.file("stanc.js", package = "StanHeaders", mustWork = TRUE)
test <- try(stanc_ctx$source(stanc_js), silent = TRUE)
if (inherits(test, "try-error")) {

if (packageVersion("StanHeaders") == "2.26.28") {

This comment has been minimized.

Sign in to view

Copy link
@hsbadr

hsbadr Oct 16, 2023

Member

@bgoodri Instead of hardcoding "2.26.28" version number, I'd compare the major version number of both StanHeaders and rstan.

This comment has been minimized.

Sign in to view

Copy link
@bgoodri

bgoodri Oct 16, 2023

Author Contributor

Yeah, that is a better way of coding it in general, but it is really only 2.26.28 that is a problem (due to the corrupted stanc.js). In any event, rstan 2.32.x is on CRAN as of Saturday with that in it and once I get StanHeaders 2.32.x up on CRAN we can take that logic out of zzz.R.

This comment has been minimized.

Sign in to view

Copy link
@bgoodri

bgoodri Oct 18, 2023

Author Contributor

Does anyone know why users need to compile their models without optimization in order to see the message when exceptions are thrown (and to not crash R)? I don't think this was happening before 2.32. E.g. ConnorDonegan/geostan#17

This comment has been minimized.

Sign in to view

Copy link
@hsbadr

hsbadr Oct 18, 2023

Member

Could it be CXX17 vs CXX14 compiler flags?

The other change I remember is replacing boost::lexical_cast<unsigned int> with std::stoull.

This comment has been minimized.

Sign in to view

Copy link
@bgoodri

bgoodri Oct 19, 2023

Author Contributor

Maybe. This Stan program does not even throw an exception with StanHeaders 2.26.x:

data {
  int N;
  vector[N] x;
}
transformed data {
  real bad = x[N + 1];
  print("bad = ", bad);
}
parameters {
  real<lower = 0, upper = 1> theta;
}

which is bad, but it is arguably worse that it throws an exception and crashes R under StanHeaders 2.32 (iff compiled with optimization).

This comment has been minimized.

Sign in to view

Copy link
@hsbadr

hsbadr Oct 19, 2023

Member

What's the difference in C++ code and behavior with the different combinations of rstan, StanHeaders, and stanc3 JS v2.26 and v2.32? I'd like to check if it's a change in Stan/Math or the generated C++ code. Maybe, we need to use stanc3 JS v2.32 to generate C++ code compatible with the new Stan/Math headers.

This comment has been minimized.

Sign in to view

Copy link
@bgoodri

bgoodri Oct 19, 2023

Author Contributor

StanHeaders 2.32 (not on CRAN) has the corresponding stanc.js and rstan 2.32 (on CRAN) uses the 2.32 stanc.js if both are installed (and uses the 2.26 stanc.js if StanHeaders 2.26 is installed). What I find confusing is that stanc.js 2.32 generates C++ code that crashes on an exception if compiled with -O2 or -O3 but is fine (albeit slow) with -O0. So, the C++ code is presumably right but the compiler optimization eliminates something that it tries to read in the event of an exception.

This comment has been minimized.

Sign in to view

Copy link
@hsbadr

hsbadr Oct 19, 2023

Member

It seems that a compiler flag may fix this issue. It could be triggered by a compiler flag such as -fno-exceptions.

This comment has been minimized.

Sign in to view

Copy link
@hsbadr

hsbadr Oct 19, 2023

Member

See http://gcc.gnu.org/onlinedocs/libstdc++/manual/using_exceptions.html

This comment has been minimized.

Sign in to view

Copy link
@bgoodri

bgoodri Oct 19, 2023

Author Contributor

We do want the exceptions because they give the user a message about what went wrong. But -fno-exceptions has to be specified explicitly I think in order to avoid them; in other words, I don't think it is enabled just by doing -O2 or higher or -std=c++17.

This comment has been minimized.

Sign in to view

Copy link
@hsbadr

hsbadr Oct 19, 2023

Member

I'm not sure about this specific flag, but it seems that -O2 flag enabled other flags that caused this issue. I'm thinking of adding a flag that forces the compiler to keep exceptions regardless of optimization level or explicitly enable specific optimization flags. Does -O1 cause the same problem?

       -O1 Optimize.  Optimizing compilation takes somewhat more time,
           and a lot more memory for a large function.

           With -O, the compiler tries to reduce code size and execution
           time, without performing any optimizations that take a great
           deal of compilation time.

           -O turns on the following optimization flags:

           -fauto-inc-dec -fbranch-count-reg -fcombine-stack-adjustments
           -fcompare-elim -fcprop-registers -fdce -fdefer-pop
           -fdelayed-branch -fdse -fforward-propagate
           -fguess-branch-probability -fif-conversion -fif-conversion2
           -finline-functions-called-once -fipa-profile -fipa-pure-const
           -fipa-reference -fipa-reference-addressable -fmerge-constants
           -fmove-loop-invariants -fomit-frame-pointer -freorder-blocks
           -fshrink-wrap -fshrink-wrap-separate -fsplit-wide-types
           -fssa-backprop -fssa-phiopt -ftree-bit-ccp -ftree-ccp
           -ftree-ch -ftree-coalesce-vars -ftree-copy-prop -ftree-dce
           -ftree-dominator-opts -ftree-dse -ftree-forwprop -ftree-fre
           -ftree-phiprop -ftree-pta -ftree-scev-cprop -ftree-sink
           -ftree-slsr -ftree-sra -ftree-ter -funit-at-a-time

       -O2 Optimize even more.  GCC performs nearly all supported
           optimizations that do not involve a space-speed tradeoff.  As
           compared to -O, this option increases both compilation time
           and the performance of the generated code.

           -O2 turns on all optimization flags specified by -O.  It also
           turns on the following optimization flags:

           -falign-functions  -falign-jumps -falign-labels
           -falign-loops -fcaller-saves -fcode-hoisting -fcrossjumping
           -fcse-follow-jumps  -fcse-skip-blocks
           -fdelete-null-pointer-checks -fdevirtualize
           -fdevirtualize-speculatively -fexpensive-optimizations -fgcse
           -fgcse-lm -fhoist-adjacent-loads -finline-small-functions
           -findirect-inlining -fipa-bit-cp  -fipa-cp  -fipa-icf
           -fipa-ra  -fipa-sra  -fipa-vrp
           -fisolate-erroneous-paths-dereference -flra-remat
           -foptimize-sibling-calls -foptimize-strlen -fpartial-inlining
           -fpeephole2 -freorder-blocks-algorithm=stc
           -freorder-blocks-and-partition  -freorder-functions
           -frerun-cse-after-loop -fschedule-insns  -fschedule-insns2
           -fsched-interblock  -fsched-spec -fstore-merging
           -fstrict-aliasing -fthread-jumps -ftree-builtin-call-dce
           -ftree-pre -ftree-switch-conversion  -ftree-tail-merge
           -ftree-vrp

           Please note the warning under -fgcse about invoking -O2 on
           programs that use computed gotos.

This comment has been minimized.

Sign in to view

Copy link
@hsbadr

hsbadr Oct 19, 2023

Member

If we find the culprit flag, we could disable it using something like -fno-<?>.

This comment has been minimized.

Sign in to view

Copy link
@bgoodri

bgoodri Oct 19, 2023

Author Contributor

I think the problem may be with the specific exception that is thrown in (one of) the geostan cases, rather than exceptions generally (the above Stan program throws the correct exception from the transformed data block regardless of the optimization level). In geostan's case, the problem is that the code is going down the wrong path and it indexes with an integer array of size 0.
stanc_js <- system.file("exec", "stanc.js", package = "rstan", mustWork = TRUE)
stanc_ctx$source(stanc_js)
}
} else stanc_js <- system.file("stanc.js", package = "StanHeaders", mustWork = TRUE)
stanc_ctx$source(stanc_js)
assignInMyNamespace("rstan_load_time", value = Sys.time())
set_rstan_ggplot_defaults()
assignInMyNamespace("RNG", value = get_rng(0))
Expand Down

0 comments on commit 350ef0d

Please sign in to comment.