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

Add pybammsolvers #28748

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

Add pybammsolvers #28748

wants to merge 32 commits into from

Conversation

prady0t
Copy link

@prady0t prady0t commented Jan 3, 2025

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

Signed-off-by: Pradyot Ranjan <[email protected]>
Copy link
Contributor

github-actions bot commented Jan 3, 2025

Hi! This is the staged-recipes linter and your PR looks excellent! 🚀

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Jan 3, 2025

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/pybammsolvers/meta.yaml) and found some lint.

Here's what I've got...

For recipes/pybammsolvers/meta.yaml:

  • ❌ The home item is expected in the about section.
  • ❌ Non noarch packages should have python requirement without any version constraints.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13464369326. Examine the logs at this URL for more detail.

@prady0t
Copy link
Author

prady0t commented Jan 3, 2025

@agriyakhetarpal @Saransh-cpp @arjxn-py @kratman
Locally I have problems with gfortran

@prady0t prady0t marked this pull request as draft January 3, 2025 22:14
@kratman
Copy link

kratman commented Jan 3, 2025

Ok I will take a look on Sunday

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Thanks @prady0t, looks like a good start! #28748 (comment) would need to be addressed as far as the recipe's static analysis goes, and then I've left other comments below about how the packaging of the recipe should go:

Comment on lines 3 to 5
### 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.

Comment on lines 24 to 38
requirements:
build:
- {{ compiler('c') }}
host:
- python >=3.9,<3.13
- setuptools >=64
- wheel
- casadi >=3.6.7 # [not win]
- cmake # [not win]
- 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.


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.

Comment on lines 31 to 32
- 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...

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.

prady0t and others added 7 commits January 6, 2025 03:05
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.

@prady0t
Copy link
Author

prady0t commented Jan 5, 2025

We have some progress, I can now compile both Suitesparse and Sundials. The failure now is conda-build cannot import pybammsolvers which suggests the build was not completed.

Copy link

@kratman kratman left a comment

Choose a reason for hiding this comment

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

Just starting to take a look at this repo. I will share anything I find


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).

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

It proceeds to compile now but fails to find a BLAS distribution:

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Jan 8, 2025

How about we don't attempt to compile SuiteSparse and directly add SUNDIALS as a host dependency – it's linked with KLU like we need it to be, and version 6.5.0 is available as well. That would be much easier :) Chaste and Octave use SUNDIALS in this manner. It would also circumvent our BLAS requirement that we're not able to fix here.

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Jan 16, 2025

Yes, we'll need the libs and the CMake directory for CasADi. We rely on the CasADi Python wheels we get from PyPI, but we don't have that option in this case. So, as I mentioned somewhere earlier/before, we can't use this command

import os; import sysconfig; print(os.path.join(sysconfig.get_path('purelib'), 'casadi', 'cmake'))

anymore, and need to find CasADi's CMake file in other ways. We can do this with two options:

A) Use casadi as a host dependency and see where it stores these files. Then, we can point CMake, the compiler, the linker, etc., to them.
B) Compile CasADi (without the Python interface) ourselves at the cost of ~4 minutes of extra CI time

I would recommend going with A) first, as it is the less intrusive and better approach. You'll need to patch that line for it to happen (in this case, we didn't have CMakeLists.txt in the sdist, so we can just replace those lines for the patch)

I do remember that these .cmake files are stored in the archive indeed; you would most likely find them in casadi_3.6.7_py3X_<...>/lib/cmake/casadi, but if you could confirm that with https://github.com/conda/conda-package-handling or similar, it would be great.

Signed-off-by: Pradyot Ranjan <[email protected]>
@prady0t
Copy link
Author

prady0t commented Jan 18, 2025

I was able to find casadi but some packages nvector/nvector_openmp.h cannot be found.

  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/Solution.cpp:1:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/Solution.hpp:4:
  /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/common.hpp:10:10: fatal error: 'nvector/nvector_openmp.h' file not found
     10 | #include <nvector/nvector_openmp.h>  /* access to openmp N_Vector            */
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolverOpenMP_solvers.cpp:1:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolverOpenMP_solvers.hpp:4:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolverOpenMP.hpp:4:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolver.hpp:4:
  /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/common.hpp:10:10: fatal error: 'nvector/nvector_openmp.h' file not found
     10 | #include <nvector/nvector_openmp.h>  /* access to openmp N_Vector            */
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolverGroup.cpp:1:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolverGroup.hpp:4:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolver.hpp:4:
  /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/common.hpp:10:10: fatal error: 'nvector/nvector_openmp.h' file not found
     10 | #include <nvector/nvector_openmp.h>  /* access to openmp N_Vector            */
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolver.cpp:1:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IDAKLUSolver.hpp:4:
  /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/common.hpp:10:10: fatal error: 'nvector/nvector_openmp.h' file not found
     10 | #include <nvector/nvector_openmp.h>  /* access to openmp N_Vector            */
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/SolutionData.cpp:1:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/SolutionData.hpp:5:
  /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/common.hpp:10:10: fatal error: 'nvector/nvector_openmp.h' file not found
     10 | #include <nvector/nvector_openmp.h>  /* access to openmp N_Vector            */
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IdakluJax.cpp:1:
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/IdakluJax.hpp:4:
  /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/common.hpp:10:10: fatal error: 'nvector/nvector_openmp.h' file not found
     10 | #include <nvector/nvector_openmp.h>  /* access to openmp N_Vector            */
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
  In file included from /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/common.cpp:1:
  /opt/miniconda3/envs/pybammsolvers-build/conda-bld/pybammsolvers_1737224830610/work/src/pybammsolvers/idaklu_source/common.hpp:10:10: fatal error: 'nvector/nvector_openmp.h' file not found
     10 | #include <nvector/nvector_openmp.h>  /* access to openmp N_Vector  

