Specify lowest requirements #867
Conversation
|
See the tests passing here: https://github.com/pybop-team/PyBOP/actions/runs/21141815763 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #867 +/- ##
===========================================
+ Coverage 89.41% 90.79% +1.37%
===========================================
Files 63 66 +3
Lines 4857 5160 +303
===========================================
+ Hits 4343 4685 +342
+ Misses 514 475 -39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "nox[uv]>=2025", | ||
| "nbmake>=1.5.5", | ||
| "ipython>=8.38", # for nbmake | ||
| "pyzmq>=27", # for nbmake | ||
| "pre-commit>=4", | ||
| "pytest>=9", | ||
| "pytest-cov>=7", | ||
| "pytest-mock>=3.14", | ||
| "pytest-xdist>=3.7", | ||
| "ruff>=0.14", |
There was a problem hiding this comment.
This is generally a good idea, but I only realised now that we are checking for development dependencies in the test as well. I didn't realise it at the time in our previous discussion. We should drop that and only run tests with minimum dependency versions for all, and not all,dev.
| ] | ||
| scifem = [ | ||
| "scikit-fem>=8.1.0" # scikit-fem is a dependency for the multi-dimensional pybamm models | ||
| "scikit-fem>=8.1.0" # scikit-fem is a dependency for the multi-dimensional pybamm models |
There was a problem hiding this comment.
(Unrelated whitespace change)
| "scikit-fem>=8.1.0" # scikit-fem is a dependency for the multi-dimensional pybamm models | |
| "scikit-fem>=8.1.0" # scikit-fem is a dependency for the multi-dimensional pybamm models |
|
|
||
| pyprobe = [ | ||
| "PyProBE-Data>=2.5.0;python_version>='3.11'" | ||
| "PyProBE-Data>=2.5.0;python_version>='3.11'" |
There was a problem hiding this comment.
(Unrelated whitespace change)
| "PyProBE-Data>=2.5.0;python_version>='3.11'" | |
| "PyProBE-Data>=2.5.0;python_version>='3.11'" |
| "ep-bolfi>=3.0.2", | ||
| "GPy>=1.13.2", # for ep-bolfi | ||
| "cython>=3", # for GPy | ||
| "wheel>=0.20", # for ep-bolfi | ||
| "h5py>=3.10", # for ep-bolfi | ||
| "elfi>=0.8.6", # for ep-bolfi | ||
| "numdifftools>=0.9.41", # for elfi | ||
| "dask[distributed]>=2025.12", # for elfi | ||
| "networkx>=3", # for elfi | ||
| "pyimpspec>=4", # for ep-bolfi | ||
| "defusedxml>=0.6", # for pyimpspec | ||
| "pillow>11", # for pyimpspec | ||
| "xmlhelpy>=0.14", # for ep-bolfi | ||
| "pyarrow>=14", # for ep-bolfi |
There was a problem hiding this comment.
| "ep-bolfi>=3.0.2", | |
| "GPy>=1.13.2", # for ep-bolfi | |
| "cython>=3", # for GPy | |
| "wheel>=0.20", # for ep-bolfi | |
| "h5py>=3.10", # for ep-bolfi | |
| "elfi>=0.8.6", # for ep-bolfi | |
| "numdifftools>=0.9.41", # for elfi | |
| "dask[distributed]>=2025.12", # for elfi | |
| "networkx>=3", # for elfi | |
| "pyimpspec>=4", # for ep-bolfi | |
| "defusedxml>=0.6", # for pyimpspec | |
| "pillow>11", # for pyimpspec | |
| "xmlhelpy>=0.14", # for ep-bolfi | |
| "pyarrow>=14", # for ep-bolfi | |
| "ep-bolfi>=3.0.2", |
Cython is used only as a build-time dependency, so it shouldn't be specified as a runtime dependency. Similarly, wheel is only a CLI tool and we do not have much use for it in our runtime, so we should not list it. Is it a transitive dependency of us through a package? If yes, this is not good practice by them. We should drop both of them.
Similarly, I don't think it's helpful to specify the rest of the dependencies here, since we are a library rather than an application. In this case, doing this will increase the scope of our dependencies, making them non-transitive; i.e., if the package that depends on these projects stops doing so at some point, we may not get to know about it, and we will be specifying them for our users for no good reason.
| "ipython>=8.38", # for nbmake | ||
| "pyzmq>=27", # for nbmake |
There was a problem hiding this comment.
| "ipython>=8.38", # for nbmake | |
| "pyzmq>=27", # for nbmake |
Same as #867 (comment)
| "numpy>=1.26", | ||
| "scipy>=1.12", | ||
| "pints>=0.5.1", | ||
| "cma>=3", # for pints |
There was a problem hiding this comment.
| "cma>=3", # for pints |
Same as #867 (comment)
| "multiprocess>=0.70.13", # for SALib | ||
| "six>=1.16", # for python-dateutil for pandas | ||
| "posthog>=3", # for pybamm |
There was a problem hiding this comment.
| "multiprocess>=0.70.13", # for SALib | |
| "six>=1.16", # for python-dateutil for pandas | |
| "posthog>=3", # for pybamm |
Same as #867 (comment)
|
Hi @NicolaCourtier, thank you for trying this out! I think it is okay to drop the lockfile if you think so. However, I am not convinced that explicitly specifying our transitive dependencies is a good approach. Perhaps we should rethink our approach and instead use |
|
Thanks @agriyakhetarpal , these are good points! I agree there may be a better way. I didn't really understand what it would require when I suggested the test... still learning! |
Description
This may be overkill... but here is a list of minimum requirements that passes the new continuous integration test.
Fixes
check_lowest_dependencies.Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #).
Important checks:
Please confirm the following before marking the PR as ready for review:
$ pre-commit runor$ 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)nox -s testsnox -s doctest