Fix #3418 Allow pybamm-requires to return early if an existing SUNDIALS installation is found #3455
Conversation
Saswatsusmoy
changed the title
| @@ -36,11 +37,19 @@ def set_environment_variables(env_dict, session): | |||
|
|
|||
| @nox.session(name="pybamm-requires") | |||
| def run_pybamm_requires(session): | |||
| """Download, compile, and install the build-time requirements for Linux and macOS: the SuiteSparse and SUNDIALS libraries.""" # noqa: E501 | |||
| set_environment_variables(PYBAMM_ENV, session=session) | |||
| """Download, compile, and install the build-time requirements for Linux and macOS""" | |||
There was a problem hiding this comment.
The previous docstring was okay
| @@ -36,11 +37,19 @@ def set_environment_variables(env_dict, session): | |||
|
|
|||
| @nox.session(name="pybamm-requires") | |||
| def run_pybamm_requires(session): | |||
| """Download, compile, and install the build-time requirements for Linux and macOS: the SuiteSparse and SUNDIALS libraries.""" # noqa: E501 | |||
| set_environment_variables(PYBAMM_ENV, session=session) | |||
There was a problem hiding this comment.
This line should not be deleted (this function ensures that the PYBAMM_ENV dictionary of environment variables is available to the session context throughout the duration of the session)
| sundials_path = Path("/path/to/sundials") | ||
| suitesparse_path = Path("/path/to/suitesparse") |
There was a problem hiding this comment.
The relevant files whose existence is to be checked for each of the build-time components are these shared objects, i.e., .so files (these will be, similarly, dynamic libraries in .dylib file format on macOS):
SUNDIALS
libsundials_idas.so
libsundials_sunlinsolklu.so
libsundials_sunlinsoldense.so
libsundials_sunlinsolspbcgs.so
libsundials_sunlinsollapackdense.so
libsundials_sunmatrixsparse.so
libsundials_nvecserial.so
libsundials_nvecopenmp.soSuiteSparse
libsuitesparseconfig.so
For the KLU component
libsuitesparseconfig.so
libklu.so
libamd.so
libcolamd.so
libbtf.so
If these files are not found, we should run the install_KLU_Sundials.py script
For more information: the SuiteSparse KLU module depends on these four components: suitesparseconfig, AMD, COLAMD, and BTF.
noxfile.py
Outdated
| sundials_path = Path("/path/to/sundials") | ||
| suitesparse_path = Path("/path/to/suitesparse") | ||
| if (sundials_path.exists() and suitesparse_path.exists()) and not force_rebuild: | ||
| session.log("Found existing build dependencies") |
There was a problem hiding this comment.
| session.log("Found existing build dependencies") | |
| session.warn("Found existing build-time requirements, skipping installation. Note: run with the --force flag (nox -s pybamm-requires -- --force) to invoke re-installation.") |
It is indeed better to run session.log here, but session.warn will display explicit warnings in yellow-coloured text which will be more visible to the user
There was a problem hiding this comment.
Also, could you similarly add a session.log that displays "Found pybind11" if the pybind11/ folder exists, below?
| if not os.path.exists("/path/to/klu"): | ||
| session.run("python", "scripts/install_klu.py") | ||
| if not os.path.exists(sundials_path): | ||
| session.run("python", "scripts/install_sundials.py") |
There was a problem hiding this comment.
We compile the build-time components in a single script, i.e., install_KLU_Sundials.py
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
|
Closing this because #3461 has been opened. I would recommend pushing commits to the same branch and therefore in a single PR going forward |
Description
Users can skip the re-installation of the build-time components if already present but still factor in the cases where it is necessary for them to reinstall for debugging purposes.
The key aspects are:
-Checking for existing Sundials and KLU installations before installing
-Breaking out the install scripts for each dependency
-Allowing a force rebuild with --force
-Keeping the pybind11 install logic the same
Fixes #3418
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.
Key checklist:
$ 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)$ python run-tests.py --all(or$ nox -s tests)$ 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: