Create dynamic GitHub Actions matrix to test against PyBaMM

[agriyakhetarpal]

Description

This PR adds a helper script that is invoked in the GitHub Actions workflows for the scheduled tests. This will extract the last three versions of PyBaMM from PyPI and test PyBOP against them according to the set CRON schedule.

This PR is currently a work-in-progress:

• Create matrix with PyBaMM versions as variables:

I have tested this and the script is working: https://github.com/agriyakhetarpal/PyBOP/actions/runs/7863984356

For 3 operating systems, 4 Python versions, and 3 versions of PyBaMM; this shall generate 36 individual jobs. The JSON output is as follows:

Output from running script – displayed as prettified and formatted JSON:

{
  "include": [
    {
      "os": "ubuntu-latest",
      "python_version": "3.8",
      "pybamm_version": "23.5"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.8",
      "pybamm_version": "23.9"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.8",
      "pybamm_version": "24.1"
    },
    {
      "os": "windows-latest",
      "python_version": "3.8",
      "pybamm_version": "23.5"
    },
    {
      "os": "windows-latest",
      "python_version": "3.8",
      "pybamm_version": "23.9"
    },
    {
      "os": "windows-latest",
      "python_version": "3.8",
      "pybamm_version": "24.1"
    },
    {
      "os": "macos-latest",
      "python_version": "3.8",
      "pybamm_version": "23.5"
    },
    {
      "os": "macos-latest",
      "python_version": "3.8",
      "pybamm_version": "23.9"
    },
    {
      "os": "macos-latest",
      "python_version": "3.8",
      "pybamm_version": "24.1"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.9",
      "pybamm_version": "23.5"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.9",
      "pybamm_version": "23.9"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.9",
      "pybamm_version": "24.1"
    },
    {
      "os": "windows-latest",
      "python_version": "3.9",
      "pybamm_version": "23.5"
    },
    {
      "os": "windows-latest",
      "python_version": "3.9",
      "pybamm_version": "23.9"
    },
    {
      "os": "windows-latest",
      "python_version": "3.9",
      "pybamm_version": "24.1"
    },
    {
      "os": "macos-latest",
      "python_version": "3.9",
      "pybamm_version": "23.5"
    },
    {
      "os": "macos-latest",
      "python_version": "3.9",
      "pybamm_version": "23.9"
    },
    {
      "os": "macos-latest",
      "python_version": "3.9",
      "pybamm_version": "24.1"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.10",
      "pybamm_version": "23.5"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.10",
      "pybamm_version": "23.9"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.10",
      "pybamm_version": "24.1"
    },
    {
      "os": "windows-latest",
      "python_version": "3.10",
      "pybamm_version": "23.5"
    },
    {
      "os": "windows-latest",
      "python_version": "3.10",
      "pybamm_version": "23.9"
    },
    {
      "os": "windows-latest",
      "python_version": "3.10",
      "pybamm_version": "24.1"
    },
    {
      "os": "macos-latest",
      "python_version": "3.10",
      "pybamm_version": "23.5"
    },
    {
      "os": "macos-latest",
      "python_version": "3.10",
      "pybamm_version": "23.9"
    },
    {
      "os": "macos-latest",
      "python_version": "3.10",
      "pybamm_version": "24.1"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.11",
      "pybamm_version": "23.5"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.11",
      "pybamm_version": "23.9"
    },
    {
      "os": "ubuntu-latest",
      "python_version": "3.11",
      "pybamm_version": "24.1"
    },
    {
      "os": "windows-latest",
      "python_version": "3.11",
      "pybamm_version": "23.5"
    },
    {
      "os": "windows-latest",
      "python_version": "3.11",
      "pybamm_version": "23.9"
    },
    {
      "os": "windows-latest",
      "python_version": "3.11",
      "pybamm_version": "24.1"
    },
    {
      "os": "macos-latest",
      "python_version": "3.11",
      "pybamm_version": "23.5"
    },
    {
      "os": "macos-latest",
      "python_version": "3.11",
      "pybamm_version": "23.9"
    },
    {
      "os": "macos-latest",
      "python_version": "3.11",
      "pybamm_version": "24.1"
    }
  ]
}

