Skip to content

Adds test suite and benchmarks #69

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

Jump to bottom
Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Adds test suite and benchmarks #69

wants to merge 18 commits into from

Conversation

BradyPlanden
Copy link
Member

This PR adds the following:

  • A test suite with local unit/integration tests
  • A nox session to run PyBaMM tests locally
  • A benchmarking suite for comparison between local changes and released PyBaMM
  • A fix for Casadi linking with isolated pip envs

@BradyPlanden
Fix: CasADi linking to work with pip isolated build environments
23741ef 
This commit resolves a build failure that occurred when building the package
with pip/nox due to pip's use of isolated and changing build environments.

Problem:
- CMake was using find_library() to get the absolute path to libcasadi.dylib
  during configuration in one temporary pip build environment
- By the time the actual build ran, pip had switched to a different temporary
  build environment, making the hard-coded library path invalid
- This caused: "No rule to make target .../libcasadi.dylib" error

Solution:
- Link against CasADi by library name instead of absolute path, relying on
  target_link_directories() to provide the search path during build
- Use relative RPATH (@loader_path on macOS, $ORIGIN on Linux) so the built
  module can find CasADi in the same Python environment at runtime
- Set BUILD_RPATH for the build environment and INSTALL_RPATH for runtime
- Remove duplicate casadi link (already added in USE_PYTHON_CASADI section)
@BradyPlanden
tests: adds initial test suite w/ basic functionality
7551a6d
@BradyPlanden
infra: adds __version__ to package, adds nox to dev dependencies w/ u…
8dd3ea2 
…v backend preference
@BradyPlanden
tests: adds benchmark session, uv backend for nox, updates to unit su…
90ca76c 
…ite, integration_tests.yml uses nox session, updates docs
@BradyPlanden
benchmarks: remove icons
ebffa02
@BradyPlanden BradyPlanden requested a review from a team as a code owner October 13, 2025 09:18
agriyakhetarpal
agriyakhetarpal reviewed Oct 13, 2025
View reviewed changes
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.

I know that this is still WIP but thanks for taking this up, there are lots of useful additions!

CMakeLists.txt
Comment on lines +175 to +177
# Link against casadi by name, not absolute path, to avoid issues with
# pip's isolated build environments changing paths between configure and build
target_link_libraries(idaklu PRIVATE casadi)
Copy link
Member

Choose a reason for hiding this comment

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

All good catches here, thanks! and sorry for the trouble here, I realise I was responsible for all this 😄

martinjrobins
martinjrobins requested changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

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

Thanks @BradyPlanden :) The benchmarks and nox env look really useful. I'm not sure on the tests though. Most of them are very superficial (checking if function exists, is callable), many look like they can't possibly fail, some are repeats of other tests, and the vector tests are testing code that isn't in pybammsolvers, this is just pybind11 code. I would prefer fewer tests that actually test the internals of the C++ code that we wrote.

Saransh-cpp
Saransh-cpp previously approved these changes Oct 13, 2025
View reviewed changes
@BradyPlanden
Copy link
Member Author

BradyPlanden commented Oct 13, 2025 •
edited
Loading

Most of them are very superficial (checking if function exists, is callable), many look like they can't possibly fail,

Yes, I fully agree. The main goal of this PR was to set up the testing suite; as such, I let Cursor throw together the unit tests. I'll open an additional PR to remove these low-quality tests and add a few useful ones. Suggestions welcome, or feel free to open a PR with pybammsolver-specific tests!

@agriyakhetarpal
Copy link
Member

I'll open an additional PR to remove these low-quality tests and add a few useful ones. Suggestions welcome

I previously discussed this with @MarcBerliner, and we think it would be nice to have a two-way testing structure with nightlies, quoting it below:

the idea on a high level is that we clone the PyBaMM repo in the pybammsolvers workflows for testing, but we don't do so for pybammsolvers in the PyBaMM repo (as building pybammsolvers wheels takes a long time). With nightly wheels, we can enable running tests against a dev version of pybammsolvers in PyBaMM, and against a dev version of PyBaMM in pybammsolvers, covering both directions. These tests would run on a schedule and not on PRs. There is another action that I've been using at https://github.com/scientific-python/issue-from-pytest-log-action, which will allow us to report any failures as GitHub issues, and we can resolve them as applicable. Most people in the Scientific Python ecosystem use Anaconda.org for this, as it provides storage and a PyPI-conformant wheel index for free. Here's the original issue where we discussed this: pybamm-team/PyBaMM#3251

martinjrobins
martinjrobins previously approved these changes Oct 13, 2025
View reviewed changes
Copy link
Contributor

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

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

ok, happy to approve given the tests are being replaced later

@agriyakhetarpal agriyakhetarpal mentioned this pull request Oct 15, 2025
Merged
BradyPlanden and others added 3 commits October 16, 2025 13:45
@BradyPlanden @agriyakhetarpal
Suggestions from review
728ec40 
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@BradyPlanden
precommit additions
dc5427f
@BradyPlanden
fix: avoid single-element vector operations for MacOS-intel
a204965
@BradyPlanden
Copy link
Member Author

I'm hitting an issue specific to intel-based MacOS runners. I've tried changing the rhs definition syntax and upgrading Casadi to 3.7.2, but neither worked. Cursor suggests it's a Casadi bug—we don't see this on pybamm-tests. We can skip these tests for macOS-intel, but fix suggestions are welcome! @agriyakhetarpal @martinjrobins @Saransh-cpp @MarcBerliner

@agriyakhetarpal
Copy link
Member

Hi @BradyPlanden, I am unable to see the logs for some reason (GitHub is unable to load them, nor do the raw logs work), so I've retriggered the workflow run. Let's see how it goes!

@agriyakhetarpal
Copy link
Member

I'm pretty sure this is an upstream issue. I'm okay with skipping the affected tests on macos-15-intel (i.e., when sys.platform == "darwin" and platform.machine() == "arm64").

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

Reviewers

@agriyakhetarpal agriyakhetarpal agriyakhetarpal left review comments

@martinjrobins martinjrobins martinjrobins left review comments

@Saransh-cpp Saransh-cpp Saransh-cpp left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@BradyPlanden @agriyakhetarpal @martinjrobins @Saransh-cpp