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.

Sign up for GitHub

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

Jump to bottom

StanHeaders 2.26 revdep status #1034

Closed
andrjohns opened this issue Feb 10, 2023 · 20 comments
Closed

StanHeaders 2.26 revdep status #1034

andrjohns opened this issue Feb 10, 2023 · 20 comments

Comments

@andrjohns
Copy link
Contributor

andrjohns commented Feb 10, 2023 •
edited
Loading

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

hsbadr commented Feb 11, 2023

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

andrjohns commented Feb 11, 2023 •
edited
Loading

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: stan-dev/math#2874

@andrjohns
Copy link
Contributor Author

andrjohns commented Feb 11, 2023 •
edited
Loading

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 andrjohns mentioned this issue Feb 16, 2023
Closed
@andrjohns
Copy link
Contributor Author

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

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

jgabry commented Mar 10, 2023

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

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

bgoodri commented Mar 10, 2023

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

jgabry commented Mar 10, 2023

Sounds good

@andrjohns andrjohns mentioned this issue Mar 15, 2023
Closed
@andrjohns
Copy link
Contributor Author

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

hsbadr commented Mar 16, 2023

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

@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

@andrjohns andrjohns mentioned this issue Mar 20, 2023
Open
@WardBrian
Copy link
Member

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

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

@bgoodri
Copy link
Contributor

bgoodri commented Mar 23, 2023

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

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

bgoodri commented Mar 23, 2023 via email

@andrjohns
Copy link
Contributor Author

andrjohns commented Mar 24, 2023 •
edited
Loading

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

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 andrjohns mentioned this issue Mar 29, 2023
Open
@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
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@bgoodri @hsbadr @jgabry @andrjohns @WardBrian