Extend pybamm_install_odes to include support for macOS systems

[arjxn-py]

Description

Fixes #3404

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 (ee64eaf) 99.59% compared to head (738cd57) 99.59%.
Report is 26 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3417      +/-   ##
===========================================
- Coverage    99.59%   99.59%   -0.01%     
===========================================
  Files          258      258              
  Lines        20797    20796       -1     
===========================================
- Hits         20712    20711       -1     
  Misses          85       85              

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

[agriyakhetarpal]

Seems like we were testing pybamm_install_odes earlier (#2973) but that was back when we were using tox, do you remember why we removed it? I can't seem to find it anywhere. Maybe it was because we decided not to have a nox equivalent of tox -e odes or we just forgot to add one

[arjxn-py]

Seems like we were testing pybamm_install_odes earlier (#2973) but that was back when we were using tox, do you remember why we removed it? I can't seem to find it anywhere. Maybe it was because we decided not to have a nox equivalent of tox -e odes or we just forgot to add one

I don't remember exactly but it might be because of we wanted to cut time in CI or just might have forgotten to have a session for odes.

Edit: I can make the same nox session again to test them.

[arjxn-py]

Thanks @agriyakhetarpal, waiting on tests to pass the initial steps and then fail while installing odes to observe logs.

[agriyakhetarpal]

It looks like it worked without any additional configuration (just like it did for me locally). Is there an error in specific that we should reproduce to see if it is robust enough? Edit: similar to #2973, we can try installing an older version of SUNDIALS and check if it fails on macOS and GNU/Linux

We could refactor the script a bit and add some additional configuration (since this entry point is for user installations, it would be wise to add .zshrc alongside .bashrc in the update_LD_LIBRARY_PATH function because it is enabled by default on newer macOS systems). Some other stuff I identified that we can do:

1. We ask users to run the script and that it works out of the box, but it would fail on not finding wget and cmake. We could install both of them with pip inside a subprocess or instead throw more helpful errors
2. Use f-strings in the script since they are more efficient
3. scikits_odes_setup.log is not git-ignored currently
4. add a nox session that installs cmake and wget and wraps pybamm_install_odes (actually, this might not be needed because users don't get or need nox installed as a dependency unless they install the dev dependencies). Developers can already install SUNDIALS but with KLU enabled using pybamm-requires.
5. Update the documentation about macOS support for this entry point in the optional solvers section and other areas (and maybe make the script exit early with a helpful error if on a Windows system because we haven't done so yet). We should update the CHANGELOG about this too since this is rather a user-facing change.

[agriyakhetarpal]

We should also run pip cache purge in the installation step so that the runners don't end up using the cached wheels between runs and/or remove the cache configuration from the setup-python step

[agriyakhetarpal]

I will test the changes locally in a moment after removing my local installation of SUNDIALS

[agriyakhetarpal]

Tested locally without a pip-cached scikits.odes wheel and without SUNDIALS installed via Homebrew nor present in ~/.local/ and it works, thanks, @arjxn-py! I left a few comments, feel free to address.

@Saransh-cpp, I remember you were having problems with getting scikits.odes to work on your machine before, could you test this and see if it works?

[agriyakhetarpal]

Feel free to add some of the un-fixable broken links to the .lycheeignore file

[arjxn-py]

Feel free to add some of the un-fixable broken links to the .lycheeignore file

Most of them are due to file rename which I hope should be fixed except this one, let me know how should i go with this.

[agriyakhetarpal]

Most of them are due to file rename which I hope should be fixed except this one, let me know how should i go with this.

It can be added to .lycheeignore as well, or we can ignore files or entire folders using glob patterns in the workflow file, let me know if neither of these options work. Some of the links such as those coming from the rename to gnu-linux-mac will get fixed upon merging.

[arjxn-py]

Added the non-fixable one in .lycheeignore, hopefully others would be fixed upon merging.

[agriyakhetarpal]

I realised I missed reading through the workflow so I have a few comments. Could you please run it on your fork once and add a link to the logs here?

I think we can leave wget as-is for this PR. We can look into removing it in #3651 together after this gets merged. Could you resolve the conflicts here?

[arjxn-py]

Could you please run it on your fork once and add a link to the logs here?

Here's the link to the workflow run on the respective branch, waiting for tests to pass.

[arjxn-py]

Those failing example tests on linux are unrelated, right?

[agriyakhetarpal]

Yes, opening a new issue about them

[Saransh-cpp]

Looks good, thanks for picking this up, @arjxn-py!