stan-dev / rstan

RStan, the R interface to Stan
https://mc-stan.org
1.02k stars 264 forks source link

Future RStan/StanHeaders 2.31+ Issues #1046

Closed andrjohns closed 1 year ago

andrjohns commented 1 year ago

Opening this issue to flag early any difficulties that are going to be encountered in the (eventual) transition from 2.26 -> 2.31+

Will update as/if more are encountered

andrjohns commented 1 year ago

@hsbadr What would you think of bundling the stanc.js in StanHeaders rather than rstan? Then it would be much harder to encounter mismatches between the generated c++ and the available headers during the upgrade process

hsbadr commented 1 year ago

What would you think of bundling the stanc.js in StanHeaders rather than rstan?

I think that this is handled by the dependency on the newer version of StanHeaders. rstan >= 2.31 requires preinstalled StanHeaders >= 2.31. Also, stanc is linked to rstan::stanc not the Stan headers. We could check the versions or checksums.

The future issue I see now is due to merging https://github.com/stan-dev/math/pull/2583 while RcppEigen on CRAN hasn't been updated to Eigen 3.4 yet (see https://github.com/RcppCore/RcppEigen/issues/103). That's why I've freezon the experimental branch rstan for now.

andrjohns commented 1 year ago

But the issue is with the rstan 2.26 and StanHeaders 2.31 combination - since the code generated by the 2.26 stanc is not compatible with the 2.31 headers

andrjohns commented 1 year ago

Also, the rstan 2.31 & StanHeaders 2.31 combination does not appear to be compatible with any packages. Installing a package with Stan models results in the following error:

In file included from stanExports_stanmarg.cc:5:
In file included from ./stanExports_stanmarg.h:23:
In file included from /Users/andrew/Library/R/arm64/4.2/library/rstan/include/rstan/rstaninc.hpp:4:
/Users/andrew/Library/R/arm64/4.2/library/rstan/include/rstan/stan_fit.hpp:930:9: error: field type 'model_stanmarg_namespace::model_stanmarg' is an abstract class
  Model model_;
hsbadr commented 1 year ago

But the issue is with the rstan 2.26 and StanHeaders 2.31 combination - since the code generated by the 2.26 stanc is not compatible with the 2.31 headers

Ummm. This makes sense.

Also, the rstan 2.31 & StanHeaders 2.31 combination does not appear to be compatible with any packages.

I didn't run into any issues with earlier versions. So, we need to update stan_fit & Module after https://github.com/stan-dev/stan/pull/3139.

hsbadr commented 1 year ago

Also, the rstan 2.31 & StanHeaders 2.31 combination does not appear to be compatible with any packages.

Looks like we need to update stanc3 JS after https://github.com/stan-dev/stan/pull/3139.

andrjohns commented 1 year ago

Looks like we need to update stanc3 JS after https://github.com/stan-dev/stan/pull/3139.

Good call! I just tested with the latest nightly stanc and all worked without issue.

hsbadr commented 1 year ago

Good call! I just tested with the latest nightly stanc and all worked without issue.

Yes, I've shipped it in the experimental version. This will get automatically fixed when stanc3 v2.32 is released. Note that RcppEigen v3.4 is required now.

andrjohns commented 1 year ago

When you've got a minute, would you mind backporting the stanc location change to the 2.26 branch and doing another release? Once it's updated in the repo I can run the 2.26 + 2.31 revdeps

hsbadr commented 1 year ago

v2.26.18 has been released.

andrjohns commented 1 year ago

Brilliant, thanks!

andrjohns commented 1 year ago

Looks like expose_stan_functions() is broken (for both the 2.26+2.31 & 2.31+2.31 combinations):

funmod <- "
functions {
  real rtn_input(real x) {
    return x;
  }
}
"

rstan::expose_stan_functions(rstan::stanc(model_code = funmod))
#> Error in compiled$functions: $ operator is invalid for atomic vectors

This is causing a test failure for the prophet package

andrjohns commented 1 year ago

The 2.26 + 2.31 revdep checks only identified six failures in addition to those from #1034:

