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 22 commits into
base: main
Choose a base branch
from
Draft

Add pybammsolvers #28748

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 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
463ec23
Adding patches
prady0t Jan 12, 2025
eb13c68
Merge branch 'pybammsolvers' of https://github.com/prady0t/staged-rec…
prady0t Jan 12, 2025
22835b3
Merging
prady0t Jan 12, 2025
7d1e6af
Add cmake as host dependency
agriyakhetarpal Jan 12, 2025
1220d04
Adding pybind11 submodule
prady0t Jan 14, 2025
3cf1ed4
finding casadi
prady0t Jan 18, 2025
1fca94e
Adding make as host dependency
prady0t Jan 21, 2025
76317db
Removing libs and include location
prady0t Jan 22, 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.
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=suitsparse
prady0t marked this conversation as resolved.
Show resolved Hide resolved
for dir in SuiteSparse_config AMD COLAMD BTF KLU
do
make -C $SUITESPARSE_DIR/$dir library
make -C $SUITESPARSE_DIR/$dir install INSTALL=/usr
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\
prady0t marked this conversation as resolved.
Show resolved Hide resolved
-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=/usr\
../$SUNDIALS_DIR
make install
prady0t marked this conversation as resolved.
Show resolved Hide resolved
54 changes: 54 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,54 @@
{% set name = "pybammsolvers" %}
{% set version = "0.0.4" %}
{% set sundials_version = "6.5.0" %}
{% set suitsparse_version = "6.0.3" %}
prady0t marked this conversation as resolved.
Show resolved Hide resolved

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{{ suitsparse_version }}.tar.gz
sha256: 7111b505c1207f6f4bd0be9740d0b2897e1146b845d73787df07901b4f5c1fb7
folder: suitsparse
prady0t marked this conversation as resolved.
Show resolved Hide resolved


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.

host:
prady0t marked this conversation as resolved.
Show resolved Hide resolved
- python >=3.9,<3.13
- setuptools >=64
- wheel
prady0t marked this conversation as resolved.
Show resolved Hide resolved
- casadi >=3.6.7 # [not win]
- cmake # [not win]
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
- casadi >=3.6.7 # [not win]
- cmake # [not win]
- casadi >=3.6.7
- cmake

Our packaging is going to differ here because we won't use Vcpkg and MSVC – we'll use Clang that conda-forge provides, so we'll need CasADi and CMake too.

Copy link
Member

Choose a reason for hiding this comment

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

Though, it would be better to compile CasADi (just the C++ interface, that is – which means, with WITH_PYTHON3 set to OFF ) on our own as well so that we guarantee ABI compatibility with CasADi's conda-forge package.

We could then patch these lines: https://github.com/pybamm-team/PyBaMM/blob/a5616c960ac4769db7ff98a04628a04789e8bf7e/CMakeLists.txt#L119-L139 to turn off our lookup for the CasADi Python interface. The reason for this is that we wouldn't want to link against PyPI dependencies with conda-forge ones; both follow different methodologies for ABI compatibility. Let's revisit this once we have some progress and when we have SuiteSparse and SUNDIALS being compiled correctly...

- pip
run:
- python >=3.9,<3.13
- casadi
- numpy <2.0

Copy link
Member

Choose a reason for hiding this comment

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

We can use conda-forge's {{ python_min }} syntax here, at least for the minimum version of Python:

See https://github.com/conda-forge/cfep/blob/main/cfep-25.md

Copy link
Author

Choose a reason for hiding this comment

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

For some reason this fails for conda-build locally, so can't test this part locally.

Copy link
Member

Choose a reason for hiding this comment

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

That's strange. Let's keep this conversation unresolved for now, so that we can add it back in just before this is merged.

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