search

stan-dev / rstantools

Tools for Developing R Packages Interfacing with Stan
https://mc-stan.org/rstantools
GNU General Public License v3.0
51 stars 21 forks source link

Fix RStan 2.26 building & Standalone functions #85

Closed andrjohns closed 3 years ago

andrjohns commented 3 years ago

This PR updates rstan_config() to define USE_STANC3 prior to including stan headers, and also adds functionality for building packages with stan models of just standalone functions

andrjohns commented 3 years ago

@hsbadr sorry I forgot about this! I also modified the function called by rstan_config to append the rstantools Makevars to the package-included makevars. This way the user just needs to include the rstantools configure files, and then we can update the rstantools Makevars as needed.

What do you think?

hsbadr commented 3 years ago

Thanks @andrjohns! This makes sense to me, particularly because we plan to require using rstantools moving forward. I haven't tested this PR with the different versions of rstan (CRAN ~ should be ok, transitional 2.26, experimental/development 2.27+). I think it can stay pending until we decide on the best way to address the issues with CRAN submission. In one of the potential solutions, we may be able to redirect the Stan/Math headers from StanHeaders to rstan in order to fix the circular dependency (rstan will be updating the headers directly) but that's still under discussion.

andrjohns commented 3 years ago

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 rstantools implementation like this one, since it requires pretty minimal changes from package maintainers in the first instance and would be ready quickly. But I'm easy either way

andrjohns commented 3 years ago

@hsbadr can we merge a fix with just the addition of the STANC_FLAGS section (i.e., removing the changes to rstan_config)? That would fix a good deal of reverse dependencies for 2.26 at the moment

hsbadr commented 3 years ago

@hsbadr can we merge a fix with just the addition of the STANC_FLAGS section (i.e., removing the changes to rstan_config)? That would fix a good deal of reverse dependencies for 2.26 at the moment

Sounds good to me. The major challenge with CRAN releases is the backward incompatibility at C++ level. Some packages either link directly to StanHeaders using their pre-generated C++ code or don't rerun/update Stan to C++ code via rstantools. This requires patching the headers, which in turn breaks many packages especially because we can't release both StanHeaders and rstan at the same time. To address this (completely for all packages that use rstantools & partially for the minority of the other packages), we're testing a redirection of the headers to rstan (and reversing the dependency) in a few minor releases. This may require a legal advice (licensing) though. We'll likely open this for discussion on the forums.

andrjohns commented 3 years ago

Great! I've made those changes. Would we be able to get this out to CRAN?

hsbadr commented 3 years ago

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.

andrjohns commented 3 years ago

@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 (rmdcev as the main example). Currently those packages have to manually generate and modify the c++, which gets in the way of upgrading rstan versions

The majority of the changes are based on the existing stan_functions branch by @bgoodri, with the addition of some compatibility changes with stanc3 (changing the auto return to a plain type).

I've tested the changes against the rmdcev package (fork here if you want to test locally) with both rstan 2.21 & stanheaders 2.26 and rstan 2.26 & stanheaders 2.26

hsbadr commented 3 years ago

Sorry for the delay! I'll test this ASAP.

andrjohns commented 3 years ago

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 lgpr package, so you can also test against this fork: https://github.com/andrjohns/lgpr/tree/rstan-next

andrjohns commented 3 years ago

@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

hsbadr commented 3 years ago

@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 rstan to free it from StanHeaders dependency (so we can release updates directly by maintaining rstan only, mostly) -- This requires legal advice on licensing related issues. The second fixes the TBB dependency by building the required code into libStanHeaders. On one hand, this is not recommended by Intel, but on the other hand, it fixes many packages and will let us control TBB version directly from Math.

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 StanHeaders v2.26.4 to CRAN and reporting the taken actions).

I'm tagging @wds15 b/c I think he's interested in this discussion.

andrjohns commented 3 years ago

@hsbadr I mentioned this PR to @bgoodri in the Stan meeting and he said that the USE_STANC3 definition should have been handled by stanc, rather than needing the additional makeflags.

It turns out that rstan_config() was inserting the #define USE_STANC3 after "#include <rstan/rstaninc.hpp>", which means that all of the rstan/stan headers were being included before the stanc3 flag was set. By updating rstan_config to append the rstaninc header after the definition (relevant change here), packages are compiling fine with 2.26+2.26 without needing the additional makevars flag

hsbadr commented 3 years ago

@andrjohns Any code generated by the experimental version of rstan does define USE_STANC3, which can be removed when/if stanc3 handles it. But, some packages may fail when they somehow customize the (order of) headers or have generated compatible C++ code by an earlier version of rstan >= 2.26.

https://github.com/stan-dev/rstan/blob/experimental/rstan/rstan/R/stanc.R#L218-L223

bgoodri commented 3 years ago

The second fixes the TBB dependency by building the required code into libStanHeaders. On one hand, this is not recommended by Intel, but on the other hand, it fixes many packages and will let us control TBB version directly from Math.

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.

hsbadr commented 3 years ago

The second fixes the TBB dependency by building the required code into libStanHeaders. On one hand, this is not recommended by Intel, but on the other hand, it fixes many packages and will let us control TBB version directly from Math.

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 RcppParallel. What I've tested, and it works, is automatically adding some/all of the TBB objects from Math into the static library libStanHeaders.a and that also allows shipping the TBB headers without version conflicts. So, basically, we treat TBB like sundials. I think we could exploit this workaround at least temporarily until we can revert it later. If you agree, I can merge the changes and update the revdep results.

andrjohns commented 3 years ago

But, some packages may fail when they somehow customize the (order of) headers or have generated compatible C++ code by an earlier version of rstan >= 2.26.

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?

bgoodri commented 3 years ago

Let's try it.

andrjohns commented 3 years ago

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

bgoodri commented 3 years ago

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 https://github.com/stan-dev/rstantools/pull/85#issuecomment-931808960, 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.

andrjohns commented 3 years ago

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)

hsbadr commented 3 years ago

Let's try it.

Great! I'm on it.

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)

True. But, as far as the changes in those PRs (specifically linking to RcppParallel and respecting its flags for library paths & -ltbb) aren't submitted to CRAN, our reverse dependency checks will fail.

jgabry commented 3 years ago

@andrjohns @hsbadr @bgoodri I'm now getting an error from rstan_config that we didn't used to get. One way to reproduce is to try knitting the vignette "minimal-rstan-package.Rmd". When I do that I get: 

Error in if (!after) c(values, x) else if (after >= lengx) c(x, values) else c(x[1L:after],  : 
  argument is of length zero

coming from rstan_config. I also get this error from calling rstan_config outside the vignette after using rstan_create_package, adding a Stan file, then calling rstan_config.