• Figure out a way to install PyBaMM with the specified version in the matrix via the nox configuration in noxfile.py
• Ensure that all tests pass for all generated jobs

Closes #123

[agriyakhetarpal]

Hi, @BradyPlanden – I would appreciate a review even though this is a work-in-progress PR before I add further changes – I am noticing that we have a lower bound on the PyBaMM version in setup.py as pybamm>=23.5, should that be relaxed or should we be testing against release candidates too? In my opinion, it would be better to test against stable versions and go forward with the former. Alternatively, we can run against the last three PyBaMM versions instead of four so that the 23.5 lower bound isn't broken and we can test against 23.5, 23.9, and 24.1. This shall reduce the generated jobs from 48 to 36.

I am currently figuring out a method to include the PyBaMM version in noxfile.py so as to download the PyBaMM version specified in the matrix; one way to do this is with environment variables, i.e., let nox detect something like PYBOP_SCHEDULED that can be set in the workflows and then add some custom logic – this is going to be a hacky way but it should work. Otherwise, I can pass something like PYBAMM_VERSION as an environment variable set to matrix.pybamm_version that nox can have access and perform the PyBaMM installation in the said context. But this doesn't alter the fact that pip will always download the latest version of PyBaMM available on PyPI, so we will have to add an explicit session.install("pybamm==PYBAMM_VERSION") step in each nox session after PyBOP has been installed in order to overwrite the installation of PyBaMM that comes with it (only for these scheduled tests, this can be skipped in the PR tests).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (59653d4) 94.56% compared to head (9572d8f) 94.57%.
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #193   +/-   ##
========================================
  Coverage    94.56%   94.57%           
========================================
  Files           44       44           
  Lines         1823     1826    +3     
========================================
+ Hits          1724     1727    +3     
  Misses          99       99           

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

[agriyakhetarpal]

I see that #187 is changing up some of the configuration to add GitHub Actions' M1 runners, so I am happy to either wait till that is complete or complete this earlier (there won't be any conflicts though).

Also, PyBaMM 24.1 brings Python 3.12 support, does the PyBOP team have plans to add that soon? I ask this because it would limit the matrix configuration here by a bit later on – PyBaMM versions earlier than 24.1 would have to be excluded from the jobs for Python 3.12 in that case owing to the lack of support.

[agriyakhetarpal]

In the last three commits, I have done exactly what I suggested above, i.e., setting PYBOP_SCHEDULED and PYBAMM_VERSION which seems to work in a neat manner, both locally and in CI – where it installs the correct PyBaMM version set in the matrix and runs tests against it, uninstalling the earlier rendition that came from setup.py. It should be up to you to decide on what should be the best scenario to go forward with.

The workflow run on my fork can be accessed here: https://github.com/agriyakhetarpal/PyBOP/actions/runs/7874163448. PyBaMM 23.5, 23.9, and 24.1 are being tested on Linux, macOS, and Windows – with Python versions 3.8–3.11.

Edit: I am not sure how to fix the failing tests though since I do not know how PyBOP works, feel free to take this onwards from here!

[BradyPlanden]

Excellent, thanks for the additions @agriyakhetarpal. This looks great.

We will need to sort out our failing tests for varying PyBaMM versions, but I think the right move is to pull the three most recent PyBaMM versions to test against. Any more than that and it becomes quite the challenge to maintain.

Also, PyBaMM 24.1 brings Python 3.12 support, does the PyBOP team have plans to add that soon?

Yes, I think with PyBaMM supporting 3.12, we should now be able to start testing against 3.12.

@martinjrobins, mind taking a look at this one before it's merged?