package version status Merged On CRAN
EcoEnsemble 1.0.1 No public repo, emailed maintainer
prophet 1.0 expose_stan_functions failure Resolved by #1050
ctsem 3.7.2 PR Opened
RoBTT 1.0.1 PR Opened
tmbstan 1.0.4 PR Opened
TriDimRegression 1.0.1 Stanc3 Issue Resolved

Looks like it might be a fairly painless transition from 2.26 -> 2.31 after all!

hsbadr commented 1 year ago

Looks like expose_stan_functions() is broken

Confirmed! I think the generated C++ code fails to compile. We may need to fix it in expose_stan_functions_hacks(). I don't have time to look into it right now, but if you create a PR, I can review it ASAP.

Looks like it might be a fairly painless transition from 2.26 -> 2.31 after all!

Yes, once StanHeaders v2.26 is on CRAN, we could probably release rsran v2.32 followed by StanHeaders v2.32.

cdriveraus commented 1 year ago

ctsem can't switch to rstantools on CRAN until CRAN stops testing on 32 bit windows, which is planned and hopefully soon but couldn't find a timeframe anywhere...

andrjohns commented 1 year ago

ctsem can't switch to rstantools on CRAN until CRAN stops testing on 32 bit windows

Why is that? Do you have custom makeflags/entries that need to be added?

cdriveraus commented 1 year ago

the system runs out of memory when compiling on win32 so I have a bunch of exceptions and hacks coded in to create basically a dummy package on w32 systems...

andrjohns commented 1 year ago

If you can link me to those, I can show you how to add them to the rstantools setup

cdriveraus commented 1 year ago

https://github.com/cdriveraus/ctsem/blob/master/src/Makevars.win has a bunch of commented out stuff still floating around but the core piece I needed (with the rest handled in the R code) is this, which just ignores any of the .stan code on win32:

ifeq   "$(WIN)" "64" 
    SOURCES = $(wildcard stan_files/*.stan)
else
    SOURCES = $(wildcard)
endif
andrjohns commented 1 year ago

If you don't want to build the models at all, then you can just use an if statement in the configure.win script to only generate the c++ on 64-bit:

if [${R_ARCH} = "/x64"]
then
  "${R_HOME}/bin${R_ARCH_BIN}/Rscript.exe" -e "rstantools::rstan_config()"
fi
cdriveraus commented 1 year ago

I guess it's something simple but /x64 seems problematic somehow... running this in bash shows that it's not working as expected, as it echoes the 'not 64 bit' message when building on 64 bit R.

if [${R_ARCH} = "/x64"]; then
    # If it is, run the rstan_config() function
    "${R_HOME}/bin${R_ARCH_BIN}/Rscript.exe" -e "rstantools::rstan_config()"
else
    echo "R architecture is not 64-bit"
    echo ${R_ARCH}
fi

but also when I submitted to winbuilder with a check based on pointer size, this also didn't work, presumably because R is in both 32 bit and 64 bit form so passes such a check on bash when the build is initiated for 32 bit R? so I'm not sure this approach will suffice.

andrjohns commented 1 year ago

Ah I see what you mean. It might not actually be an issue though, as the current CRAN rstan doesn't build on 32-bit windows either - so the CRAN page only has windows binaries for R4.2+

cdriveraus commented 1 year ago

Interesting, I can give it a try then...

andrjohns commented 1 year ago

@cdriveraus There's a bug in the compilation of eigenvalues_sym in Stan 2.31 which will break your package installation. The simple fix until it's patched is to change this line to add to_vector.

From:

if(corpriortype==3) corprior= normal_lpdf(eigenvalues_sym(mcor) | 0, 1);

To:

if(corpriortype==3) corprior= normal_lpdf(to_vector(eigenvalues_sym(mcor)) | 0, 1);

This will have next to no runtime cost since the to_vector() still propagates expressions

cdriveraus commented 1 year ago

ctsem is updated on CRAN, using rstantools and with the eigenvalues_sym workaround.

andrjohns commented 1 year ago

I've also checked against 2.26 and 2.31 and all is passing, thanks!

andrjohns commented 1 year ago

Obsoleted by #1053