search

HopkinsIDD / flepiMoP

The Flexible Epidemic Modeling Pipeline
https://flepimop.org
GNU General Public License v3.0
9 stars 4 forks source link

Split `ci.yml` into separate actions #280

Closed TimothyWillard closed 1 month ago

TimothyWillard commented 3 months ago

Split the "unit-tests" action into multiple actions, currently one for each package contained within the flepiMoP repo. Also updated checkout from v3 to v4 to address node16 deprecation warnings and swapped ubuntu 20.04 for ubuntu latest. Changed the gempyor CI to not print stdout and exit on first failure.

See GH-278.

TimothyWillard commented 3 months ago

That did not do quite what I expected. Let me make a few small edits before this is ready for review.

TimothyWillard commented 3 months ago

These changes are now ready for review.

TimothyWillard commented 3 months ago

Ah, I did not realize that we had sunsetted the breaking-improvements branch, but that makes sense. I kept the dev branch, but I also don't see that as a branch on GitHub so should I remove that as well?

jcblemai commented 3 months ago

Look good to me, thanks. As mentionned, perhaps the verbosity and all tests with failure were useful as I know from the commit log we have folks using ci as way to test code. But I agree it's better to not have them in Github action, and we can add it back.

the splitting is very welcome.

TimothyWillard commented 1 month ago

@jcblemai I've added python 3.11 to the gempyor CI in addition to 3.10 per the discussion at the flepiMoP dev meeting this morning. Would be nice to get this merged in sooner rather than later.

jcblemai commented 1 month ago

Thanks, This is ready to merge for me! Nice bit for the matrix strategy, that's one less use of our docker image and will be useful to make sure we don't break production environments (e.g this commit: https://github.com/HopkinsIDD/flepiMoP/pull/311/commits/7e2a061d8de899c32372f769975324ae43e2e62d that i plan to remove after the current are done, was necessary to run on one of my longleaf env).

TimothyWillard commented 1 month ago

Looks like it is not easy to force the inference-ci workflow to run on non-default branches, hence why it's not triggering here as expected: https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#workflow_run.

Note: This event will only trigger a workflow run if the workflow file is on the default branch.

TimothyWillard commented 1 month ago

~@shauntruelove whenever you have a chance could you review again? The change request is blocking the ability to merge.~

Edit: I realized there are some issues with how the R package CI works that I'm addressing now.

TimothyWillard commented 1 month ago

This PR is ready for review again. This PR now:

  1. Splits the CI action into separate actions for gempyor, flepicommon and inference. Notably, this excludes flepiconfig which is also not included in the current CI action and does not have a passing test suite (see GH-350).
  2. This runs the GitHub actions against explicit R and python versions, which clarifies the environments that we support. For now R 4.3.3 and Python 3.10 and 3.11.
  3. Uses caching to speed up the R package environment setup. These caches are reset weekly, but this can be adjusted if this is painful on a Monday. It doesn't do the same for python since that is quick to do, but could also be done in the future if we see slow downs in setup speed.

Some open questions:

  1. I kept the dev branch, but I also don't see that as a branch on GitHub so should I remove that as well?
  2. I suspect there will be some repo settings changes required to accommodate this (see how it is expecting an action called "unit-tests" to run below). Who should I work with to accomplish this?

Steps after this PR:

  1. Investigate switching the environment setup to use conda instead of doing it manually after GH-329.
  2. Incorporate flepiconfig into this set of GitHub actions.
  3. Ensure that the workflow chaining works as expected, see: https://github.com/HopkinsIDD/flepiMoP/pull/280#issuecomment-2403285763.
TimothyWillard commented 1 month ago

Removed the whitespace changes, but I it would be helpful to get answers to the above open questions before merging.

jcblemai commented 1 month ago

Still approve, but also still some weird whitespace diffs?

where @pearsonca ?

TimothyWillard commented 1 month ago

Still approve, but also still some weird whitespace diffs?

I'm also a bit confused about this one:

twillard@epid-iss-MacBook-Pro ~/D/G/H/flepiMoP (split-up-unit-tests-action)> git diff --name-only HEAD ( git merge-base main HEAD )
.github/workflows/ci.yml
.github/workflows/flepicommon-ci.yml
.github/workflows/gempyor-ci.yml
.github/workflows/inference-ci.yml