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

Future RStan/StanHeaders 2.31+ Issues #1046

Closed
andrjohns opened this issue Mar 11, 2023 · 27 comments
Closed

Future RStan/StanHeaders 2.31+ Issues #1046

andrjohns opened this issue Mar 11, 2023 · 27 comments

Comments

@andrjohns
Copy link
Contributor

andrjohns commented Mar 11, 2023

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

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

hsbadr commented Mar 13, 2023

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 stan-dev/math#2583 while RcppEigen on CRAN hasn't been updated to Eigen 3.4 yet (see RcppCore/RcppEigen#103). That's why I've freezon the experimental branch rstan for now.

@andrjohns
Copy link
Contributor Author

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

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

hsbadr commented Mar 14, 2023

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 stan-dev/stan#3139.

@hsbadr
Copy link
Member

hsbadr commented Mar 14, 2023

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 stan-dev/stan#3139.

@andrjohns
Copy link
Contributor Author

Looks like we need to update stanc3 JS after stan-dev/stan#3139.

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

@hsbadr
Copy link
Member

hsbadr commented Mar 14, 2023

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

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

hsbadr commented Mar 15, 2023

v2.26.18 has been released.

@andrjohns
Copy link
Contributor Author

Brilliant, thanks!

@andrjohns
Copy link
Contributor Author

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

andrjohns commented Mar 15, 2023

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

hsbadr commented Mar 16, 2023

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

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

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

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

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

@cdriveraus
Copy link

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

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

cdriveraus commented Mar 16, 2023

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

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

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

@andrjohns
Copy link
Contributor Author

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

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

@andrjohns
Copy link
Contributor Author

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

@andrjohns
Copy link
Contributor Author

Obsoleted by #1053

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

3 participants