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.

Sign up for GitHub

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

Add pybammsolvers #28748

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Add pybammsolvers #28748

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8d54891
Add pybammsolvers
prady0t Jan 3, 2025
d9b57d3
Update recipes/pybammsolvers/build.sh
prady0t Jan 5, 2025
6f7e331
Update recipes/pybammsolvers/build.sh
prady0t Jan 5, 2025
06986fa
Update recipes/pybammsolvers/meta.yaml
prady0t Jan 5, 2025
e8d4def
Update recipes/pybammsolvers/build.sh
prady0t Jan 5, 2025
c3b58e5
Update recipes/pybammsolvers/meta.yaml
prady0t Jan 5, 2025
292631f
Update recipes/pybammsolvers/meta.yaml
prady0t Jan 5, 2025
4e208c1
Added suggestions
prady0t Jan 5, 2025
3e1c6f9
Update recipes/pybammsolvers/meta.yaml
prady0t Jan 7, 2025
d02a288
Update recipes/pybammsolvers/meta.yaml
prady0t Jan 7, 2025
9b00b47
Update meta.yaml
prady0t Jan 7, 2025
063dd55
Try to find BLAS
agriyakhetarpal Jan 7, 2025
b1843c6
Try to find BLAS again
agriyakhetarpal Jan 7, 2025
6ff1610
Using ${PREFIX} instead of /usr/local
prady0t Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions recipes/pybammsolvers/build.sh
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to be addressed right away, but we'll also need a build.bat equivalent of this script for Windows.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm aware. Once the sh script works, I'll try replication the steps for bat file.

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
set -euxo pipefail

### Compile and install SUITESPARSE ###
# SuiteSparse is required to compile SUNDIALS's
# KLU solver.
Comment on lines +3 to +5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Compile and install SUITESPARSE ###
# SuiteSparse is required to compile SUNDIALS's
# KLU solver.
### Compile and install SUITESPARSE ###
# SuiteSparse is required to compile SUNDIALS's
# KLU solver.

Nitpicking: if you do wish to keep comments for future reference in this recipe, maybe consolidating them in one place is one way to go. That said, I don't think the comments themselves will add much, could you add links to the docs here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely forgot about this. Let me resolve this in the next commits.


SUITESPARSE_DIR=suitesparse
for dir in SuiteSparse_config AMD COLAMD BTF KLU
do
make -C $SUITESPARSE_DIR/$dir library
make -C $SUITESPARSE_DIR/$dir install INSTALL=$PREFIX
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used $PREFIX here and for the Sundials installation as well because there were permission problems with usr. Let me know if this is not preferable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can still see the CI fails due to the same problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, $PREFIX is the ideal location to store them, usr/ is not user-writeable on most CI runners.

done

mkdir -p build_sundials
cd build_sundials
KLU_INCLUDE_DIR=/usr/local/include
KLU_LIBRARY_DIR=/usr/local/lib
SUNDIALS_DIR=sundials
cmake -DENABLE_LAPACK=ON\
-DSUNDIALS_INDEX_SIZE=32\
-DEXAMPLES_ENABLE:BOOL=OFF\
-DENABLE_KLU=ON\
-DENABLE_OPENMP=ON\
-DKLU_INCLUDE_DIR=$KLU_INCLUDE_DIR\
-DKLU_LIBRARY_DIR=$KLU_LIBRARY_DIR\
-DCMAKE_INSTALL_PREFIX=$PREFIX\
../$SUNDIALS_DIR
make install
56 changes: 56 additions & 0 deletions recipes/pybammsolvers/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
{% set name = "pybammsolvers" %}
{% set version = "0.0.4" %}
{% set sundials_version = "6.5.0" %}
{% set suitesparse_version = "6.0.3" %}

package:
name: {{ name|lower }}
version: {{ version }}

source:
- url: https://pypi.org/packages/source/{{ name[0] }}/{{ name }}/pybammsolvers-{{ version }}.tar.gz
agriyakhetarpal marked this conversation as resolved.
Show resolved Hide resolved
sha256: 6b76fb61e68af34b3a4803edc8020f508b10195f67857d8010e40f78cbbe2768
- url: https://github.com/LLNL/sundials/archive/v{{ sundials_version }}.tar.gz
sha256: 7557cdb8fe2a9b56fa0c259aa4e2c2dc488a1c79d482bb7943e27b7d76f6931b
folder: sundials
- url: https://github.com/DrTimothyAldenDavis/SuiteSparse/archive/refs/tags/v{{ suitesparse_version }}.tar.gz
sha256: 7111b505c1207f6f4bd0be9740d0b2897e1146b845d73787df07901b4f5c1fb7
folder: suitesparse


build:
number: 0

requirements:
build:
- {{ compiler('c') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll also need stdlib('c') (libc), compiler('c++'), and compiler('fortran') here – does that help with your issue? Or, are you currently testing your changes using build_locally.py and Docker?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can compile Sundials now. I don't think it's necessary to have stdlib('c') here, a c compiler should automatically get standard c libraries. Also {{ stdlib("c") }} causes :

Could not solve for environment specs
The following package could not be installed
└─ c_osx-arm64 =* * does not exist (perhaps a typo or a missing channel).

But I may be doing something wrong. I'm commenting out this step until further discussion, as I can still compile without it.

I'm currently only using conda-build. Should I choose docker instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry I didn't see the lint bot comments. We do need {{ stdlib("c") }}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I think it wouldn't hurt to try using Docker, since getting it to work on Linux before macOS would be nice, and it being isolated doesn't modify your local development environment.

- {{ compiler('fortran') }}
- {{ stdlib("c") }}
- {{ compiler('cxx') }}
host:
prady0t marked this conversation as resolved.
Show resolved Hide resolved
- python {{ python_min }}
- setuptools >=71
- casadi >=3.6.7
- cmake
prady0t marked this conversation as resolved.
Show resolved Hide resolved
- pip
run:
- python >={{ python_min }}
- casadi
- numpy <2.0

test:
imports:
- pybammsolvers
commands:
- pip check
requires:
- pip

about:
summary: Python interface for the IDAKLU solver
license: BSD-3-Clause
license_file: LICENSE

extra:
recipe-maintainers:
- prady0t
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the IDAKLU maintainers group from pybamm team

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's common for conda-forge feedstocks for packages to be maintained by folks other than the upstream package maintainers, so I will suggest keeping both – unless you did mean that, of course.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always find it easier to add maintainers once the feedstock is up, otherwise -

GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there

which is cumbersome (and will make the CI fail).

Loading