[agriyakhetarpal]

Thanks for the review, @BradyPlanden! Does this require a CHANGELOG entry? Also, I'm happy to rebase the changes here depending on if #188 gets merged first

[BradyPlanden]

Yes, please add a changelog entry. Thanks!

[agriyakhetarpal]

I just glanced at the failing tests for PyBaMM 23.5, here: https://github.com/agriyakhetarpal/PyBOP/actions/runs/7889580638.

I think it is coming from the fact that PyBOP tries to load a parameter set as a JSON object from pybamm.ParameterValues, but PyBaMM v23.5 did not have the following check for a non-existent parameter set in the parameter set dictionary (it exists in v24.1) as follows:

# Check if values is a named parameter set
if isinstance(values, str) and values in pybamm.parameter_sets.keys():
    values = pybamm.parameter_sets[values]
    values.pop("chemistry", None)
    self.update(values, check_already_exists=False)
else:
    raise ValueError("Invalid Parameter Value")

and therefore PyBaMM 23.5 just ends up loading the following physical constants, without raising a ValueError:

{
    "Ideal gas constant [J.K-1.mol-1]": pybamm.constants.R.value,
    "Faraday constant [C.mol-1]": pybamm.constants.F.value,
    "Boltzmann constant [J.K-1]": pybamm.constants.k_b.value,
    "Electron charge [C]": pybamm.constants.q_e.value,
}

My recommendation to fix this failing test would be to implement this check in the PyBOP codebase itself too, since PyBaMM does not provide a public method for this for its dependents like PyBOP to use (edit: it sure does, we can just check for the strings in list(pybamm.parameter_sets), didn't realise!). This isn't the best approach, but it should be safe to add since all it does is load the parameter set entry points that setuptools provides. This check can be added in the pybamm classmethod in the ParameterSet class in order to raise the desired ValueError.

I can go ahead and make these changes as required in this PR and add another CHANGELOG entry for this. Please let me know what works!

[BradyPlanden]

Thanks for investigating this further @agriyakhetarpal. I believe we had this check implemented previously, but with a pybamm update it became redundant, happy to have it back it to support previous pybamm versions. Let's proceed with adding it to the ParameterSet class.

[agriyakhetarpal]

Addressed in 937aea4 by adding this check inside the function itself (coverage passes both on CodeCov and locally). Scheduled tests with PyBaMM v23.5 are passing here: https://github.com/agriyakhetarpal/PyBOP/actions/runs/7901745176 and this should be good to merge following @martinjrobins's review.

---

BTW, I noticed that PyBOP does not use pyproject.toml yet and still relies on setup.py, and the testing infrastructure does not use pytest-xdist, even though it came up in prior conversation in #61 (comment) – adding it should provide some nice speedups. I am happy to contribute further and do both of these things! Furthermore, I'm willing to help on #176.

[BradyPlanden]

Thanks for sorting this @agriyakhetarpal!

BTW, I noticed that PyBOP does not use pyproject.toml yet and still relies on setup.py, and the testing infrastructure does not use pytest-xdist, even though it came up in prior conversation in #61 (comment) – adding it should provide some nice speedups. I am happy to contribute further and do both of these things!

Yes, we did use pytest-xdist (locally) for a while, but the addition of our plotly manager caused problems as the tests became order dependent. I haven't seen an easy way to deal with order dependent tests with xdist, I think you can limit threads to single files, but I believe I still ran into issues. Please open an issue on this and we can proceed, it would be great to have it sorted on the github actions.

Furthermore, I'm willing to help on #176.

That would be great, it's fallen by the wayside, if you want to take a look at the current issue and try to sort it out that would be great.

I'm going to go ahead and merge this PR, thanks again.

[BradyPlanden]

@all-contributors add @agriyakhetarpal to code and infrastructure

[allcontributors]

@BradyPlanden

I've put up a pull request to add @agriyakhetarpal! 🎉