Sync periodic tests with the push workflow

[kratman]

Description

Syncs the test on push workflows with the periodic tests.

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[kratman]

This needs some more work before it will be ready for review. I just pushed it in advance as a reminder to myself for the weekend.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.55%. Comparing base (e22d10c) to head (ec650c8).
Report is 222 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4172   +/-   ##
========================================
  Coverage    99.55%   99.55%           
========================================
  Files          288      288           
  Lines        21856    21856           
========================================
  Hits         21759    21759           
  Misses          97       97           

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

[agriyakhetarpal]

Thanks, @kratman – I know see that you mentioned needs some work, but I am not very supportive of these changes in general. Two reasons:

1. We want to avoid the caching stuff in the scheduled tests so as to always download newer versions packages from PyPI and foresee things in advance if they are going to fail due to upstream changes. The scheduled tests are meant to fail to catch things early (same as the philosophy for dependents that will make use of the nightly releases in Add CI jobs to upload nightly wheels to Anaconda #3945).
2. I opine that the integration and the unit tests in this workflow should stay in parallel jobs (they were very recently split up in Issue 4082 Documentation CI Fix #4102 to run in parallel), because the run time for a nightly job is not a concern. If they run in serial, it's not possible to find any potential failures in the integration tests if the unit tests fail.

I guess a better way of refactoring this would be to create a setup-pybamm.yml workflow that runs on the workflow_call: event which can be reused both in PR tests and the scheduled tests (it can take cache as a string-based workflow input that can be turned on or off based on where the workflow is triggered from).

[kratman]

@agriyakhetarpal I started this because the periodic tests were failing, so I started by syncing the workflows. If things like caching are problematic then I can remove them from this workflow as I go. The goal is to get the periodic tests running smoothly

[agriyakhetarpal]

Okay, makes sense, thanks for getting this done. I can review after this is ready of course

[kratman]

This seemed to run locally besides the mseries block which only runs on the main pybamm repo. Not sure if that CI job is even needed anymore now that we have the macos-13 and macos-14 jobs

[kratman]

I did a quick fix for a lychee issue, but it just exposed more issues with links from tinyurl. I will address those elsewhere

[kratman]

Remaining lychee issues are fixed by #4219

[agriyakhetarpal]

The integration and the unit tests in this workflow should stay in parallel jobs because the run time for a nightly job is not a concern. If they run in serial, it's not possible to find any potential failures in the integration tests if the unit tests fail.

The changes here look good to me. I still feel that the above point applies, though. Creating a reusable workflow with inputs for disabling/enabling etc. that responds to a workflow_call: event, or separating CI jobs into different files by either OS or the type of the test suite (unit, integration), etc. seem to be the better approaches and are similar to how they are structured in other projects I have looked at

[kratman]

The integration and the unit tests in this workflow should stay in parallel jobs because the run time for a nightly job is not a concern. If they run in serial, it's not possible to find any potential failures in the integration tests if the unit tests fail.

Yeah I figure that making them match brings us closer to using reusable workflows. This makes sure that the same test procedures are used for the regular PRs and the periodic tests. I think the reusable workflow and additional configuration can be revisited in the future. Our infrastructure is already quite complicated so the simpler we can make it the better

[kratman]

@agriyakhetarpal Is this good to go then?

[agriyakhetarpal]

I feel the reusable workflow is easy enough to be added here itself before we merge, but if you wish to do it in a follow-up after this PR, I shall approve it

[kratman]

I feel the reusable workflow is easy enough to be added here itself before we merge, but if you wish to do it in a follow-up after this PR, I shall approve it

I created a followup ticket since I suspect that more can be turned into re-usable workflows. For now, this improves the scheduled tests, so I think I can safely revisit the reusable workflows