Upcoming Stan release removes old array syntax, new RStan needed #1084
Comments
|
If it's ready, I would go ahead and submit it ASAP, I think CRAN is back from vacation |
|
@bgoodri if you're short on time or overloaded with other work, did you want to temporarily assign someone else as the maintainer for CRAN? Then we can get everything up to date ASAP and take some of the pressure off. @hsbadr would be my first suggestion, but I'm also happy to take it if he doesn't have capacity either |
|
I can do it. I have just been swamped this past month with a conference and teaching. We need to merge @andrjohns PR about switching the javascript library, but then 2.26.x is pretty much ready I think. Hopefully, we can go to 2.32.x almost immediately afterward because the error messages in 2.26 are not great and it doesn't have the tool to convert to the new array syntax. I imagine it will take a bit before all the packages actually convert to the new array syntax though. |
|
Thank you for all your efforts in getting this to work! From the perspective of brms, not having the new array syntax in rstan is a huge problem since the latest cmdstan(r) will not support the old one anymore. So without the new rstan, I can either give up on one of the backends (which I won't), remove brms from CRAN (which I won't either), or add a lot of code that outputs new or old array syntax depending on the stan version that is used. The latter I would like to avoid as well, of course, since it complicates the whole code structure and would be a lot of additional work just for a temporary work around. So from my side, I really cannot wait to have rstan 2.26+ on CRAN! |
|
@bgoodri Here are all the changes needed for rstan 2.26 on CRAN, I've run the reverse dependency checks and also checked on winbuilder and all passes. StanHeaders needs one header updated for c++17: stan-dev/math@StanHeaders_2.26...rstan226-cran Rstan changes (including the change from V8): StanHeaders_2.26...rstan-cran Can you please either submit ASAP or let me know what else you need. The longer it takes to get rstan up, the worse it's going to be when dealing with the array syntax in packages |
Thank you!
Agree, let me know if there's anything I can do to help. |
This is a big change that requires a lot of effort. The
Do we need to rebase this for the
Does CRAN allow for multiple maintainers? If so, I'd add more maintainers with @bgoodri to keep RStan alive. |
Regarding the dependencies, if we give them a month or two of notice it should be OK for us to submit the update. So we could do 2.26 now and then 2.32 this fall. Especially if we give them a way to update their code (there's a tool to convert to the new syntax right?) then we satisfy CRAN's policy and we shouldn't keep holding up Stan development (plus having 2.26 out is going to lead to a lot of confusion if the error messages are messed up). I'm happy to help out with any of this (dependencies, test code), just let me know if/how I can be useful.
I'm not sure. @andrjohns @bgoodri?
No, unfortunately. They want a single point of contact. Thanks again for all the work you've done to help with this already! |
|
We are go for StanHeaders with the additional The develop branch of rstan would satisfy the reverse dependency checks if not for that one error that is now fixed with StanHeaders. It is similar to @andrjohns branch, especially in the sense that it uses his QuickJSR package rather than V8 to handle the javascript stanc, which allows us to drop the old C++ stanc and parse both the old and the new array syntax. So, I think we should do a rstan 2.26 right after the StanHeaders 2.26.28 gets through CRAN so that @paul-buerkner just has to push a brms that requires rstan >= 2.26 . I do want to get both StanHeaders and rstan up to Stan 2.32 right afterward, which as I understand it is basically the experimental branch. That release accepts the old array syntax but warns that it will be unsupported in 2.33. So, we will presumably have to prod a bunch of packages to run the canonicalizing tool on their Stan files. |
|
Uploading the new StanHeaders (that only changed one It is the same error that we have encountered and fixed before, so I don't understand how it got triggered in these few circumstances by a StanHeaders that should not have affected their C++ code. |
|
I have no idea unfortunately. That said, it is totally okay for rstan to break some downstream packages with a major release. We desperately need rstan 2.26+ on CRAN asap (not just for brms but for many other reasons too). So if figuring out this error would caused another non-trivial delay to the release, I would strongly argue in favor of just allowing these packages to break and be fixed later (or removed from CRAN if no fixe it provided). |
|
It is possible that we can go ahead with the rstan 2.26 because doing so would not make those three packages any "worse" as far as the CRAN checks are concerned than what they already are now that StanHeaders 2.26.28 already broke them. But it is still our responsibility to manage backwards incompatible changes on CRAN, even though in this case, it seems to be due to those packages using the older build process rather than letting rstantools do its thing to regenerate the C++ and the Makevars{.win} files when the package is compiled. |
|
@bgoodri those failures are because the packages aren't using They're easy fixes and I'll open PRs now, all safe to go ahead with the rstan submission |
|
OK. Let me see if I can merge your rstan-cran branch into develop without
too many merge conflicts.
…On Thu, Sep 7, 2023 at 12:43 PM Andrew Johnson ***@***.***> wrote:
@bgoodri <https://github.com/bgoodri> PRs submitted:
- bmlm <mvuorre/bmlm#23>
- Rlgt <cbergmeir/Rlgt#6>
- eggCounts: No public repo, emailed maintainer
I think that's plenty to satisfy CRAN, so please go ahead and submit rstan
or let me know if you need anything else
—
Reply to this email directly, view it on GitHub
<#1084 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKWYXW6EJGNUAMNTVTDXZH2TTANCNFSM6AAAAAA36XFGEI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I appreciate the work on this. I just updated my rmsb package to work with the new syntax for |
|
Hopefully just a few more hours Frank.
…On Thu, Sep 7, 2023, 5:44 PM Frank Harrell ***@***.***> wrote:
I appreciate the work on this. I just updated my rmsb package to work with
the new syntax for cmdstanr only to find that it breaks rstan so I've
kind of stuck.
—
Reply to this email directly, view it on GitHub
<#1084 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKS7542MJNQQSQLNJCDXZI52XANCNFSM6AAAAAA36XFGEI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@paul-buerkner @harrelfe There is now a RStan on CRAN that accepts both array syntaxes. We will have to do a follow-up RStan based on 2.26 before we can do one based on 2.32 but that shouldn't affect you. It does seem possible to make a Stan program that is too complicated to be parsed by QuickJSR, so that might become an issue for some brms models. |
|
Thanks, nice to see this out. Not sure if I should start a new issue, but with the updated rstan / stanheaders, package building (with ctsem) has a substantial pause (30s maybe?) where it 'looks' like nothing is happening. |
|
That is presumably the parser parsing. I had the same thing with rstanarm (and then it failed). So, if it is just a compile time thing (in the case of ctsem and other packages that use rstantools), I am not too worried. The pause will be less noticeable with 2.32. |
|
happens with the rstantools configure script disabled too, and no compilation occurring, fyi. |
This is because we're using a less powerful javascript engine (quickjs instead of V8) which is more portable, but less performant. There were significant optimisations in |
|
I think I will modify the rmsb package to be for cmdstan and create a new package rmsbr whose sole content is rstan stanmodel objects that rmsb can use , primarily for Windows users. I'd like to avoid some future stan version conflicts. @paul-buerkner what do you think of that? |
I don't think that's necessary (imo), we're past the difficult conflicts and updates so things should be fairly smooth from here on |
|
I think you're right @andrjohns but I wonder if I need to hedge my bets because of future changes to |
Speaking of @hsbadr, looks like still listed as a contributor. Should probably be an author going forward! (Noticed this when trying to update the website) |
|
Hi, @harrelfe. @andrjohns and @bgoodri have been working hard to make sure that things will be smoother after the 2.26 update (which just went live on CRAN) that things will be smoother. To ensure there are no nasty surprises, I would strongly recommend updating your Stan code so that there are no deprecation warnings. We are going to start removing deprecated functionality. Also, we're not guaranteeing that binary models will be compatible version to version, just the Stan programs will still work if you recompile them. Other than that, there shouldn't be issues with backward compatibility going forward. |
Sorry---missed the later comment. The backward compatibility issues will be the same for cmdstan as for rstan and cmdstanr---we plan to remove deprecated functionality. Overall, we're recommending that people use cmdstanr when possible because it's not clear we'll keep upgrading RStan in the future given how much of a pain it is to get things on CRAN given lack of version dependency management and restrictive external unit test compatibility. |
|
@WardBrian Can we temporarily get a patched version of stac3 JS v2.33.0 with soft deprecations (warnings instead of errors)? |
|
I think it wouldn’t be too much work to do so, I think we could cleanly revert the PR which made them errors (but the messages would say “2.33”, which might be confusing if it is 2.33) Can I ask why this would be useful? |
It will allow us to quickly release RStan v2.33 and buy time to let the dependencies adopt the new syntax. |
|
I would prefer there to never be a version where users could write both the old array syntax and the new tuple types. They interact in weird and inconsistent ways (which was one of the primary motivations for the syntax change as I understand). I think the plan of moving to 2.32 and pausing to allow packages to update is the best |
Sounds good to me. |
|
Does anyone know how to avoid this "significant" (in CRAN's eyes) warning message from clang that is now happening to @cdriveraus package with the 2.26ish Stan? |
|
@bgoodri this is fixed now, just some pointless code that crept in, and I believe there is the intention to automatically remove such lines in future stan versions... |
In addition to @WardBrian’s reason for not jumping straight to 2.33, another reason to prioritize 2.32 is that 2.33 has pathfinder, which still needs an RStan implementation and would delay things further. Doing an RStan 2.33 without the new featured method of Stan 2.33 would lead to a bunch of confusion, I think. |
|
The new |
There’s an Additional_repositories field you can specify in DESCRIPTION, e.g. in brms, which has cmdstanr in Suggests: |
This might also be of interest: https://discourse.mc-stan.org/t/instantiate-pre-compiled-cmdstan-models-in-r-packages/32191 |
|
Something that's also going to be a little tricky, Line 232 in 951683d It only checks for errors. There needs to be an additional block: else if (length(model_cppcode$warnings)) {
model_cppcode$status <- FALSE
warning(paste(model_cppcode$warnings, collapse = "\n"))
}However, this would currently cause downstream dependencies to start failing
Any preferences? |
It's ready to get merged into I think we should merge it as it's the real development branch, and then we create a branch of it for each release and tag it. It seems that we target v2.32 for now. That would require resetting the Stan submodule to v2.32.2 tag, shipping @bgoodri Are you ok with this? If so, I can create a PR to merge the |
|
@hsbadr quick FYI on the |
|
Alternatively, updating the reverse dependencies with the new array syntax should be an easy task (though, it might cause delays depending the response of the maintainers). If we create PRs for this, we can submit v2.33 to CRAN after a few weeks. This will let CRAN as well force the maintainers to update their packages with the new array syntax. |
Yep I'm already in progress with this. I've got PR's open with about half of the downstream dependencies. I'll be opening an issue summarising the progress once I've identified all of the downstream failures |
It uses the current |
Fixed. It was a leftover from merging the |
|
thanks for working on this!
If we want to go this route, I think message() instead of warning() could probably get around CRAN issues. that wouldn’t convey the same sense of urgency as a warning but we could use strong language in the message I guess. |
|
@bgoodri @hsbadr @andrjohns What's the current status of StanHeaders? RStan is at 2.32 but StanHeaders is at 2.26 and I think that's starting to lead to understandable confusion about which features are available in RStan (it seems reasonable to expect 2.32 features). For example, see this forum post from @cdriveraus about complex numbers: https://discourse.mc-stan.org/t/complex-custom-functions-not-working/33268. If I recall correctly StanHeaders 2.32 was held up by CRAN for some reason, but not sure if that's accurate. Anything I can do to help? |
|
The status of StanHeaders 2.32 (with the stanc.js from 2.32) is blocked by ConnorDonegan/geostan#17 |
|
Oh ok, thanks. Have you seen the most recent message there? ConnorDonegan/geostan#17 (comment) |
See https://discourse.mc-stan.org/t/cmdstan-stan-2-33-release-candidate/32531. This one actually removes the old array syntax, not just a deprecation warning.
@bgoodri @andrjohns @hsbadr Is RStan with new array syntax ready to be submitted to CRAN?
The text was updated successfully, but these errors were encountered: