Skip to content
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

Update actions #769

Merged
merged 7 commits into from
Sep 27, 2024
Merged

Update actions #769

merged 7 commits into from
Sep 27, 2024

Conversation

seabbs
Copy link
Contributor

@seabbs seabbs commented Sep 13, 2024

Description

This PR closes #.

Initial submission checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have tested my changes locally (using devtools::test() and devtools::check()).
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
  • I have followed the established coding standards (and checked using lintr::lint_package()).
  • I have added a news item linked to this PR.

After the initial Pull Request

  • I have reviewed Checks for this PR and addressed any issues as far as I am able.

sbfnk
sbfnk previously approved these changes Sep 13, 2024
@seabbs
Copy link
Contributor Author

seabbs commented Sep 13, 2024

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

@sbfnk
Copy link
Contributor

sbfnk commented Sep 16, 2024

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
Copy link
Contributor Author

seabbs commented Sep 16, 2024

Testing a dummy PR in #773

@seabbs
Copy link
Contributor Author

seabbs commented Sep 16, 2024

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
Copy link
Contributor

sbfnk commented Sep 16, 2024

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
Copy link
Contributor Author

seabbs commented Sep 16, 2024

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
Copy link
Contributor

sbfnk commented Sep 16, 2024

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

@seabbs
Copy link
Contributor Author

seabbs commented Sep 16, 2024

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

@seabbs
Copy link
Contributor Author

seabbs commented Sep 16, 2024

#773 does pass so it is definitely something in this PR

@seabbs
Copy link
Contributor Author

seabbs commented Sep 16, 2024

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
Copy link
Contributor Author

seabbs commented Sep 16, 2024

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 sbfnk mentioned this pull request Sep 19, 2024
7 tasks
sbfnk
sbfnk previously approved these changes Sep 19, 2024
@sbfnk
Copy link
Contributor

sbfnk commented Sep 19, 2024

This is very confusing. Ubuntu installs rstan 2.32.6 whereas the dev version was installed in #775

@jamesmbaazam jamesmbaazam removed their request for review September 19, 2024 16:27
@jamesmbaazam
Copy link
Contributor

Removing myself as reviewer since Seb is reviewing.

@seabbs
Copy link
Contributor Author

seabbs commented Sep 20, 2024

Removing myself as reviewer since Seb is reviewing.

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

@sbfnk
Copy link
Contributor

sbfnk commented Sep 20, 2024

@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
Copy link
Contributor Author

seabbs commented Sep 20, 2024

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
Copy link
Contributor Author

seabbs commented Sep 20, 2024

maybe using the gh package

@seabbs
Copy link
Contributor Author

seabbs commented Sep 20, 2024

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

@sbfnk
Copy link
Contributor

sbfnk commented Sep 20, 2024

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

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

@sbfnk
Copy link
Contributor

sbfnk commented Sep 20, 2024

maybe using the gh package

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

@seabbs
Copy link
Contributor Author

seabbs commented Sep 20, 2024

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
Copy link
Contributor Author

seabbs commented Sep 20, 2024

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
Copy link
Contributor

sbfnk commented Sep 20, 2024

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
Copy link
Contributor

sbfnk commented Sep 20, 2024

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
Copy link
Contributor Author

seabbs commented Sep 21, 2024

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

That is a good idea

sbfnk
sbfnk previously approved these changes Sep 27, 2024
@sbfnk
Copy link
Contributor

sbfnk commented Sep 27, 2024

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

@sbfnk
Copy link
Contributor

sbfnk commented Sep 27, 2024

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

@sbfnk sbfnk added this pull request to the merge queue Sep 27, 2024
Merged via the queue into main with commit 95c3643 Sep 27, 2024
10 checks passed
@sbfnk sbfnk deleted the update-actions branch September 27, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants