-
-
Notifications
You must be signed in to change notification settings - Fork 87
Added cmdstanr to GHA #399
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
The issue in the vignette ( |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #399 +/- ##
==========================================
+ Coverage 99.09% 99.13% +0.03%
==========================================
Files 35 35
Lines 5750 5750
==========================================
+ Hits 5698 5700 +2
+ Misses 52 50 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Yeah there's a part in that vignette where we want to show some diagnostic plots and we need enough divergences from HMC for the plot to make sense. So we have a check to make sure that happens. The problem is that on different systems the same seed can result in slightly different results (if hardware is different or toolchain is different, etc.). I haven't seen this happen in a while though. We might need to go in and try a different seed here:
|
Seems to still be skipping a test due to not finding cmdstanr: https://github.com/stan-dev/bayesplot/actions/runs/20016348876/job/57396448626?pr=399#step:7:195 |
|
Huh none of them are running, that's strange. I'll look into this |
|
It's also fine to just not use cmdstanr on GHA if this takes a while to figure out. Not the highest priority (if you're looking for things to do I can find other ones if this ends up being a pain). But if you are going to try adding cmdstanr to Suggests like in your latest commit then I think you'll also have to add the |
|
I think we're getting somewhere though--I think the last Windows run didn't skip any tests and code coverage did increase which I'm thinking (hoping?) is due to those extra tests running. |
|
I was running into an issue with rstan relying on a deprecated boost RNG (l'ecuyer), so for testing I bumped the DESCRIPTION's rstan version to 2.36 and everything works (except for the mac divergence thing) |
|
Seems like using |
|
Tests don't seem to be skipped either, is it ok to bump the rstan dependency @jgabry? It should be possible to not update it and just figure out dependencies but it'll be a bit more of a pain. 2.36 is agressive too, just needs to be a version which resolved this issue. |
|
The issue with bumping the RStan version in DESCRIPTION that CRAN only has 2.32.7. We can certainly bump the version to at least 2.32 but I'm not sure if that's sufficient (I guess we could force GHA to use a more recent version as long as we don't require that in DESCRIPTION). What I don't understand is why we're only having this issue with this PR. Other recent PRs passed all checks on GHA without issue. Any ideas? |
|
Oh right I forgot 2.32.7 was the latest CRAN release. Hm. I added the Stan R universe repository so I'm guessing that's changing the generated lockfile and some dependencies are getting pinned to incompatible versions. Should undo the description change and run pak locally to see what versions it pins. I also need to read into what exactly Does the R universe host old versions or just the latest release and devel binaries? If its the latter that's probably the issue since I'd hazard the newest cmdstanr version would force some other packages to be pinned to newer versions too |
I'm not entirely sure, but I think just the latest. |
|
If a new rstan CRAN version is going to be released that should fix the issues here, but I still not entirely sure why this is only happening now. |
Added Stan r-universe and cmdstanr as a dependency so some tests aren't skipped. Didn't touch description, so cmdstanr is not suggested or anything, just added in the builds so a suite of tests aren't skipped. (check in action logs?)
Used in both the R CMD CHECK and coverage actions.
Fixes #398.