Fix pipx + nox + pip dependency resolution bug in macOS workflows

[agriyakhetarpal]

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d88b7dd) 99.58% compared to head (0755c35) 99.58%.
Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3501   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        20116    20117    +1     
========================================
+ Hits         20033    20034    +1     
  Misses          83       83           

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

[agriyakhetarpal]

Looks like there is a different error now: OpenBLAS is not found. I am hoping that a re-installation will fix it

[agriyakhetarpal]

I did some more digging, and I realise that the error is most likely because the macOS runners are trying to install scipy off the source distribution rather than the wheels, but I don't get why this is the case. I don't think there are any version conflicts we need to take care of, because whenever the run passes a wheel is being downloaded, and the source distribution installation of scipy always causes a failure, as expected.

Edit: I am not sure if it is a pip problem, a GitHub Actions one, or a scipy one so as to file it in an appropriate issue tracker, but the act of simply re-triggering the failed runs has made them work in previous instances, so it should be fine to close this or add PIP_ONLY_BINARY="scipy" to the environment variables. Please let me know which of the two would be better

[brosaplanella]

Edit: I am not sure if it is a pip problem, a GitHub Actions one, or a scipy one so as to file it in an appropriate issue tracker, but the act of simply re-triggering the failed runs has made them work in previous instances, so it should be fine to close this or add PIP_ONLY_BINARY="scipy" to the environment variables. Please let me know which of the two would be better

@Saransh-cpp what do you think?

[brosaplanella]

Found this: DeepLabCut/DeepLabCut#2138 (comment)

Could the issue be related to the caching (i.e. we cached a version that fails so we run into the failing tests over and over)?

[agriyakhetarpal]

I think that is possible, which expands it to potentially be a setup-python cache issue too. It should have a configuration option to cache just wheels and not tarballs, but at least that is fixable by removing the cache entries in the Actions tab.

The issue has been irregular which is suggestive that it has to do with pip's dependency resolver as well (it might not be deterministic?). I would suggest bumping the minimum required versions to scipy>=1.10.1 and numpy>=1.19.5, i.e., the last versions that supported Python 3.8 so as to not put excess strain on the resolution algorithm. We could also look into setting up a constraints file to be tested like pyhf does in their scheduled workflows (https://github.com/scikit-hep/pyhf/actions/workflows/lower-bound-requirements.yml) as a continuation of #3304. I can look into opening a separate issue and PR about this after the release.

[Saransh-cpp]

I think this is a caching issue, definitely not a gfortran/SciPy issue. This could be a pip issue but I went through PyBaMM's dependencies and we don't have any conflicts. This is probably not a SciPy issue, but instead a SymPy issue. The CI is unable to find a SymPy version that satisfies our constraints, but that should not be a problem because there is only 1 SymPy constraint in our dependency tree -