@agriyakhetarpal
Copy link
Member

Thanks! It might be good to see what the CI jobs need in comparison to your local build, as it is difficult for us to look at your build the full logs.

As for finding nvector_openmp.h file, that should be present in the solvers source code. As the logs are truncated, I'm not sure why it fails – in the full logs, is there a longer stack trace? If it fails to find OpenMP, I'm not sure why that would be the case. This is because SUNDIALS does build with it, at least on Linux: https://github.com/conda-forge/sundials-feedstock/blob/bd8b70c3d07d845eae73222010e3cef7a4120d80/recipe/build-sundials.sh#L7-L11

@prady0t
Copy link
Author

prady0t commented Jan 22, 2025

Mostly, the CI logs represent my local build failures. For osx, I think OpenMP was not found. Even inside $PREFIX/include/nvector, I cannot find OpenMP related files and these are its only contents:

nvector_manyvector.h
nvector_serial.h

Same for Windows builds. For linux now we cannot find klu. Might it be possible KLU linked Sundials is not available across all platforms?

@agriyakhetarpal
Copy link
Member

From https://github.com/conda-forge/sundials-feedstock/blob/bd8b70c3d07d845eae73222010e3cef7a4120d80/recipe/build-sundials.sh#L7-L11, SUNDIALS builds without OpenMP for Darwin, but builds with it for Linux. And, checking build.bat, it uses OpenMP for Windows. So we would indeed need to switch to a custom build of SUNDIALS for all platforms, where we properly link against both KLU and OpenMP (llvm-openmp on macOS and libgomp on Linux).

Signed-off-by: Pradyot Ranjan <[email protected]>
@prady0t
Copy link
Author

prady0t commented Feb 6, 2025

Sundials is build successfully. I've used pre-build Suitesparse from conda-forge and linked KLU against it. I don't know about the error in CI

    raise OverLinkingError(overlinking_errors)
conda_build.exceptions.OverLinkingError: <exception str() failed>

Locally it fails as it's not able to run tests, even though all steps pass without failure.

@agriyakhetarpal
Copy link
Member

The overlinking means that some dependencies are included as a part of the runtime, which aren't needed – for example, SUNDIALS and SuiteSparse are required only at build time, and not at runtime.

Also, is the IDAKLU solver being compiled? I ask because the CMakeLists.txt file was removed, so I'm not sure if it is. Could you add a test for the IDAKLU solver import?

@prady0t
Copy link
Author

prady0t commented Feb 6, 2025

I missed that. Thanks! But tests for importing pybamsolvers are still failing?

@agriyakhetarpal
Copy link
Member

I'm not sure if I see an import failure in the logs, though. Could you copy that, if you have some logs from a local build?

Also, @Saransh-cpp, @kratman, and I discussed this PR briefly in-person today. Would it be fine with you if we were to take over this PR in case you feel it will take some time over the next few weeks or so? Eric is planning for a v25.2 release at the end of February, and we will be two releases behind by then. I think contributing to the same PR should be fine for preserving authorship. I also have access to your fork already, so I can help push a few changes next week. If it's a yes from you, please grant Eric access in a few weeks as well.

I think the plan of action at the moment will be as follows:

  1. get the CMakeLists.txt file back
  2. patch CMakeLists.txt to look for the CasADi libs + headers under the prefix dirs, wherever they are (with CasADi set as a host dependency so that the libs and headers are available)
  3. remove whichever overlinked dependencies are left
  4. test that the import pybammsolvers statement works across all platforms.

@prady0t
Copy link
Author

prady0t commented Feb 8, 2025

Yes, it's totally fine! I've already sent the access to all of you.

@agriyakhetarpal
Copy link
Member

There are errors like:

  ERROR (pybammsolvers,lib/libsundials_sunlinsollapackband.so.4.5.0): Needed DSO lib/libopenblas.so.0 found in ['conda-forge/linux-64::libopenblas==0.3.28=pthreads_h94d23a6_1']
  ERROR (pybammsolvers,lib/libsundials_sunlinsollapackband.so.4.5.0): .. but ['conda-forge/linux-64::libopenblas==0.3.28=pthreads_h94d23a6_1'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)

in the logs, which we'll need to fix. I noticed SuiteSparse offers libklu as a separate package as its recipe contains multiple outputs. Please bear with me while I try to make more changes.

@agriyakhetarpal
Copy link
Member

Okay, I fixed the overlinking issue with SuiteSparse and OpenBLAS – we now need to compile pybammsolvers.

@agriyakhetarpal
Copy link
Member

I think this should be unblocked enough for you to proceed with the following steps, @prady0t. The next thing on the list would be to point to the KLU headers correctly – they will be in $PREFIX/include, so we can patch these lines: https://github.com/pybamm-team/pybammsolvers/blob/791677d64aff3b62025a62e85e97bb73736a0391/setup.py#L14-L16 so that we look for them in the right location.

@prady0t
Copy link
Author

prady0t commented Feb 21, 2025

Cannot find sundials_legacy_wrapper.hpp but it was removed. We might have to wait for a new release (the latest release still has it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants