search

stan-dev / rstan

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

StanHeaders 2.26 revdep status #1034

Closed andrjohns closed 1 year ago

andrjohns commented 1 year ago

@bgoodri @hsbadr FYI I re-ran the reverse-dependency checks for StanHeaders 2.26 on an (M1) Mac, and it flagged issues with some packages not having the -D_REENTRANT flag in their Makevars (and a couple of other smaller issues).

I've opened PRs for most packages to properly handoff the installation to rstantools, but here's the current status:

package version status Merged On CRAN
beanz 2.4 PR Opened
CARME 0.1 PR Merged ✓ ✓
conStruct 1.0.4 PR Opened ✓ ✓
densEstBayes 1.0-2.1 No public repo, emailed maintainer
dfpk 3.5.1 PR Opened
glmmfields 0.1.4 PR Opened ✓ ✓
glmmPen 1.5.2.11 PR Opened
idem 5.1 PR Opened
nlmixr2est 2.1.3 PR Opened ✓
OpenMx 2.21.1 PR Merged ✓
ProbReco 0.1.0.1 PR Opened
publipha 0.1.1 PR Opened
rPBK 0.2.0 No public repo, emailed maintainer ✓
rstanemax 0.1.3 PR Opened ✓ ✓
rxode2ll 2.0.9 PR Merged ✓ ✓
ssMousetrack 1.1.5 PR Opened
survHE 1.1.2 PR Merged ✓ ✓
trialr 0.1.5 PR Opened ✓ ✓
visit 2.1 PR Opened
PosteriorBootstrap 0.1.1 PR Opened ✓ ✓
rxode2parse 2.0.13 PR Opened ✓ ✓

EDIT: Running the revdep checks under Windows identified a failure with rxode2parse

hsbadr commented 1 year ago

Thanks @andrjohns! I've merged updated from the experimental branch into RStan v2.26 (currently, 2.26.15). Let's use the latest version in reverse dependency checks.

andrjohns commented 1 year ago

Great! I'll re-run today and let you know

EDIT: Only new failure was the blavaan package, caused by the new c++17 standard. PR with a workaround opened here: https://github.com/stan-dev/math/pull/2874

andrjohns commented 1 year ago

And additional failures/breakages under RStan 2.26 & StanHeaders 2.26:

package version status Merged On CRAN
baggr 0.7.4 PR Opened ✓ ✓
bakR 0.2.4 PR Opened ✓ ✓
BeeGUTS 1.0.0 PR Opened
bmstdr 0.3.0 PR Opened ✓
cbq 0.2.0.2 PR Opened
expertsurv 1.0.0 PR Merged ✓ ✓
hwep 2.0.0 PR Opened ✓ ✓
isotracer 1.1.3 PR Opened ✓ ✓
lgpr 1.1.5 PR Opened ✓
MADPop 1.1.4 PR Opened
oncomsm 0.1.2 PR Opened ✓ ✓
rmdcev 1.2.4 PR Opened ✓
RoBTT 1.0.0 PR Opened ✓ ✓
rstanarm 2.21.3 PR Opened
rts2 0.4 PR Opened
andrjohns commented 1 year ago

@bgoodri is there a particular state we want things to be in before a CRAN submission? Is it an option to submit to CRAN as-is and get a response on what else they would need done before a submission is accepted?

andrjohns commented 1 year ago

@bgoodri Sorry to double-ping! But it would be great to have an idea of what the goalposts are for the submission process, especially if there's anything extra that I can do to help

jgabry commented 1 year ago

Is it an option to submit to CRAN as-is and get a response on what else they would need done before a submission is accepted?

I’m not totally up to date on what the holdup is, so ignore this comment if not applicable.

If the delay is just because some packages would break and haven’t been updated yet, then there is no need to wait until they are all fixed as long as they are notified in advance and given time to make necessary updates. If they choose not to update their package for whatever reason, you can still submit to CRAN even if it breaks their package (just tell CRAN you notified them in advance). This happens all the time. So if you’ve already made a good faith effort to help update packages that break, I would say just go ahead and submit to CRAN. But I might be misunderstanding the situation.

andrjohns commented 1 year ago

Thanks for clarifying Jonah, that all makes sense to me! Hopefully this issue would be sufficient evidence for CRAN of notifying/patching packages as well.

bgoodri commented 1 year ago

I agree we have nominally met CRAN's policy about notifying package maintainers weeks (if not months or years) in advance of changes that will break their packages, but breaking 10+ packages is different than breaking one or two. I would also agree that we can't continue to do what we have been doing in terms of trying to make rstan / StanHeaders backward compatible with every CRAN package, but no widely-used C++ library breaks downstream projects as wantonly as Stan does. I'm going to pester these maintainers once more and then we will have to do a StanHeaders release.