pybamm                                Python Battery Mathematical Modelling.
├── anytree>=2.4.3                    Powerful and Lightweight Python Tree Data Structure with various plugins
│   └── six                           Python 2 and 3 compatibility utilities
├── autograd>=1.2                     Efficiently computes derivatives of numpy code.
│   ├── future>=0.15.2                Clean single-source support for Python 3 and 2
│   └── numpy>=1.12                   Fundamental package for array computing in Python
├── bpx                               BPX schema and parsers
│   ├── pydantic<2                    Data validation and settings management using python type hints
│   │   └── typing-extensions>=4.2.0  Backported and Experimental Type Hints for Python 3.8+
│   └── pyparsing                     pyparsing module - Classes and methods to define and execute parsing grammars
├── casadi>=3.6.0                     CasADi -- framework for algorithmic differentiation and numeric optimization
│   └── numpy                         Fundamental package for array computing in Python
├── imageio>=2.9.0                    Library for reading and writing a wide range of image, video, scientific, and volumetric data formats.
│   ├── numpy                         Fundamental package for array computing in Python
│   └── pillow<10.1.0,>=8.3.2         Python Imaging Library (Fork)
├── importlib-metadata                Read metadata from Python packages
│   └── zipp>=0.5                     Backport of pathlib-compatible object wrapper for zip files
├── matplotlib>=2.0                   Python plotting package
│   ├── contourpy>=1.0.1              Python library for calculating contours of 2D quadrilateral grids
│   │   └── numpy<2.0,>=1.20          Fundamental package for array computing in Python
│   ├── cycler>=0.10                  Composable style cycles
│   ├── fonttools>=4.22.0             Tools to manipulate font files
│   ├── kiwisolver>=1.3.1             A fast implementation of the Cassowary constraint solver
│   ├── numpy<2,>=1.21                Fundamental package for array computing in Python
│   ├── packaging>=20.0               Core utilities for Python packages
│   ├── pillow>=8                     Python Imaging Library (Fork)
│   ├── pyparsing>=2.3.1              pyparsing module - Classes and methods to define and execute parsing grammars
│   └── python-dateutil>=2.7          Extensions to the standard Python datetime module
│       └── six>=1.5                  Python 2 and 3 compatibility utilities
├── numpy>=1.16                       Fundamental package for array computing in Python
├── pandas>=0.24                      Powerful data structures for data analysis, time series, and statistics
│   ├── numpy<2,>=1.23.2              Fundamental package for array computing in Python
│   ├── python-dateutil>=2.8.2        Extensions to the standard Python datetime module
│   │   └── six>=1.5                  Python 2 and 3 compatibility utilities
│   ├── pytz>=2020.1                  World timezone definitions, modern and historical
│   └── tzdata>=2022.1                Provider of IANA time zone data
├── pybtex>=0.24.0                    A BibTeX-compatible bibliography processor in Python
│   ├── PyYAML>=3.01                  YAML parser and emitter for Python
│   ├── latexcodec>=1.0.4             A lexer and codec to work with LaTeX code in Python.
│   │   └── six>=1.4.1                Python 2 and 3 compatibility utilities
│   └── six                           Python 2 and 3 compatibility utilities
├── scikit-fem>=0.2.0                 Simple finite element assemblers
│   ├── numpy                         Fundamental package for array computing in Python
│   └── scipy                         Fundamental algorithms for scientific computing in Python
│       └── numpy<1.28.0,>=1.21.6     Fundamental package for array computing in Python
├── scipy>=1.3                        Fundamental algorithms for scientific computing in Python
│   └── numpy<1.28.0,>=1.21.6         Fundamental package for array computing in Python
├── sympy>=1.8                        Computer algebra system (CAS) in Python
│   └── mpmath>=0.19                  Python library for arbitrary-precision floating-point arithmetic
├── tqdm                              Fast, Extensible Progress Meter
└── xarray                            N-D labeled arrays and datasets in Python
    ├── numpy>=1.22                   Fundamental package for array computing in Python
    ├── packaging>=21.3               Core utilities for Python packages
    └── pandas>=1.4                   Powerful data structures for data analysis, time series, and statistics
        ├── numpy<2,>=1.23.2          Fundamental package for array computing in Python
        ├── python-dateutil>=2.8.2    Extensions to the standard Python datetime module
        │   └── six>=1.5              Python 2 and 3 compatibility utilities
        ├── pytz>=2020.1              World timezone definitions, modern and historical
        └── tzdata>=2022.1            Provider of IANA time zone data

I would also suggest installing PyBaMM without Nox in the CI once to check if this is a Nox issue.

[agriyakhetarpal]

Thanks for the reviews. I temporarily removed the version bounds and I am now trying to exercise some new version constraints to see if they work and fix the sympy issue (they worked locally on Python 3.8 when installing [all,dev,docs] together). I will clean up the changes after that.

[agriyakhetarpal]

Surprisingly (or rather not), the dependency resolution does pass sometimes: https://github.com/pybamm-team/PyBaMM/actions/runs/6812446181/job/18525103672?pr=3501 when the others fail. It is not related to the Python versions because we have seen it pass on a different workflow run or on a re-run while it has failed on runs where it previously passed.

[agriyakhetarpal]

So far, caching is not the problem here and can be ruled out. These logs have cached wheels for scipy and sympy, and therefore all integration tests proceed as normal. The issue comes only when pip chooses to install from a source distribution of scipy, which is always version 1.11.1 as far as I have noticed. It should also be safe to rule out nox, since it uses pip in a subprocess and installs in a virtual environment without any modifications to the resolver or the installation hooks.

[brosaplanella]

What's the status of this PR? Should we reviewed or does more stuff need to be done?

[agriyakhetarpal]

What's the status of this PR? Should we reviewed or does more stuff need to be done?

So far, I have not been able to reproduce the problem on macOS arm64 or Windows amd64 locally. I will try bumping up a few of the lower bounds for some more of our dependencies to the minimum version where they started supporting Python 3.8 to see if that solves the issue or try to single out the error in other ways

@Saransh-cpp reports that there are no dependency conflicts and I concur, so even if even if bumping the lower bounds doesn't help, it should be a good thing to do and we can merge those changes.

[agriyakhetarpal]

Possible areas where this issue is coming from

Ruled out

Likely cause	Reason it was ruled out
gfortran missing	Not required when all dependencies have wheels
Homebrew issue	Fixed, and Homebrew is not related to and does not manage Python dependencies
Caching issue with setup-python	Removed in f87b6bf, but subsequent workflow runs failed as well
nox	Runs pip in a subprocess with the current Python executable, unlikely to be the cause of the error
Version conflicts	Checked, none found. No problems when installing locally

Investigating

Potential reason	(Incomplete) explanation
❌ Dependency resolution bug in pip	Might not be deterministic; sometimes the tests proceed and pass
❌ Discrepancies in GitHub Actions runners' configurations	They keep getting updated all the time and not all of them are updated
❌ scipy	Version 1.11.1 was yanked on PyPI due to a licensing issue, maybe that is why pip keeps installing it with its tarball?
❌ sympy	Version constraints mismatch?
✅ Bug in pipx and/or its use with nox	The "Install PyBaMM dependencies" step that I had removed in dc1f6ed does not issue any errors and installs everything normally, so pipx could be the culprit

---

Note to self: try python -m nox instead of pipx run nox (worked!)

[agriyakhetarpal]

It turns out this is likely an issue when working with pipx and nox together. The pip dependency resolver has been deterministic ever since its origin and post its revamp in 2020 as far as I can tell; changing to python -m nox instead of pipx run nox helped fix it. The failures on the Linux tests (scripts, unit, integration, coverage) here are unrelated since I turned the pybamm-requires caches back on – they can be fixed by either deleting some of the recent cache entries in the Actions tab or safely left as-is because these changes will not affect other PRs. The previous run, where they had been removed, had passed: https://github.com/pybamm-team/PyBaMM/actions/runs/6889728515.

The notebook tests and pre-commit.ci tests will be fixed by #3519 and the failing doctests are unrelated coming from an issue with SciPy's intersphinx inventory. All macOS unit and integration tests proceeded to run here and passed after the installation of dependencies.

I have also bumped up the versions of some of the dependencies keeping compatibility to a minimum of Python 3.8 and introduced recursive optional dependencies in setup.py. This PR should now be ready to review and merge.

[Saransh-cpp]

I guess the CI is failing because of the CMake cache?

[agriyakhetarpal]

Yes, as mentioned in #3501 (comment)

[Saransh-cpp]

Thanks, @agriyakhetarpal! Finally some green checks haha

A couple of comments about the minimum supported version -

[agriyakhetarpal]

I have now bumped the lower bounds keeping to a minimum of those with more than six months left in the support cycle according to the SPEC and ignored those versions which have less than three months left

[Saransh-cpp]

Looks good now, thanks!

[brosaplanella]

Looks great, thanks!

[arjxn-py]

Thanks @agriyakhetarpal, looks nice.