@wds15 reported that new reserved keyword offset in stanc3 is causing a problem https://github.com/stan-dev/stanc3/issues/453. I agree that this is a big problem as “offset” is likely name for variable, and there was no warning beforehand.
I think the list of new reserved keywords should be included in Release Notes for stan and all interfaces when they switch to stanc3. It would be good to make a separate posting warning that also people still using stanc2 should consider not using those reserved keywords. Also the list of reserved keywords in manual https://mc-stan.org/docs/2_21/reference-manual/variables-section.html should be updated.
I’m in a hurry, so I posted here instead of making issues for all the repos. I think the priority would to to post the new reserved keywords to discourse. Who can make such a list?
This is the full list of keywords that will trigger the “Identifier ‘%s’ clashes with reserved keyword.” error. That and all variable names with a “__” suffix. But I think the latter was already in place for the existing parser.
I think it dawns on me why this is useful: “offset” is one of our keywords which allows shifting of the internal sampler unconstrained state. Together with the “multiplier” keyword this enables very sleek coding of non-centred parameterisations… at the cost of blocking the words themselves, f course.
I’m just wondering why a change in language design was not included in any Release Note with very big letters.
stan 2.19.0 release notes mention offset/multiplier transformations for easier non-centering, etc, but not that language design was changed, and I think it didn’t until 2.22.
CmdStan 2.22.0 release notes say
As of version 2.22, CmdStan has switched to the new Stan-to-C++ compiler, called stanc3. This compiler is intended to be backwards compatible with the existing Stan language and should accept all models that compile under the release 2.21 compiler.
Nothing about changed language design, and explicitly stating should accept all models that compile under the release 2.21 compiler. which turned out not be true.
Could whoever is responsible for release notes to go and add the information about changed language design, please? We’ve had 2.22.0 and 2.22.1 releases but they can’t be advertised as the relevant information about what has changed is not easily available.
Cmdstan README.md links to the list of changes wiki and we could also link to this wiki in the release notes and
extend the wiki with the information on offset and multiplier being reserved keywords. Would that work?
I think we should not drift to a blame game. It states “should” parse all old models, so given the complexity of all of this it’s well understandable that we have glitches.
Augmenting the release notes, improving the doc and the parser Error messages are good ways forward.
(I would never dare to rewrite the parser, so I have great respect for that effort)
I’m sorry if my post could be interpreted that way. I tried intentionally to avoid it. I’m still curious what is the current release process, why did it fail and how it could be improved? I hope it’s not considered bad attitude to criticize processes? I have not been making Stan releases so I don’t know what kind of check list is used for the releases, but I would suggest adding item for “did language specification change” if it’s not yet there.
Agree completely and I’ve been clicking like for all those messages :)
No, not at all. It’s all about the tone of it… and your post was not offensive, I just thought it sounds a bit like blaming and I wanted to move it away from it.
The new parser is a massive change…and the reason why we offer atm that you can have the old parser to be build.
We should have clear notes on new reserved words. That’s not a language design change so much as a consequence of our adding new reserved identifiers.
How do you think we should deal with this? We could up the major version number every release, but that seems counterproductive for this kind of change.
The problem is tracking the effects of all the merged PRs. It’s way too intensive to go through all of the PRs in detail at release time. The only fix I can imagine is creating release notes along with each PR that gets merged.
I’m responsible for the phrase should accept all models, it reflects my ignorance about stanc3 - I will correct the release notes and add more to the CmdStan wiki about changes to the language, unless there’s a better place to document this.
on the one hand, the release didn’t go smoothly because there wasn’t enough communication and planning around issues of packaging and documentation of the new stanc3 compiler for cmdstan. on the other hand, everyone who participated in the release process worked really hard and really well together to address these issues and several good ideas have been proposed:
heads up to devs 2 weeks before the release
putting together RC builds before the release
I’ve been working with @rybern to better understand what’s in the new parser and will continue to add to the CmdStan documentation and wiki pages.
Could we establish a “NEWS.md” or “RELEASE-NOTES.md” file that each PR has to modify (add a line under the Development versions) and its the reviewers responsibility to check that happens. Example: Notify parallel (or other pkg) about SIGCHLD by gaborcsardi · Pull Request #237 · r-lib/processx · GitHub
On release “Development version” becomes Repo version X.Y.Z
I think its the cleaner and easier to handle than sections in PR or issue comments.
We can also use the current release-notes.txt and move it to the .md format.
It might be worth a try for the next release and we then revisit it after.