Cx Organization

[JuanPedroGHM]

Due Diligence

• General:
  • title of the PR is suitable to appear in the Release Notes
• Implementation:
  • unit tests: all split configurations tested
  • unit tests: multiple dtypes tested
  • NEW unit tests: MPS tested (1 MPI process, 1 GPU)
  • benchmarks: created for new functionality
  • benchmarks: performance improved or maintained
  • documentation updated where needed

Description

Goal is to organize our workflows by modularizing certain steps and jobs.
Reusable workflows should be turned into actions, that can be added onto larger workflows.

Anger towards #1937 made me finish this out of spite. It does all the changes proposed. Let's talk about what's missing.

Benchmarks are also working again.

What are we getting from this

• More detailed test reports in gitlab and codecov (allegedly)
• Actions that are easier to maintain and workflows that are easier to plan.

TODO

• Clean-up secrets and old tokens
• Change GH_TOKEN, CB cannot comment on pr's at the moment.
• Restore full CI runs (after pr approved, and on main?)
• Somehow test the weekly workflow.
• Make sure special cases CI cases (support) are somehow handled properly.

See #1874 Cx section for rationale.

Changes proposed:

• Weekly actions packed into weekly workflow
• A single Github CI Action
• A single Codebase CI Action
• A single Benchmark trigger action
• A PR open/update workflow
• A PR approve workflow
• A push to main workflow
• NO MORE THANKING YOU FOR THE PR!

[github-actions]

This pull request is stale because it has been open for 60 days with no activity.

[brownbaerchen]

What is the status of this PR? Is this still relevant after #2139? Some things like removing thanking for PR are certainly still relevant but may be better merged separately.

[brownbaerchen]

Thanks for cleaning up all this stuff!

I left a few comments, but I have one larger issue outside of that, which is that no tests currently run on push to non-main branches, right?
I really like running the CI on push to any branch because I don't always run the entire test suite locally. Most of the time, I just run the tests I am working on and I know that I will get an email if I messed up something else. Or maybe the tests pass on my machine and not on GitHub or whatever. Or there is a new optional dependency that I have yet to install. Or maybe I just forgot to check different number of tasks.
I think it's really nice to be able to run the CI on push on my fork before doing a PR because then I can fix failures without sending out a bunch of emails to everybody.

Unfortunately, this is a bit tricky to implement in this framework because we don't want duplicate runs when pushing to a branch within the main repository that is currently part of a PR. There needs to be some check that the CI on push is skipped if the repository is not a fork and if the branch is currently part of a PR or something like that.

What's your opinion on this?

[brownbaerchen]

Note that this will then only check with the additional optional dependencies installed, which I think is fine. Actually, I have suggested this myself in #2196, but @mtar wanted to keep a run with no optional dependencies. I am indifferent as long as they run concurrently.

[JuanPedroGHM]

Hi @brownbaerchen, I forgot how quickly you answer reviews. There were some changes that needed to be tested. But I think the current version now addresses most of the issues you mentioned.

• push_other.yml is meant to run only on branches without a PR or forks.
• Dependencies have been changed to .[dev]
• Restored the full CI matrix for pushes to main and stable.
• Using uv for caching and faster dependency installation.
• Restored the push_support action for testing newer pytorch versions.

[brownbaerchen]

This is great, thanks!

I don't really see the point in the quick CI, to be honest. To me, the current lean CI is fast enough. So I would stick to the current medium and full, while renaming medium to quick just to keep the workflows simpler.
But I am also fine with what the state is now. You (or any other reviewer) choose.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.71%. Comparing base (991f913) to head (46b29a4).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1884   +/-   ##
=======================================
  Coverage   91.71%   91.71%           
=======================================
  Files          86       86           
  Lines       14221    14221           
=======================================
  Hits        13043    13043           
  Misses       1178     1178           

Flag	Coverage Δ
unit	91.71% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[brownbaerchen]

We need to merge #2262 before this one in order to run the optional IO tests. That's probably also why coverage is reduced currently..

[JuanPedroGHM]

I fixed the issues with the access token, wrote more documentation on the CX repository, and added the necessary setting for the support new pytorch version workflow. The last pipeline ran without any issue. Ready to merge in my opinion, @brownbaerchen @mtar.

[brownbaerchen]

I actually noticed a few things I hadn't before:

• We should trigger more CI on PR open and synchronize
• We may want to limit CI runs on dependabot PRs, but not as much as we are currently doing

Then again, I may be misunderstanding some stuff. So feel free to dismiss the comments if they don't make sense.

[brownbaerchen]

One issue that came up is the update of the setup-mpi action. The dependabot PR doesn't trigger any CI action in the current main. Luckily, we know there are issues and so I ran the CI on this manually and indeed it failed. had we not known that, I would want to know at least when the PR is merged into main. So maybe always run the CI on push to main? Not sure what's the best way of handling this..
Maybe we should bring back the medium CI on dependabot PRs. The quick one would pass with the setup-MPI thing.

The suggestion diff is messed up. I want to remove all paths, not just the last one.

Suggested change

- 'pyproject.toml'

[JuanPedroGHM]

This would get tested anyway on any push to main, which should be enough for most dependabot updates. For the special cases, like the setup-mpi actions, triggering manually is fairly simple with a small commit.

Another option would be to just enable the quick tests when a PR is open, regardless of the source.

[brownbaerchen]

Is there a reason to only run the CI on push to main with changes to select paths?

[JuanPedroGHM]

Updates based on the review comments:

• On Push PR also triggers when opening/reopening a PR, to get instant tests.
• On Push PR, the github tests are closer to the medium tests, testing the oldest and newest python and pytorch versions, without any dependencies. It makes sense to do this, we would rather not find the compatibility issues after a merge to main.
• Tests will still not run dependabot PR's, I don't think it makes sense 99% of the time, but they can be triggered manually by creating a "fake" commit if we feel the need to test it.

[mtar]

Some action SHA pinning for security. It's non-critical code though.

[brownbaerchen]

Is there a reason to only run the CI on push to main with changes to select paths?

[JuanPedroGHM]

@brownbaerchen From your last comment: changes to github workflows will be tested on PR Open/Update (unless they have dependabot/** branch), to ensure that they work (just like the ones here), but I would say, once they work on the PR, there is no need to test them on main. Many dependabot merges won't clog the CI anymore. Same principle applies to documentation changes, or other non-source code changes.