From f3616d73dec003a10c3b2582d1d148b768680db7 Mon Sep 17 00:00:00 2001 From: Lorenzo <44714920+lorenzofavaro@users.noreply.github.com> Date: Mon, 18 Mar 2024 16:29:35 +0100 Subject: [PATCH 1/2] Test Optional Dependencies (#3892) * Remove anytree test * Make pandas required * Add test for optional dependencies * Add test pybamm import * Add test job * Remove comments * Rename job * Change exception type * Filter regex (dev&docs extra) * Clarify docstrings * Clarify error message * Remove test runner * Reformulate docstrings * Update changelog and documentation * Remove test module * Remove workflows * Refactor tests * Set package dynamically * Fix comment * Set conditional dependency --------- Co-authored-by: Eric G. Kratz Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com> --- CHANGELOG.md | 1 + docs/source/user_guide/installation/index.rst | 14 +-- pyproject.toml | 9 +- tests/__init__.py | 3 + tests/shared.py | 39 ++++++++ tests/unit/test_util.py | 94 ++++++++++++++----- 6 files changed, 118 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dec0c6741..fdaf360d6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ ## Breaking changes +- Integrated the `[pandas]` extra into the core PyBaMM package, deprecating the `pybamm[pandas]` optional dependency. Pandas is now a required dependency and will be installed upon installing PyBaMM ([#3892](https://github.com/pybamm-team/PyBaMM/pull/3892)) - Renamed "have_optional_dependency" to "import_optional_dependency" ([#3866](https://github.com/pybamm-team/PyBaMM/pull/3866)) - Integrated the `[latexify]` extra into the core PyBaMM package, deprecating the `pybamm[latexify]` set of optional dependencies. SymPy is now a required dependency and will be installed upon installing PyBaMM ([#3848](https://github.com/pybamm-team/PyBaMM/pull/3848)) - Renamed "testing" argument for plots to "show_plot" and flipped its meaning (show_plot=True is now the default and shows the plot) ([#3842](https://github.com/pybamm-team/PyBaMM/pull/3842)) diff --git a/docs/source/user_guide/installation/index.rst b/docs/source/user_guide/installation/index.rst index 1a773c65a5..00887bab6c 100644 --- a/docs/source/user_guide/installation/index.rst +++ b/docs/source/user_guide/installation/index.rst @@ -69,6 +69,7 @@ Package Minimum supp `Anytree `__ 2.8.0 `SymPy `__ 1.9.3 `typing-extensions `__ 4.10.0 +`pandas `__ 1.5.0 =================================================================== ========================== .. _install.optional_dependencies: @@ -97,19 +98,6 @@ Dependency Minimum Version p `matplotlib `__ 3.6.0 plot To plot various battery models, and analyzing battery performance. =========================================================== ================== ================== ================================================================== -.. _install.pandas_dependencies: - -Pandas dependencies -^^^^^^^^^^^^^^^^^^^ - -Installable with ``pip install "pybamm[pandas]"`` - -=========================================================== ================== ================== ================================================================== -Dependency Minimum Version pip extra Notes -=========================================================== ================== ================== ================================================================== -`pandas `__ 1.5.0 pandas For data manipulation and analysis. -=========================================================== ================== ================== ================================================================== - .. _install.docs_dependencies: Docs dependencies diff --git a/pyproject.toml b/pyproject.toml index 4e0b66bebf..d477419b91 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -43,6 +43,7 @@ dependencies = [ "anytree>=2.8.0", "sympy>=1.12", "typing-extensions>=4.10.0", + "pandas>=1.5.0" ] [project.urls] @@ -112,10 +113,8 @@ dev = [ "pytest>=6", "pytest-xdist", "nbmake", -] -# Reading CSV files -pandas = [ - "pandas>=1.5.0", + # To access the metadata for python packages + "importlib-metadata; python_version < '3.10'", ] # For the Jax solver. Note: these must be kept in sync with the versions defined in pybamm/util.py. jax = [ @@ -130,7 +129,7 @@ odes = [ all = [ "autograd>=1.6.2", "scikit-fem>=8.1.0", - "pybamm[examples,plot,cite,bpx,tqdm,pandas]", + "pybamm[examples,plot,cite,bpx,tqdm]", ] [project.scripts] diff --git a/tests/__init__.py b/tests/__init__.py index 919605998e..e057191c71 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -37,5 +37,8 @@ get_unit_2p1D_mesh_for_testing, get_cylindrical_discretisation_for_testing, get_base_model_with_battery_geometry, + get_required_distribution_deps, + get_optional_distribution_deps, + get_present_optional_import_deps, ) from .testcase import TestCase diff --git a/tests/shared.py b/tests/shared.py index c863cac175..fbcb18f292 100644 --- a/tests/shared.py +++ b/tests/shared.py @@ -3,6 +3,13 @@ # import pybamm from scipy.sparse import eye +import sys +import re + +if sys.version_info < (3, 10): + import importlib_metadata +else: + import importlib.metadata as importlib_metadata class SpatialMethodForTesting(pybamm.SpatialMethod): @@ -276,3 +283,35 @@ def get_base_model_with_battery_geometry(**kwargs): model = pybamm.BaseModel() model._geometry = pybamm.battery_geometry(**kwargs) return model + + +def get_required_distribution_deps(package_name): + pattern = re.compile(r"(?!.*extra\b)^([^<>=;\[]+)\b.*$") + if json_deps := importlib_metadata.metadata(package_name).json.get("requires_dist"): + return {m.group(1) for dep_name in json_deps if (m := pattern.match(dep_name))} + return set() + + +def get_optional_distribution_deps(package_name): + pattern = re.compile(rf"(?!.*{package_name}\b|.*docs\b|.*dev\b)^([^<>=;\[]+)\b.*$") + if json_deps := importlib_metadata.metadata(package_name).json.get("requires_dist"): + return { + m.group(1) + for dep_name in json_deps + if (m := pattern.match(dep_name)) and "extra" in m.group(0) + } + return set() + + +def get_present_optional_import_deps(package_name, optional_distribution_deps=None): + if optional_distribution_deps is None: + optional_distribution_deps = get_optional_distribution_deps(package_name) + + present_optional_import_deps = set() + for ( + import_pkg, + distribution_pkgs, + ) in importlib_metadata.packages_distributions().items(): + if any(dep in optional_distribution_deps for dep in distribution_pkgs): + present_optional_import_deps.add(import_pkg) + return present_optional_import_deps diff --git a/tests/unit/test_util.py b/tests/unit/test_util.py index 9be5f6a6e2..c880fd5604 100644 --- a/tests/unit/test_util.py +++ b/tests/unit/test_util.py @@ -1,6 +1,7 @@ # # Tests the utility functions. # +import importlib from tests import TestCase import numpy as np import os @@ -10,9 +11,12 @@ import unittest from unittest.mock import patch from io import StringIO -from tempfile import TemporaryDirectory -anytree = sys.modules["anytree"] +from tests import ( + get_optional_distribution_deps, + get_required_distribution_deps, + get_present_optional_import_deps, +) class TestUtil(TestCase): @@ -32,7 +36,6 @@ def test_rmse(self): pybamm.rmse(np.ones(5), np.zeros(3)) def test_is_constant_and_can_evaluate(self): - sys.modules["anytree"] = anytree symbol = pybamm.PrimaryBroadcast(0, "negative electrode") self.assertEqual(False, pybamm.is_constant_and_can_evaluate(symbol)) symbol = pybamm.StateVector(slice(0, 1)) @@ -100,27 +103,70 @@ def test_git_commit_info(self): self.assertEqual(git_commit_info[:2], "v2") def test_import_optional_dependency(self): - with self.assertRaisesRegex( - ModuleNotFoundError, "Optional dependency pybtex is not available." - ): - pybtex = sys.modules["pybtex"] - sys.modules["pybtex"] = None - pybamm.print_citations() - with self.assertRaisesRegex( - ModuleNotFoundError, "Optional dependency anytree is not available." - ): - with TemporaryDirectory() as dir_name: - sys.modules["anytree"] = None - test_stub = os.path.join(dir_name, "test_visualize") - test_name = f"{test_stub}.png" - c = pybamm.Variable("c", "negative electrode") - d = pybamm.Variable("d", "negative electrode") - sym = pybamm.div(c * pybamm.grad(c)) + (c / d + c - d) ** 5 - sym.visualise(test_name) - - sys.modules["pybtex"] = pybtex - pybamm.util.import_optional_dependency("pybtex") - pybamm.print_citations() + optional_distribution_deps = get_optional_distribution_deps("pybamm") + present_optional_import_deps = get_present_optional_import_deps( + "pybamm", optional_distribution_deps=optional_distribution_deps + ) + + # Save optional dependencies, then set to None + modules = {} + for import_pkg in present_optional_import_deps: + modules[import_pkg] = sys.modules.get(import_pkg) + sys.modules[import_pkg] = None + + # Test import optional dependency + for import_pkg in present_optional_import_deps: + with self.assertRaisesRegex( + ModuleNotFoundError, + f"Optional dependency {import_pkg} is not available.", + ): + pybamm.util.import_optional_dependency(import_pkg) + + # Restore optional dependencies + for import_pkg in present_optional_import_deps: + sys.modules[import_pkg] = modules[import_pkg] + + def test_pybamm_import(self): + optional_distribution_deps = get_optional_distribution_deps("pybamm") + present_optional_import_deps = get_present_optional_import_deps( + "pybamm", optional_distribution_deps=optional_distribution_deps + ) + + # Save optional dependencies, then set to None + modules = {} + for import_pkg in present_optional_import_deps: + modules[import_pkg] = sys.modules.get(import_pkg) + sys.modules[import_pkg] = None + + # Test pybamm is still importable + try: + importlib.reload(importlib.import_module("pybamm")) + except ModuleNotFoundError as error: + self.fail( + f"Import of 'pybamm' shouldn't require optional dependencies. Error: {error}" + ) + + # Restore optional dependencies + for import_pkg in present_optional_import_deps: + sys.modules[import_pkg] = modules[import_pkg] + + def test_optional_dependencies(self): + optional_distribution_deps = get_optional_distribution_deps("pybamm") + required_distribution_deps = get_required_distribution_deps("pybamm") + + # Get nested required dependencies + for distribution_dep in list(required_distribution_deps): + required_distribution_deps.update( + get_required_distribution_deps(distribution_dep) + ) + + # Check that optional dependencies are not present in the core PyBaMM installation + optional_present_deps = optional_distribution_deps & required_distribution_deps + self.assertFalse( + bool(optional_present_deps), + f"Optional dependencies installed: {optional_present_deps}.\n" + "Please ensure that optional dependencies are not present in the core PyBaMM installation, or list them as required.", + ) class TestSearch(TestCase): From 2e354155e447fc3b3947c75375012942b271dd1d Mon Sep 17 00:00:00 2001 From: "allcontributors[bot]" <46447321+allcontributors[bot]@users.noreply.github.com> Date: Mon, 18 Mar 2024 11:38:14 -0400 Subject: [PATCH 2/2] docs: add lorenzofavaro as a contributor for code, and test (#3899) * docs: update all_contributors.md [skip ci] * docs: update .all-contributorsrc [skip ci] --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> Co-authored-by: Eric G. Kratz --- .all-contributorsrc | 10 ++++++++++ all_contributors.md | 3 +++ 2 files changed, 13 insertions(+) diff --git a/.all-contributorsrc b/.all-contributorsrc index c78f369325..bbc0147dec 100644 --- a/.all-contributorsrc +++ b/.all-contributorsrc @@ -836,6 +836,16 @@ "contributions": [ "code" ] + }, + { + "login": "lorenzofavaro", + "name": "Lorenzo", + "avatar_url": "https://avatars.githubusercontent.com/u/44714920?v=4", + "profile": "https://github.com/lorenzofavaro", + "contributions": [ + "code", + "test" + ] } ], "contributorsPerLine": 7, diff --git a/all_contributors.md b/all_contributors.md index 8cb18cdb35..efbbc254cf 100644 --- a/all_contributors.md +++ b/all_contributors.md @@ -104,6 +104,9 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d AKHIL SHARMA
AKHIL SHARMA
📖 Harshvir Sandhu
Harshvir Sandhu
💻 + + Lorenzo
Lorenzo
💻 ⚠️ +