Fix #3418 Allow pybamm-requires to return early if an existing SUNDIAL… #3461

Saswatsusmoy · 2023-10-21T00:32:14Z

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.

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

…ing SUNDIALS installation is found

Saswatsusmoy · 2023-10-21T00:35:19Z

@agriyakhetarpal I have created a new PR with all the requested changes.

agriyakhetarpal · 2023-10-21T02:34:09Z

noxfile.py

+    sundials_so_path = [
+            Path("/path/to/sundials/libsundials_idas.so"),
+            Path("/path/to/sundials/libsundials_idas.dylib"),
+
+            Path("/path/to/sundials/libsundials_sunlinsolklu.so"),
+            Path("/path/to/sundials/libsundials_sunlinsolklu.dylib"),
+
+            Path("/path/to/sundials/libsundials_sunlinsoldense.so"),
+            Path("/path/to/sundials/libsundials_sunlinsoldense.dylib"),
+
+            Path("/path/to/sundials/libsundials_sunlinsolspbcgs.so"),
+            Path("/path/to/sundials/libsundials_sunlinsolspbcgs.dylib"),
+
+            Path("/path/to/sundials/libsundials_sunlinsollapackdense.so"),
+            Path("/path/to/sundials/libsundials_sunlinsollapackdense.dylib"),
+
+            Path("/path/to/sundials/libsundials_sunmatrixsparse.so"),
+            Path("/path/to/sundials/libsundials_sunmatrixsparse.dylib"),
+
+            Path("/path/to/sundials/libsundials_nvecserial.so"),
+            Path("/path/to/sundials/libsundials_nvecserial.dylib"),
+
+            Path("/path/to/sundials/libsundials_nvecopenmp.so"),
+            Path("/path/to/sundials/libsundials_nvecopenmp.dylib")
+        ]


The path to these .so and .dylib files is $HOME/local/lib, i.e., LD_LIBRARY_PATH (we already have these variables set above as constants in the file which you can reuse).

Here, .dylib files exist only on macOS and similarly .so files only on Linux, so you should use sys.platform like it has been used in other places in the file to check the target platform. Also, could you write the filenames in the list first and then append the .dylib and .so extensions and construct Path objects later? The code will be much cleaner that way. Something like this:

SUNDIALS_LIBS = [ ... "libsundials_idas", "libsundials_nvecopenmp", ... ] if sys.platform == "darwin" fileext = ".dylib" if sys.platform == "linux" fileext = ".so"

and join filenames with fileext, create Path objects by looping over them, check if they exist in LD_LIBRARY_PATH. Alternatively, the os module can be used to check if these files exist (os.path.isfile), but this is less preferable because we already have a pathlib import.

agriyakhetarpal · 2023-10-21T02:34:25Z

noxfile.py

+    klu_so_path = [
+            Path("/path/to/suitesparse/libsuitesparseconfig.so"),
+            Path("/path/to/suitesparse/libsuitesparseconfig.dylib"), # for MacOS
+
+            Path("/path/to/suitesparse/libklu.so"),
+            Path("/path/to/suitesparse/libklu.dylib"),
+
+            Path("/path/to/suitesparse/libamd.so"),
+            Path("/path/to/suitesparse/libamd.dylib"),
+
+            Path("/path/to/suitesparse/libcolamd.so"),
+            Path("/path/to/suitesparse/libcolamd.dylib"),
+
+            Path("/path/to/suitesparse/libbtf.so"),
+            Path("/path/to/suitesparse/libbtf.dylib"),
+        ]


agriyakhetarpal · 2023-10-21T02:36:50Z

noxfile.py

+        if (sundials_so_path.exists() and
+            klu_so_path.exists()) and not force_rebuild:
+            session.warn("Found existing build-time requirements, skipping installation. Note: run with the --force flag (nox -s pybamm-requires -- --force) to invoke re-installation.")  # noqa: E501
+            return


Suggested change

return

No need for return statements since nox runs as a CLI application only. You may use if-else clauses with the session.run commands to achieve the desired effect (if the path exists, don't run the script; else, run it)

agriyakhetarpal · 2023-10-21T02:39:47Z

noxfile.py

+            session.run("python", "scripts/install_Klu_Sundials.py")
+        elif os.path.exists("./pybind11"):
+            session.log("Found pybind11")
+            return


Suggested change

return

No need for the elif keyword either since this is another condition we need to check that does not rely on other conditions (pybind11 can be found even if the SUNDIALS libraries are not found)

agriyakhetarpal

This is starting to look a bit better. Are you testing these changes locally? I would recommend doing that since it will help debug how you should go forth with these changes. We can trigger a workflow run after that to test these changes in CI.

agriyakhetarpal · 2023-10-25T09:26:36Z

noxfile.py

+
+        if os.path.exists("./pybind11"):
+            session.log("Found pybind11")
+        elif not os.path.exists("./pybind11"):


There is no need for elif here, just check if the pybind11/ folder exists, else run the git clone command

else: session.error("nox -s pybamm-requires is only available on Linux & macOS.")

So should I omit this part where it throws an error from non-linux/mac users?

No, move this part to the top of the session, rather

agriyakhetarpal · 2023-10-25T22:16:42Z

noxfile.py

-    else:
-        session.error("nox -s pybamm-requires is only available on Linux & macOS.")
+        )
+        else:


There are a lot of if-else clauses here now. We should avoid nesting them. Could you check for the Windows platform and raise a session error earlier? This should avoid one level of nesting.

agriyakhetarpal · 2023-10-25T22:23:17Z

noxfile.py

+            for lib in KLU_LIBS
+        ]
+
+    elif sys.platform == "darwin":


Should be just if

agriyakhetarpal · 2023-10-25T22:44:12Z

noxfile.py

+    if sys.platform == "linux":
+        sundials_so_path = [
+            Path(PYBAMM_ENV["LD_LIBRARY_PATH"], lib)
+            for lib in SUNDIALS_LIBS
+        ]
+        klu_so_path = [
+            Path(PYBAMM_ENV["LD_LIBRARY_PATH"], lib)
+            for lib in KLU_LIBS
+        ]


On Linux, the file extension .so should be added to the paths. Could you rename sundials_so_path and klu_so_path to something like sundials_path and klu_path? Since these are being checked on macOS too, and we have .dylib files there.

agriyakhetarpal · 2023-10-25T22:55:54Z

noxfile.py

-        session.run("python", "scripts/install_KLU_Sundials.py")
-        if not os.path.exists("./pybind11"):
+
+        if sundials_so_path != [] and klu_so_path != [] and not force_rebuild:


It should be fine to just check if sundials_path and klu_path and not force_rebuild: (if a list is empty the conditional returns False).

agriyakhetarpal · 2023-10-25T23:02:54Z

noxfile.py

+        sundials_so_path = [
+            Path(PYBAMM_ENV["LD_LIBRARY_PATH"], lib + ".dylib")
+            for lib in SUNDIALS_LIBS
+    ]
+        klu_so_path = [
+            Path(PYBAMM_ENV["LD_LIBRARY_PATH"], lib + ".dylib")
+            for lib in KLU_LIBS
+    ]
+


