epiforecasts / EpiNow2

Estimate Realtime Case Counts and Time-varying Epidemiological Parameters
https://epiforecasts.io/EpiNow2/dev/
Other
116 stars 33 forks source link

Update actions #769

Closed seabbs closed 2 months ago

seabbs commented 2 months ago

Description

This PR closes #.

Initial submission checklist

After the initial Pull Request

seabbs commented 2 months ago

I'm seeing simulate_secondary and simulate_infections failing here. I can't see how this can be related to this PR?

sbfnk commented 2 months ago

I'm seeing simulate_secondary and simulate_infections failing here. I can't see how this can be related to this PR?

Me neither. How very odd.

seabbs commented 2 months ago

Testing a dummy PR in #773

seabbs commented 2 months ago

One potential factor here is could this have a newer version of cmdstan or cmdstanr? If yes perhaps the recent change in the random number generator explains this?

sbfnk commented 2 months ago

One potential factor here is could this have a newer version of cmdstan or cmdstanr? If yes perhaps the recent change in the random number generator explains this?

But the changes here shouldn't change the cmdstan(r) version and the tests pass on main.

seabbs commented 2 months ago

maybe they do because

  1. I am using the epinowcast action which auto-detects the latest cmdstan version vs fixing it (which is the thing that is hard to make work all the time)
  2. It is pulling from the non-depreciated cmdstanr source and maybe that gives a different version.

If its not that and if #773 works I can't imagine what it can be

sbfnk commented 2 months ago

But the windows action fails which doesn't use cmdstan?

seabbs commented 2 months ago

oh.... Any chance its getting rstan from the additional repo vs from CRAN and hence having version mismatch issues?

seabbs commented 2 months ago

773 does pass so it is definitely something in this PR

seabbs commented 2 months ago

The stan universe hosts the dev version of rstan which is 3 stan minor versions ahead of the CRAN version. That includes the random number gen change I believe so I think it has to be this.

seabbs commented 2 months ago

So we have a few options.. I am not sure how to control which package is installed from which repo via additional repositories. If we can do that then maybe that is the solution?

sbfnk commented 2 months ago

This is very confusing. Ubuntu installs rstan 2.32.6 whereas the dev version was installed in https://github.com/epiforecasts/EpiNow2/pull/775

jamesmbaazam commented 2 months ago

Removing myself as reviewer since Seb is reviewing.

seabbs commented 2 months ago

Removing myself as reviewer since Seb is reviewing.

Sorry this was an intentional ping @jamesmbaazam as seb has now committed code

sbfnk commented 2 months ago

@seabbs just flagging the failure of the epinowcast cmdstan action in the merged PR which presumably will happen here too https://github.com/epiforecasts/EpiNow2/actions/runs/10956025463/job/30421154764

seabbs commented 2 months ago

its the cmdstan version lookup code. Really unclear why that is stochastically unstable. There must be a better way of getting the latest release

seabbs commented 2 months ago

maybe using the gh package

seabbs commented 2 months ago

if we set a version vs trying to find the latest one then that will go away

sbfnk commented 2 months ago

its the cmdstan version lookup code. Really unclear why that is stochastically unstable.

Presumably because we're not sending an auth token along?

sbfnk commented 2 months ago

maybe using the gh package

or just using the existing functionality for identifying / downloading the latest release in cmdstanr?

seabbs commented 2 months ago

or just using the existing functionality for identifying / downloading the latest release in cmdstanr?

Does it have this before it starts installing? We need it earlier to set the cache name

seabbs commented 2 months ago

Presumably because we're not sending an auth token along?

I'm not using the API at all at the moment. It sends its a call to curl: https://github.com/epinowcast/actions/blob/d7ff9df01f7b37f1cd366a6e8dc499c9accd13d0/install-cmdstan/scripts/get-latest-release.sh#L22

sbfnk commented 2 months ago

Presumably because we're not sending an auth token along?

I'm not using the API at all at the moment. It sends its a call to curl: https://github.com/epinowcast/actions/blob/d7ff9df01f7b37f1cd366a6e8dc499c9accd13d0/install-cmdstan/scripts/get-latest-release.sh#L22

Isn't that the problem then given it yields a 403, i.e. the server receives the request but refuses to process?

sbfnk commented 2 months ago

or just using the existing functionality for identifying / downloading the latest release in cmdstanr?

Does it have this before it starts installing? We need it earlier to set the cache name

I guess yes if you're willing to use non-exported functions such as e.g. cmdstanr:::latest_released_version() https://github.com/stan-dev/cmdstanr/blob/f2e152b88fde5c2cde01ff078d5715b3b6248628/R/install.R#L401 - but of course that might introduce different pain points. Alternative would be to use -H in curl to send the github auth token in the header.

seabbs commented 2 months ago

Alternative would be to use -H in curl to send the github auth token in the header.

That is a good idea

sbfnk commented 2 months ago

Finally managed to get this to work by deleting the install-cmdstan cache.

sbfnk commented 2 months ago

@jamesmbaazam just needs your approval then should be good to merge