Make typing-extensions and sympy required dependencies to fix PyBaMM import

[arjxn-py]

Description

Fix broken import of PyBaMM raised by type hinting.

Fixes #3836

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

[kratman]

I would say that we need SymPy. It is a requirement for expression tree if it uses the types from the module. We should probably just make SymPy required instead.

If expression tree is not needed by everything else, then we could make expression tree optional, but I think SymPy is clearly required by expression tree

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (ed6df7e) to head (ad958a2).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3848      +/-   ##
===========================================
- Coverage    99.60%   99.60%   -0.01%     
===========================================
  Files          259      259              
  Lines        21284    21261      -23     
===========================================
- Hits         21200    21177      -23     
  Misses          84       84              

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

[agriyakhetarpal]

Thanks, @arjxn-py – this looks better now. To ensure that this doesn't happen again, could you please add some more tests for the have_optional_dependency method?

In particular, to do this we can either open and read the pyproject.toml file to extract the list of dependencies we have, or use importlib.metadata (the latter is better):

import importlib.metadata

importlib.metadata.metadata("pybamm").json['requires_dist']

returns this list of strings:

['numpy >=1.23.5',
 'scipy >=1.9.3',
 'casadi >=3.6.3',
 'xarray >=2022.6.0',
 'anytree >=2.8.0',
 "autograd >=1.6.2 ; extra == 'all'",
 "scikit-fem >=8.1.0 ; extra == 'all'",
 "pybamm[bpx,cite,examples,latexify,pandas,plot,tqdm] ; extra == 'all'",
 "bpx >=0.4.0 ; extra == 'bpx'",
 "bibtexparser >=2.0.0b5 ; extra == 'cite'",
 "pre-commit ; extra == 'dev'",
 "ruff ; extra == 'dev'",
 "nox ; extra == 'dev'",
 "coverage[toml] ; extra == 'dev'",
 "pytest >=6 ; extra == 'dev'",
 "pytest-xdist ; extra == 'dev'",
 "nbmake ; extra == 'dev'",
 "sphinx >=6 ; extra == 'docs'",
 "sphinx-rtd-theme >=0.5 ; extra == 'docs'",
 "pydata-sphinx-theme ; extra == 'docs'",
 "sphinx-design ; extra == 'docs'",
 "sphinx-copybutton ; extra == 'docs'",
 "myst-parser ; extra == 'docs'",
 "sphinx-inline-tabs ; extra == 'docs'",
 "sphinxcontrib-bibtex ; extra == 'docs'",
 "sphinx-autobuild ; extra == 'docs'",
 "sphinx-last-updated-by-git ; extra == 'docs'",
 "nbsphinx ; extra == 'docs'",
 "ipykernel ; extra == 'docs'",
 "ipywidgets ; extra == 'docs'",
 "sphinx-gallery ; extra == 'docs'",
 "sphinx-hoverxref ; extra == 'docs'",
 "sphinx-docsearch ; extra == 'docs'",
 "jupyter ; extra == 'examples'",
 'jax ==0.4.20 ; (python_version >= "3.9") and extra == \'jax\'',
 'jaxlib ==0.4.20 ; (python_version >= "3.9") and extra == \'jax\'',
 "sympy >=1.12 ; extra == 'latexify'",
 "scikits.odes ; extra == 'odes'",
 "pandas >=1.5.0 ; extra == 'pandas'",
 "imageio >=2.3.0 ; extra == 'plot'",
 "matplotlib >=3.6.0 ; extra == 'plot'",
 "tqdm ; extra == 'tqdm'"]

from which we should be able to clean and parse all of the optional dependencies. The json attribute was added in Python 3.10, so for versions prior to that we can perhaps read importlib.metadata.metadata("pybamm")["Provides-Extra"] (note that there are multiple keys of the same name "Provides-Extra"]).

P.S. The best resource for the job would be the PyPA packaging package (https://packaging.pypa.io/en/stable/metadata.html), but that is an external dependency and not part of the stdlib

[arjxn-py]

To ensure that this doesn't happen again, could you please add some more tests for the have_optional_dependency method?

Thanks for the review @agriyakhetarpal, but just mocking all optional dependencies to tests won't be enough as we currently we run tests on pybamm[all] variant. I should need to create a new job where i just install pybamm base variant and then run necessary tests. But considering the priority of this issue i feel it'd be right to open a separate PR for that.

[agriyakhetarpal]

Yes, a follow-up PR should be okay – we should target that soon after this PR so that we are properly testing all dependencies. It should be nothing more than another entry to the GitHub Actions matrix that can be accessed in the context of a step

[agriyakhetarpal]

Could you fix the example notebooks failure here? It does look like a small fix – not sure how it was passing earlier

[arjxn-py]

Could you fix the example notebooks failure here? It does look like a small fix – not sure how it was passing earlier

Sure

[arjxn-py]

Could you fix the example notebooks failure here? It does look like a small fix – not sure how it was passing earlier

The failure looks unrelated and inconsistent, please check the runs here. It is passing currently but if it remains an issue we shall look what causing it in the first place.

[kratman]

Just a question: Is have_optional_dependency() used anywhere else? It looks like it was used pretty heavily for sympy. If it is was not used anywhere else then can it be removed as well?

[agriyakhetarpal]

It is currently used for matplotlib and scikit-fem, which seem to be more optional in their use than sympy. matplotlib was originally the only dependency that was implemented to be optional before PyBaMM v23.5 (but it was not marked as such).

At some point we should consider renaming the function to import_optional_dependency to align it with the fact that it is just a wrapper over importlib.import_module() to raise a helpful error message

[kratman]

At some point we should consider renaming the function to import_optional_dependency to align it with the fact that it is just a wrapper over importlib.import_module() to raise a helpful error message

Yeah that would be a way better name. We should just start fixing that sort of thing on the fly as we see it

My complaints were addressed

[Saransh-cpp]

Thanks, @arjxn-py!