I tested this locally: here, Path() is just constructing PathLike objects and then you're doing a list comprehension over them, this approach doesn't check if the paths to these files actually exist. The correct flow should be to create paths from SUNDIALS_LIBS and KLU_LIBS and have one single function return a boolean if all of them are found. This value can be checked upon calling the function (whether it's True or False) and the session.install and session.run statements can be executed or skipped accordingly.

SUNDIALS_LIBS = [ # libs ] KLU_LIBS = [ # libs ] def check_lib_paths(lib_names, lib_dir): if sys.platform == "darwin": ext = ".dylib" if sys.platform == "linux": ext = ".so" paths = [Path(lib_dir, lib + ext) for lib in lib_names] return all(os.path.exists(path) for path in paths) @nox.session(name="pybamm-requires") def run_pybamm_requires(session): # Set Variables sundials_path = check_lib_paths(SUNDIALS_LIBS, PYBAMM_ENV["LD_LIBRARY_PATH"]) klu_path = check_lib_paths(KLU_LIBS, PYBAMM_ENV["LD_LIBRARY_PATH"]) if sys.platform != "win32": session.install("wget", "cmake", silent=False) if sundials_path and klu_path and not force_rebuild: # Rest of the code

Something like this?

When I test this locally, it doesn't exits early even if the dependencies are still present but if I change

if sundials_path and klu_path and not force_rebuild: to if sundials_path !=[ ] and klu_path != [ ] and not force_rebuild:

it is exiting early

You can modify check_lib_paths to iterate over the list of file paths and return either True or False, and change the condition in that case, which can then look like

if check_lib_paths() and not force_rebuild: session.run("...")

i.e., the boolean and operator can then take care of the condition

Saransh-cpp · 2023-10-27T09:01:19Z

noxfile.py

+    SUNDIALS_LIBS = [
+        "libsundials_idas",
+        "libsundials_sunlinsolklu",
+        "libsundials_sunlinsoldense",
+        "libsundials_sunlinsolspbcgs",
+        "libsundials_sunlinsollapackdense",
+        "libsundials_sunmatrixsparse",
+        "libsundials_nvecserial",
+        "libsundials_nvecopenmp"
+    ]
+
+    KLU_LIBS = [
+        "libsuitesparseconfig",
+        "libklu",
+        "libamd",
+        "libcolamd",
+        "libbtf"
+    ]


@agriyakhetarpal, I think we should check for the existing installation in install_KLU_Sundials.py itself? We recommend developers to use nox but we also have documentation for doing stuff without nox, so it might be better to use nox just as a wrapper for these scripts.

IIRC the checks are also present in install_odes.py (and not the nox session of install_odes.py).

I would recommend doing these checks with nox rather than inside the script because nox is meant to ease the installation of the build-time requirements both for newer contributors and for experienced developers. The rationale behind my suggestion is that not only does nox provide a powerful command-line interface and can abstract away the extra code required to implement a command-line argument parser in a Python script (not that doing that would take time, though), it is also our current "recommended" method of installing from source in the installation instructions. Shall someone run install_KLU_Sundials.py and clone the pybind11 repository manually, it should be assumed that they know what they are doing and the script should recompile SuiteSparse and SUNDIALS as usual even if both components already exist on the system.

However, pybamm_install_odes is provided as an entry point for use in user installations. This is why we should skip adding a nox session for it – we don't bundle the noxfile in the source distribution or in the wheels anyway, plus for developers this is redundant (pip install pybamm[odes] now does the job once SUNDIALS_INST is correctly set). Therefore, install_odes.py should keep those checks inside itself since it is a standalone application that is shipped with PyBaMM.

Note: yes, the checks are inside install_odes.py but they are not complete; just one SUNDIALS-specific file is checked, i.e., libsundials_idas.so, compared to the list of files I have included here. We should update that soon (@arjxn-py, could you do that in #3417?)

P.S. going forward in the coming months in the aftermath of adding pyproject.toml, the changes here will not matter since steps can be taken to eliminate the dependence on pybamm-requires completely wherein a combination of a robust caching mechanism inside the script with hashes and the use of CCache can be devised to speed up and simplify the installation for both users and developers.

could you do that in #3417?

Yes sure, hope it's fine to do it after completing #3475

agriyakhetarpal · 2023-11-25T19:27:32Z

@Saswatsusmoy I would now suggest moving the checks for these dynamic libraries and shared object files to install_KLU_Sundials.py itself like @Saransh-cpp had suggested above – I did suggest keeping them in noxfile.py initially, but this recommendation comes after the fact that I am changing up some parts of the script myself since it was decided to establish a user-specific SUNDIALS and SuiteSparse installation directory. Doing that would not alter or add any files in .local/, and we can use the existing argument parser available in the script and add the "--force" argument to it. nox -s pybamm-requires can then use posargs to inherit that --force argument.

agriyakhetarpal · 2024-01-06T17:25:19Z

This PR has been inactive for two months now, closing. I can take up this task in conjunction with others listed in #3651.

Saswatsusmoy and others added 2 commits October 21, 2023 05:42

ix pybamm-team#3418 Allow pybamm-requires to return early if an exist…

f85bf67

…ing SUNDIALS installation is found

style: pre-commit fixes

99ea71a

agriyakhetarpal mentioned this pull request Oct 21, 2023

Fix #3418 Allow pybamm-requires to return early if an existing SUNDIALS installation is found #3455

Closed

8 tasks

agriyakhetarpal requested changes Oct 21, 2023

View reviewed changes

Saswatsusmoy and others added 8 commits October 24, 2023 17:57

Updated noxfile.py

303f86a

style: pre-commit fixes

3e5f3e0

Updated noxfile.py

6da3282

Update noxfile.py

f4d6a80

Update noxfile.py

921f805

style: pre-commit fixes

28f0cc7

Update noxfile.py

17bc401

style: pre-commit fixes

9a7d7d1

Saswatsusmoy changed the title ~~ix #3418 Allow pybamm-requires to return early if an existing SUNDIAL…~~ Fix #3418 Allow pybamm-requires to return early if an existing SUNDIAL… Oct 24, 2023

Saswatsusmoy requested a review from agriyakhetarpal October 24, 2023 18:28

agriyakhetarpal requested changes Oct 25, 2023

View reviewed changes

Saransh-cpp reviewed Oct 27, 2023

View reviewed changes

agriyakhetarpal closed this Jan 6, 2024

agriyakhetarpal mentioned this pull request Jan 13, 2024

Replace wget with urllib for downloading and parallelize with concurrent.futures for efficiency #3719

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3418 Allow pybamm-requires to return early if an existing SUNDIAL… #3461

Fix #3418 Allow pybamm-requires to return early if an existing SUNDIAL… #3461

Saswatsusmoy commented Oct 21, 2023

Saswatsusmoy commented Oct 21, 2023

agriyakhetarpal Oct 21, 2023

agriyakhetarpal Oct 21, 2023

agriyakhetarpal Oct 21, 2023 •

edited

Loading

agriyakhetarpal Oct 21, 2023

agriyakhetarpal left a comment •

edited

Loading

agriyakhetarpal Oct 25, 2023

Saswatsusmoy Oct 26, 2023

agriyakhetarpal Oct 26, 2023

agriyakhetarpal Oct 25, 2023

agriyakhetarpal Oct 25, 2023

agriyakhetarpal Oct 25, 2023

agriyakhetarpal Oct 25, 2023

agriyakhetarpal Oct 25, 2023

Saswatsusmoy Oct 26, 2023 •

edited

Loading

Saswatsusmoy Oct 26, 2023

agriyakhetarpal Oct 28, 2023

Saransh-cpp Oct 27, 2023

agriyakhetarpal Oct 27, 2023

arjxn-py Oct 28, 2023

agriyakhetarpal commented Nov 25, 2023 •

edited

Loading

agriyakhetarpal commented Jan 6, 2024

Fix #3418 Allow pybamm-requires to return early if an existing SUNDIAL… #3461

Fix #3418 Allow pybamm-requires to return early if an existing SUNDIAL… #3461

Conversation

Saswatsusmoy commented Oct 21, 2023

Type of change

Key checklist:

Further checks:

Saswatsusmoy commented Oct 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agriyakhetarpal Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agriyakhetarpal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Saswatsusmoy Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agriyakhetarpal commented Nov 25, 2023 • edited Loading

agriyakhetarpal commented Jan 6, 2024

agriyakhetarpal Oct 21, 2023 •

edited

Loading

agriyakhetarpal left a comment •

edited

Loading

Saswatsusmoy Oct 26, 2023 •

edited

Loading

agriyakhetarpal commented Nov 25, 2023 •

edited

Loading