jgabry commented 1 year ago

Sounds good

andrjohns commented 1 year ago

@hsbadr I've figured out the segfaults that you're seeing in the revdeps for prophet and rstanarm. These packages have tests which call rstan functions (rather than just the stan model compiled during installation).

If rstan 2.21 was compiled against StanHeaders 2.21 and then StanHeaders 2.26 is installed without recompiling rstan (which revdepcheck does via crancache), then any rstan::stan calls will segfault and crash the R session.

I'm not sure if this is something we'd have to resolve, or if it's a case of telling CRAN that rstan just needs to be rebuilt. I'm not familiar enough with CRAN to know what the response is likely to be. Thoughts?

hsbadr commented 1 year ago

This is a circular dependency. It can't be resolved without releasing both rstan and StanHeaders together, which isn't allowed by CRAN policies. We can tell them and could also release rstan that checks the version of StanHeaders at runtime. So, instead of segfaulting, it will stop with a meaningful error message.

For rstanarm, @bgoodri can get a pass. So, it's only about a single package. Should be accepted!

andrjohns commented 1 year ago

@bgoodri I don't think any more packages will be submitting to CRAN any time soon (in my opinion), so I think the StanHeaders release is good to go (if you agree!).

I also prompted the maintainers for the rstan 2.26 failures at the same time that you prompted the maintainers for the StanHeaders 2.26 failures, so they've had a similar period of notice. Hopefully this means that the rstan 2.26 submission could follow relatively quickly

WardBrian commented 1 year ago

Today during the Stan meeting @bgoodri requested a version of stanc3js 2.26 with all the current deprecation warnings backported.

I've prepared that branch here, based on the lexer-patch-v2.26.1 @andrjohns has already done some work on: https://github.com/stan-dev/stanc3/tree/stancjs-2.26.1-backport-warnings/

See the diff with lexer-patch-v2.26.1: https://github.com/stan-dev/stanc3/compare/lexer-patch-v2.26.1...stancjs-2.26.1-backport-warnings?expand=1

This ports all deprecation warnings from 2.31 back to 2.26, and also backports the improved error messaging where we include the code snippets and correct filenames.

I did not backport the auto-update functionality from recent versions. That is probably too difficult of a task with the older compiler code base. However, it should be reasonable to also ship stanc3js-2.31 with rstan2.26, and use that stanc only for the auto-updating.

Does that sound good @andrjohns @bgoodri @hsbadr ?

WardBrian commented 1 year ago

I uploaded a build stanc.js from that branch here: https://gist.github.com/WardBrian/141828caabbc7c50b810bfbd2d670748

bgoodri commented 1 year ago

Yes, that sounds good. If possible, can the parser warnings be made to say something to the effect of "call rstan:::future_stanc() on your Stan program to have it spit out a future-proof version" where future_stanc is an internal function of rstan's that wraps the call to the Javascript parser for 2.31 or 2.32 or whatever.

WardBrian commented 1 year ago

Yes, we can make the wording whatever you want. Right now the text always includes something along the lines of "This can be automatically changed using the canonicalize flag for stanc", so if you can tell me what the best RStan-specific message is to use there I can switch it out easily.

bgoodri commented 1 year ago

OK, we'll come up with something once StanHeaders 2.26 is accepted by CRAN, which hopefully will be soon because it breaks less than 2.21 does at the moment.

On Thu, Mar 23, 2023 at 3:31 PM Brian Ward @.***> wrote:

Yes, we can make the wording whatever you want. Right now the text always includes something along the lines of "This can be automatically changed using the canonicalize flag for stanc", so if you can tell me what the best RStan-specific message is to use there I can switch it out easily.

— Reply to this email directly, view it on GitHub https://github.com/stan-dev/rstan/issues/1034#issuecomment-1481779241, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAZ2XKWMECSU7K6OR3EW3J3W5SQIHANCNFSM6AAAAAAUXRZGGY . You are receiving this because you were mentioned.Message ID: @.***>

andrjohns commented 1 year ago

I think these backported deprecation warnings and auto-formatting from 2.31 would really only be needed if we thought there was going to be a long time between the 2.26 and 2.31 releases. At the moment there are only three two one packages failing revdep checks with 2.31, all of which already have patches submitted or merged.

Just to say that these backports shouldn't be considered blocking for CRAN submission, imo

WardBrian commented 1 year ago

It's great if we don't need them! I just wanted to make sure it was feasible if we did need them, and it turned out to not be too bad (just copy/pasting 3-4 files from the current stanc back in time was nearly enough on its own) so it isn't much wasted effort either way

andrjohns commented 1 year ago

Obsoleted by #1053