Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEP 723 support for SuiteSparse and SUNDIALS installation script #3647

Closed
agriyakhetarpal opened this issue Dec 20, 2023 · 4 comments · Fixed by #3716
Closed

PEP 723 support for SuiteSparse and SUNDIALS installation script #3647

agriyakhetarpal opened this issue Dec 20, 2023 · 4 comments · Fixed by #3716
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours feature priority: medium To be resolved if time allows

Comments

@agriyakhetarpal
Copy link
Member

Description

PEP 723 is one of the latest accepted Python PEPs which brings a pyproject.toml-like format for Python scripts for easy sharing and collaboration, where dependencies, supported Python versions, and other metadata can be added in TOML-style comments at the top of the script.

I have been advised by the developers to wait until support for tooling around this PEP gets mature in the coming year by packages like pip-tools, pipx, virtualenv, and others; so this would be a low-priority change for now and is being opened for tracking purposes. In conversation with Martin, we agreed that a providing a URL-based short link from the pybamm.org website pointing to the raw content link of the script can be used by beginner contributors and new developers to set up the build-time dependencies for the IDAKLU solver, and the next step would be to clone the repository and simply type pip install -e ."[all]" for a successful PyBaMM installation.

Motivation

Easier installation of SUNDIALS and SuiteSparse when building PyBaMM from source for both Linux and macOS users from pipx, which can run remote URLs – say, something like pipx run https://pybamm.org/install-builddeps.py.

Possible Implementation

A comment block at the top of the install_KLU_Sundials.py file

# /// pyproject
# [run]
# requires-python = "<=3.9"
# dependencies = [
#   "wget",
#   "cmake",
# ]
# ///

which can be used by script runners to detect script metadata (and possibly install the dependencies too someday) along with other associated changes to the script to make it work as expected

Additional context

https://discuss.python.org/t/pep-722-723-decision/36763

@agriyakhetarpal agriyakhetarpal added feature difficulty: easy A good issue for someone new. Can be done in a few hours priority: low No existing plans to resolve labels Dec 20, 2023
@arjxn-py
Copy link
Member

@agriyakhetarpal We've to keep this issue open, right?

@agriyakhetarpal
Copy link
Member Author

Yes, reopening as discussed in #3716. We have the table that specifies metadata for this script through the changes in that PR, the next step to be looked at at a later time would be how well Python tools like pipx are customising behaviour for this PEP.

@agriyakhetarpal
Copy link
Member Author

PEP 723 has been revised and the metadata at the top of the script is now formatted differently (please refer to the linked document above for more information).

Script runners like pipx have adapted to this change – pipx automatically installs the dependencies listed in the metadata prior to running the script, which was the piece of functionality we intended (install CMake when running the script).

This issue can be bumped to a higher level of priority and should be an up-for-grabs one – after that, one should be able to run the script without even cloning the PyBaMM repository (one should just have pipx installed and present on their PATH, i.e., through pip install pipx && pipx ensurepath).

Some examples for this functionality are available in the pipx documentation, we can use these as references in our own documentation for the source installation instructions, and furthermore implement pipx run https://pybamm.org/install-builddeps.py, which will just point to the raw.githubusercontent.com/blob/develop/.../<path/to/file> with a 302 redirect (note: pipx will only execute apps from the internet directly if they end with '.py', and so should the URL).

I ran some tests from the current URL and it looks like we will have to remove the use of __file__ and fix a few subprocess calls, doing which should be trivial.

@agriyakhetarpal agriyakhetarpal added priority: medium To be resolved if time allows and removed priority: low No existing plans to resolve labels Feb 19, 2024
@agriyakhetarpal
Copy link
Member Author

This can be closed because we might not have a pybamm-requires session anymore after the migration to scikit-build-core (#3564). If that doesn't work out, we can always reopen it.

@agriyakhetarpal agriyakhetarpal closed this as not planned Won't fix, can't repro, duplicate, stale Mar 30, 2024
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this issue Aug 12, 2024
Introduce a TOML-style comment block at the top of the `install_KLU_Sundials.py` script. This block contains essential metadata, including the required Python version and a list of dependencies. This addition aligns the script with the PEP 723 guidelines, enhancing readability and portability for script runners and developers.

The metadata includes:
- The Python version requirement (<=3.9)
- Dependencies required for the script (wget, cmake)
- Additional information like the repository and documentation links

This enhancement facilitates easier script sharing and collaboration, providing a standardized way to specify and access script dependencies and supported Python versions. It also lays the groundwork for potential future tooling that could automate environment setup and dependency installation.

Refer to PEP 723 for more details on this format.

Resolves: pybamm-team#3647
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours feature priority: medium To be resolved if time allows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants