Skip to content

Update CI for individual package testing combinations - #249

Merged
nHackel merged 10 commits into
masterfrom
nh/ci
Aug 1, 2025
Merged

Update CI for individual package testing combinations#249
nHackel merged 10 commits into
masterfrom
nh/ci

Conversation

@nHackel

@nHackel nHackel commented Jul 31, 2025

Copy link
Copy Markdown
Member

This PR reworks the CI setup of MRIReco and its subpackages to allow us to easier judge if individual packages can be released.

In the current setup we run all tests with the latest version and a checkmark thus only tells us if that specific combination is "okay", but won't tell us for example if a latest version of MRISimulation still works with the released version of MRIOperators.

In the new setup each subpackage has it's own Github Action, which is triggered on changes in the respective subfolders. This action should use the latest version of the specific package with the "normal" registered version of its dependencies.

Additionally, I've still kept an Action for "everything", MRIReco_All action (I am open to name suggestions 😄), which produces both the documentation as well as the tests of all packages with all latest committed versions.

In the future I'd like to add some additional features to this setup:

  • Breakage Tests: Does the latest version of subpackage X break its upstream dependencies, i.e. do changes to MRIBase break MRIOperators, ... and so on or do changes to MRIOperators break MRISimulation and MRIReco. This is essentially what MRIReco_All does atm, but from the perspective of the specific subpackages
  • We could now create and host documentation for the individual subpackages which the "everything" action just aggregates with MultiDocumenter

If you, @tknopp, @aTrotier, @cncastillo, @atsanda, @JakobAsslaender, have any more ideas or wishes for the CI, please let me know. I am not able to contribute much at the moment in terms of documentation or content/features, but feel free to ping, write or email me in regards to CI and also releases.

Also since CI is a bit tedious to test, please let me know if what I described in this comment is not what's actually happening.

Known Issues:

MRICoilSensitivies will have a broken CI in this setting. From what I can tell it requires two releases to fix this: It firstly requires a new release of MRISimulation with new compat entries for MRIOperators. Secondly it requires a new release of MRIBase for the 2dEnc keyword.

MRIOperators is in a bit of a tricky situation. Its tests depend on MRISimulations for their fieldmap functions, however MRISimulations also depends on MRIOperators. This will cause issues if MRIOperators version is ahead of what is allowed MRISimulations compat entry. One solution would be to move the Fieldmap.jl from Simulation to Operators, however I don't know what other impacts this will have. As a workaround I know dev both MRIOperators and MRISimulations in MRIOperators CI action

@nHackel nHackel changed the title WIP: Update CI for individual package testing combinations Update CI for individual package testing combinations Aug 1, 2025
@nHackel

nHackel commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

As a follow up I will also perform some individual releases, mainly MRIOperators. If I am unsure if something is in a releasable state, I'll make a new issue and ping people who have contributed with questions I'd need to make such a decision😄

@nHackel
nHackel merged commit 6194aec into master Aug 1, 2025
7 of 8 checks passed
@nHackel

nHackel commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

Oh and please let me know if I break something with the releases 🙈 I'll try to check the combinations, but something might slip through

@aTrotier

aTrotier commented Aug 1, 2025

Copy link
Copy Markdown
Contributor

Great work.

Right now, the only suggestion I have is to wrote a new section in the documentation which explains how to contribute to the package (how to dev and so on) especially if we want to mimic the CI locally.

Something like this KomaMRI section : https://juliahealth.org/KomaMRI.jl/stable/how-to/5-contribute-to-koma/

@nHackel

nHackel commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

The workflows for a new PR now look something like this:

image

Which tests exactly the case Jakob found in #244, i.e. the latest version of MRICoilSens. with currently released versions of MRIBase. So that test would have found that problem

@nHackel

nHackel commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

@aTrotier I've assigned myself two documentation issues, one for GPU support and one for the contributors guide. I'll try to get around to those this month

@cncastillo

Copy link
Copy Markdown
Contributor

Great work.

Right now, the only suggestion I have is to wrote a new section in the documentation which explains how to contribute to the package (how to dev and so on) especially if we want to mimic the CI locally.

Something like this KomaMRI section : juliahealth.org/KomaMRI.jl/stable/how-to/5-contribute-to-koma

Feel free to copy it and adapt it for MRIReco 😄!

@nHackel
nHackel deleted the nh/ci branch August 1, 2025 18:20
@nHackel

nHackel commented Aug 2, 2025

Copy link
Copy Markdown
Member Author

I have also updated code coverage reporting to consider the subpackages. And I have split the "everything" job into the respective packages with PR #260 :

image

Now we have potentially two "actions" per package:

  1. Test package X with its released dependencies. This action is triggered by changes to X and can detect if we need new releases/changes in X dependencies and if our changes broke something in X (or rather its tests)
  2. Test package X with its "latest" dependencies, i.e. with the versions in the current commit. This action is always run and so if the first action is broken, we can detect a fix in a dependency Y with this action. This is essentially the previous state.

We are now missing a last action, which checks that if we change X, did we break a release that depends on it. This is actually the case Jakob found #244. However, due to missing tags I can't checkout the last release of Y. With the new setup, we would only notice if we touch Y again via action 1. (Edit: I think I have a proper solution for this)

One workaround I could implement is a scheduled action which once a week tests all released versions of our packages.

Other than that last case, I think we gained a better structure. And it's hopefully easier to release subpackages now. I recommend we also try to update per package CHANGELOG.mds at the end of each PR

@nHackel

nHackel commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

I was overthinking the breakage tests and now we should have those too 😄

@cncastillo

Copy link
Copy Markdown
Contributor

Btw, if you want a badge for the code coverage of each package, you can submit the code coverage report using a flag:

Then all the results get aggregated to the correct package, and it even works for the GPU tests. Sadly the GPU kernels are not properly detected by the codecoverage, so you need to exclude them manually (with # COV_EXCL_START and # COV_EXCL_STOP):

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