Issue 385: GitHub Action to check compilation and correctness of cmdstan syntax.

[seabbs]

Description

This PR closes #385.

As discussed in #385 this PR adds an action to check the correctness of cmdstan code on main against the latest version of cmdstan. It is setup to run on a weekly schedule and to also run if stan code is updated on main or on a PR to main or develop. I've tested this locally interactively and am just running via act to test the GitHub Action components.

@Bisaloo or @sbfnk would likely be good fits to review this PR.

Note: I think this add something on top of compiling the model in the Rmd check but I could be mistaken.

Checklist

• My PR is based on a package issue and I have explicitly linked it.
• I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
• I have read the contribution guidelines.
• I have tested my changes locally.
• I have added or updated unit tests where necessary.
• I have updated the documentation if required.
• My code follows the established coding standards.
• I have added a news item linked to this PR.
• I have updated the package development version by one increment in both NEWS.md and the DESCRIPTION.
• I have reviewed CI checks for this PR and addressed them as far as I am able.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f448548) 96.84% compared to head (b10632d) 96.84%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #386   +/-   ##
=======================================
  Coverage   96.84%   96.84%           
=======================================
  Files          15       15           
  Lines        1873     1873           
=======================================
  Hits         1814     1814           
  Misses         59       59           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Bisaloo]

• Could you try to get to workflow to fail in a new branch or in a commit you revert after?

• Note: I think this add something on top of compiling the model in the Rmd check but I could be mistaken.

  I don't know much about stan but it seems strange that invalid syntax would compile and not throw any errors in R CMD check. However, even if this check ends up being redundant with R CMD check, it doesn't mean it shouldn't be added but it should be considered as a smoke test.

• There is a pedantic argument to check_syntax() which seems great and would probably indeed add an extra layer on top of what R CMD check is probably already doing: https://mc-stan.org/cmdstanr/reference/model-method-check_syntax.html

[seabbs]

Could you try to get to workflow to fail in a new branch or in a commit you revert after?

Yes good idea.

that invalid syntax would compile and not throw any errors in R CMD check. However, even if this check ends up being redundant with R CMD check, it doesn't mean it shouldn't be added but it should be considered as a smoke test.

It comes down to how invalid it is. Sometimes there are depreciation warnings that don't fail but yes in the case where this isn't the case it should error on compilation.

which seems great and would probably indeed add an extra layer on top of what R CMD check is probably already doing

I agree and I was also excited about this. Unfortunately, it currently throws a whole bunch of false positives due to our use of functions. Potentially we could get around this if we use snapshot tests (i.e accept the current false postivites) or in some other better way.

[Bisaloo]

Thanks for the explanation! This does sound useful once it has been confirmed that deprecation warnings are indeed caught while they wouldn't be with R CMD check.

Could you add comments for the points you mentioned please?

I agree and I was also excited about this. Unfortunately, it currently throws a whole bunch of false positives due to our use of functions. Potentially we could get around this if we use snapshot tests (i.e accept the current false postivites) or in some other better way.

It would be great to have a note about this in the workflow file, next to the check_syntax() call.

It comes down to how invalid it is. Sometimes there are depreciation warnings that don't fail but yes in the case where this isn't the case it should error on compilation.

This could also be a comment in the workflow file, or part of a README.md for the .github/workflows/ folder.

[seabbs]

or part of a README.md for the .github/workflows/ folder.

We don't currently have one of these but we could and should.

In 57d9855 I added comments expanding on my replies here.

[seabbs]

Just working up a failing example.

[seabbs]

See #394 for a test PR that should capture soft depreciations. Currently testing locally using act.

[seabbs]

Another test PR that should pass #396

[seabbs]

Wow I made that seem hard. @Bisaloo #394 is now failing as expected (old version of cmdstan and something that has been depreciated) and #396 is passing as expected (current version with no depreciated features).