-
-
Notifications
You must be signed in to change notification settings - Fork 5k
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
base: main
Are you sure you want to change the base?
Add pybammsolvers #28748
Conversation
Signed-off-by: Pradyot Ranjan <[email protected]>
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
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 ( Here's what I've got... For recipes/pybammsolvers/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12659991425. Examine the logs at this URL for more detail. |
@agriyakhetarpal @Saransh-cpp @arjxn-py @kratman |
Ok I will take a look on Sunday |
There was a problem hiding this 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:
### Compile and install SUITESPARSE ### | ||
# SuiteSparse is required to compile SUNDIALS's | ||
# KLU solver. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### 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?
There was a problem hiding this comment.
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.
recipes/pybammsolvers/meta.yaml
Outdated
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 | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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') }} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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") }}
There was a problem hiding this comment.
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.
recipes/pybammsolvers/meta.yaml
Outdated
- casadi >=3.6.7 # [not win] | ||
- cmake # [not win] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Agriya Khetarpal <[email protected]>
Co-authored-by: Agriya Khetarpal <[email protected]>
Co-authored-by: Agriya Khetarpal <[email protected]>
Co-authored-by: Agriya Khetarpal <[email protected]>
Co-authored-by: Agriya Khetarpal <[email protected]>
Co-authored-by: Agriya Khetarpal <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
for dir in SuiteSparse_config AMD COLAMD BTF KLU | ||
do | ||
make -C $SUITESPARSE_DIR/$dir library | ||
make -C $SUITESPARSE_DIR/$dir install INSTALL=$PREFIX |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
We have some progress, I can now compile both |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
Co-authored-by: Agriya Khetarpal <[email protected]>
Co-authored-by: Agriya Khetarpal <[email protected]>
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).