-
-
Notifications
You must be signed in to change notification settings - Fork 21
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 RStan 2.26 building & Standalone functions #85
Conversation
inst/include/sys/Makevars
Outdated
@@ -1,6 +1,6 @@ | |||
STANHEADERS_SRC = $(shell "$(R_HOME)/bin$(R_ARCH_BIN)/Rscript" -e "message()" -e "cat(system.file('include', 'src', package = 'StanHeaders', mustWork = TRUE))" -e "message()" | grep "StanHeaders") | |||
|
|||
PKG_CPPFLAGS = -I"../inst/include" -I"$(STANHEADERS_SRC)" -DBOOST_DISABLE_ASSERTS -DEIGEN_NO_DEBUG -DBOOST_MATH_OVERFLOW_ERROR_POLICY=errno_on_error | |||
PKG_CPPFLAGS = -I"../inst/include" -I"$(STANHEADERS_SRC)" -DBOOST_DISABLE_ASSERTS -DEIGEN_NO_DEBUG -DBOOST_MATH_OVERFLOW_ERROR_POLICY=errno_on_error -DUSE_STANC3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
USE_STANC3
should only be defined if the C++ code is generated by stanc3
. This is ok when rstan 2.26
is released, but the transition to CRAN may require releasing stan-dev/rstan#912 first that's compatible with the older version of rstan
. So, if USE_STANC3
is defined, Math v4+ headers will be exposed and break the C++ code generated by stanc2
.
Solution: While the development version of rstan
defines USE_STANC3
in the generated C++ code, it's safe to define it here only if (utils::packageVersion("rstan") >= 2.26)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this comment! Thanks for clarifying that, good catch. I'll fix that up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. You may also check nzchar(system.file("stanc.js", package = "rstan"))
, but utils::packageVersion("rstan") >= 2.26
is the right way since we can remove the local JS to use the nightly version of stanc3
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrjohns A variant of the following should work:
"$(R_HOME)/bin$(R_ARCH_BIN)/Rscript" -e "cat(ifelse(utils::packageVersion('rstan') >= 2.26, '-DUSE_STANC3', ''))"
Co-authored-by: Hamada S. Badr <hamada.s.badr@gmail.com>
Co-authored-by: Hamada S. Badr <hamada.s.badr@gmail.com>
@hsbadr sorry I forgot about this! I also modified the function called by What do you think? |
Thanks @andrjohns! This makes sense to me, particularly because we plan to require using |
I've done some basic testing with CRAN rstan + StanHeaders 2.26 and rstan 2.26 + StanHeaders 2.26 and all seems to be working, but will test a bit more over the next couple of days. In terms of getting things out to CRAN quickly, I'd be leaning towards an |
@hsbadr can we merge a fix with just the addition of the |
Sounds good to me. The major challenge with CRAN releases is the backward incompatibility at C++ level. Some packages either link directly to |
Great! I've made those changes. Would we be able to get this out to CRAN? |
I've approved it. I defer to @bgoodri on merging and CRAN release. |
@hsbadr I've made some additional changes, if you wouldn't mind having another look. I've added compatibility for packages that have stan models with just standalone functions ( The majority of the changes are based on the existing stan_functions branch by @bgoodri, with the addition of some compatibility changes with I've tested the changes against the |
Sorry for the delay! I'll test this ASAP. |
Apologies to push more changes, but the implementation was failing for cases where the plain type wasn't templated, so have fixed that. This was an issue for the |
@hsbadr and @bgoodri I've updated this discourse post to track the reverse-dependencies that are blocking the next release. After this PR is merged and out to CRAN, there will be:
All of which have either had PRs submitted or the maintainer contacted. Several packages will still fail on CRAN, but have already merged PRs with fixes into their dev repos. I think once this PR gets out to CRAN, we could advertise a cut-off date for the above packages to merge the changes before we go ahead with submission. I think that would satisfy CRAN that we've done as much as possible |
@andrjohns I agree with you that's sufficient and CRAN can push really hard to force the package maintainers to update their packages (or simply merge and release your PRs). On a related note, I've tested two alternative solutions that work together but are probably not recommended. One redirects the Stan/Math headers into I personally vote for your approach (officially contact the maintainers to apply the required changes in the PRs & give them a couple weeks before submitting I'm tagging @wds15 b/c I think he's interested in this discussion. |
@hsbadr I mentioned this PR to @bgoodri in the Stan meeting and he said that the It turns out that |
@andrjohns Any code generated by the experimental version of https://github.com/stan-dev/rstan/blob/experimental/rstan/rstan/R/stanc.R#L218-L223 |
I think if we can do something along the lines linking libStanHeaders.{so,dylib,dll} to the TBB shared library, then we could keep progressing on releases while pushing package maintainers to fix their linking and migrating toward rstantools 2.x style packaging. |
The problem here is that the TBB shared library won't be in the library path for the packages, unless it's a system library or they're linking to |
Yeah I did run into that issue when doing some more testing, so reverted the makevars changes. @bgoodri are you happy to merge these changes? |
Let's try it.
|
Great, thanks! @jgabry would you be able to submit an updated version to CRAN when you have a minute? Once this hits CRAN we can put a clock on package maintainers to merge and release the PRs that I've opened for their packages before we submit the new StanHeaders |
Let's see where we are at with Hamada's idea to treat TBB like Sundials in
StanHeaders before making any proclamations.
…On Thu, Sep 30, 2021 at 8:55 PM Andrew Johnson ***@***.***> wrote:
Great, thanks! @jgabry <https://github.com/jgabry> would you be able to
submit an updated version to CRAN when you have a minute? Once this hits
CRAN we can put a clock on package maintainers to merge and release the PRs
that I've opened for their packages before we submit the new StanHeaders
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#85 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKQVYCRAVUFMMZ2URYTUEUBG7ANCNFSM43P63X5A>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Isn't that approach only necessary while the reverse dependencies themselves aren't configured for linking to the TBB? We only have 14 packages that aren't setup for this yet, all of which I've opened PRs for (more in this post) |
Great! I'm on it.
True. But, as far as the changes in those PRs (specifically linking to |
@andrjohns @hsbadr @bgoodri I'm now getting an error from
coming from |
This PR updates
rstan_config()
to defineUSE_STANC3
prior to including stan headers, and also adds functionality for building packages with stan models of just standalone functions