search

CDCgov / ww-inference-model

An in-development R package and a Bayesian hierarchical model jointly fitting multiple "local" wastewater data streams and "global" case count data to produce nowcasts and forecasts of both observations
https://cdcgov.github.io/ww-inference-model/
Apache License 2.0
16 stars 2 forks source link

add multiple os to matrix strategy #190

Closed kaitejohnson closed 2 weeks ago

kaitejohnson commented 2 weeks ago

Attempting to test on ubuntu, windows, and mac...

github-actions[bot] commented 2 weeks ago

Thank you for your contribution, @seabbs :rocket:! Your page is ready to preview here

seabbs commented 2 weeks ago

Run Rscript -e 'cmdstanr::check_cmdstan_toolchain(fix = TRUE)' Installing mingw32-make with Rtools40 The C++ toolchain required for CmdStan is setup properly! Run Rscript -e 'cmdstanr::install_cmdstan(version = "2.35.0", cores = 2)' Error: Error: unexpected numeric constant in "cmdstanr::install_cmdstan(version = 2.35.0" Execution halted Error: Process completed with exit code 1.

The remaining issue is with R 4.1 and is very odd. It looks to be due to the way that windows on that version of R only is interpreting strings?

seabbs commented 2 weeks ago

Note this is currently pinned to a fixed release but needs to be changed back to the rolling release once that has been updated

seabbs commented 2 weeks ago

@kaitejohnson I think cmdstanr wise this is now working and is "stable". The mac dependency installation issue is a odd one. I haven't seen the package that is causing the issue before.

kaitejohnson commented 2 weeks ago

Can you describe briefly what you changed here @seabbs ?

seabbs commented 2 weeks ago

I updated the rolling release of install-cmdstan to use the gh cli with a GITHUB secret rather than using the API directly with a API secret. I also switched to using javascript rather than custom bash and powershell across OS's to increase stability.

The last change I needed to make in order to get this working with R 4.1 and windows (i.e it already worked on the latest release) was to be careful about the use of " and ' quotes. Apparently windows sometimes treated these interchangeable (but only when used in one order).

Its not relevant here but I also added support for installing cmdstanr in the action if its not present in the calling action. I tried to do this for R but couldn't get it to only install R if not present vs all the time (mostly due actually needing to try and detect the user having called setup-r in the wrapper step vs R (as the built in R can't be used to install packages).

The code changes here were almost entirely exploratory and should now be mostly set back to what you had. I found this still failed on mac but that this failure was unrelated to cmdstan (its something in your dependencies that I have never seen before).

Note this does highlight something to be aware of install-cmdstan@v1 is a rolling release vs static. This means it changes as long as the changes are expected to be non-breaking. This is useful as allows us to fix issues downstream without action users being aware but imposes a small danger of code injection here via that action updating. That means you need to trust the maintainers of the action (here epinowcast). This is the same as the r actions which also use rolling releases and so could enable code injection in the same way.

To avoid this exposure you could use a fixed release (i.e this current one is 1.5) the downside of that is you will need to update (or use dependeabot to update) as new releases are made. This would only avoid it if an attacker didn't just change a static release (which they could if they had enough permissions on the home repo). I am currently unaware of better practice here but happy to hear what it is and implement.

as a final note I see floating not completed required checks. I think this is because your branch protections are out of date vs the current action names

kaitejohnson commented 2 weeks ago

Wow thank you @seabbs. I'll pretend I understood 50% of that.

I think I am in favor of the rolling release so we internally know when breaking changes are introduced (if I am understanding correctly), even

Looks like the ruleset names got updated because these all ran.

seabbs commented 2 weeks ago

Screenshot 2024-10-02 at 15 42 14

This is what I am talking about in terms of out of date branch protections. These will wait for ever as the actions no longer exist I think?

seabbs commented 2 weeks ago

Note also that all windows VM seem very slow at downloading cache objects which makes them run slower than they otherwise would. This is new behaviour as far as I am aware and so will hopefully be transient.

kaitejohnson commented 2 weeks ago

@dylanhmorris I now can't seem to change the names of the checks anywhere -- I thought it was in the ruleset but it is not an option I am seeing...

dylanhmorris commented 2 weeks ago

@dylanhmorris I now can't seem to change the names of the checks anywhere -- I thought it was in the ruleset but it is not an option I am seeing...

I already did that, but for future reference:

Settings > Rules > Rulesets > (click on the ruleset to modify)

then scroll to "Branch rules"; find "Require status checks to pass", and click on "Show additional settings"