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

Move PyChaste Bindings into Chaste Trunk #46

Closed
kwabenantim opened this issue Oct 12, 2023 · 9 comments · Fixed by Chaste/Chaste#286
Closed

Move PyChaste Bindings into Chaste Trunk #46

kwabenantim opened this issue Oct 12, 2023 · 9 comments · Fixed by Chaste/Chaste#286
Assignees
Labels
enhancement New feature or request

Comments

@kwabenantim
Copy link
Member

kwabenantim commented Oct 12, 2023

Summary
It might be a good idea to consider moving the Python bindings into the main Chaste trunk as the bindings are tightly coupled to upstream Chaste source.

Advantages:

  • Easier to keep bindings in synch with develop branch and release versions.
  • Bindings tests can be added to the CI on PRs to check if bindings break when other parts of Chaste are updated.
  • Simpler to maintain a single documentation website.

Disadvantages:

  • All developers will need to learn some python and pybind11 as well to test changes to bindings?
@kwabenantim kwabenantim added the enhancement New feature or request label Oct 12, 2023
@kwabenantim kwabenantim added this to the BBR milestone 3.1 milestone Oct 12, 2023
@kwabenantim kwabenantim added the long term Non-urgent issue that needs solving eventually label Oct 12, 2023
@mirams mirams added the question Further information is requested label Oct 12, 2023
@kwabenantim kwabenantim changed the title Move PyChaste Bindings into Chaste Trunk Move PyChaste Bindings into Chaste Trunk? Oct 12, 2023
@kwabenantim kwabenantim changed the title Move PyChaste Bindings into Chaste Trunk? Move PyChaste Bindings into Chaste Trunk Nov 7, 2023
@kwabenantim
Copy link
Member Author

kwabenantim commented May 28, 2024

Discussion @ Sheffield Hackathon

Coverage Testing

Add tests to check if there are new classes in cell_based without wrappers

Test against comprehensive list of supported use cases (100% coverage shouldn't be required initially for wrappers).

Configuring

Pybind11
Do not add Pybind11 code into trunk

Preferences in order:

  1. Use scikit-build (link) to get pybind11
  2. Use cmake fetchcontent
  3. Do a pip install into the chaste_codegen Python env (rename env?)

Pull latest pybind11 stable version to keep up to date with latest Python and C++ version support.

cppwg
Do a pip install into the chaste_codegen Python env

Conda packaging

Add Chaste developers to anaconda pychaste channel

Use GitHub actions for building conda packages

Build for Linux/MacOS amd64 and arm64 (Linux arm64 on self-hosted)

Docker

Merge into Chaste docker

Developer Documentation

  • Generating wrappers automatically
  • Explanation of contents of configuration file
  • Code inside wrappers
  • Additional code e.g. for custom PyChaste visualisation
  • Fixing common problems

@kwabenantim
Copy link
Member Author

kwabenantim commented Aug 20, 2024

Directory structure:

Chaste/pychaste
│
├── dynamic
│   ├── config.yaml
│   ├── templates
│   └── wrappers
│       └── lib
│           ├── *.cppwg.cpp
│           └── *.cppwg.hpp
├── src
│   ├── cpp
│   │   ├── cell_based
│   │   └── visualization
│   └── py
│       ├── chaste
│       │   ├── cell_based
│       │   ├── core
│       │   ├── external
│       │   ├── mesh
│       │   ├── ode
│       │   ├── pde
│       │   └── visualization
│       ├── docs
│       │   ├── api
│       │   └── tutorial
│       ├── LICENSE
│       ├── MANIFEST.in
│       ├── pyproject.toml
│       ├── setup.cfg
│       └── setup.py
└── test
    ├── cell_based
    ├── core
    ├── data
    ├── mesh
    ├── ode
    ├── tutorial
    └── visualization

@kwabenantim kwabenantim removed the long term Non-urgent issue that needs solving eventually label Aug 20, 2024
@kwabenantim
Copy link
Member Author

kwabenantim commented Sep 17, 2024

Notes from Nottingham Hackathon

  • Add xvfb to recommended packages in chaste-dependencies
  • More informative error message about files and methods that needs changing
  • Error message should say how to fix the problem
  • Put PyChaste dev documentation on developer wiki
  • Update PyChaste web pages (https://github.com/Chaste/Chaste.github.io/pull/101)
  • Create a pychaste-dependencies ubuntu package in addition to chaste-dependencies

@bdevans
Copy link
Member

bdevans commented Sep 19, 2024

Rather than listing python3-notebook as a dependency (which I believe is deprecated), shouldn't we use jupyter lab? It might in general be better to install python packages with pip (in a virtual environment) rather than through (slowly updated) ubuntu packages.

@kwabenantim
Copy link
Member Author

Thanks @bdevans. I'll update the package list.

@bdevans
Copy link
Member

bdevans commented Oct 9, 2024

Hi @kwabenantim, I made some progress on merging this in but I think it is blocked until the revised chaste-dependencies package (which fixes the libexpat1 bug) supports arm64. The error message is as follows:

The following packages have unmet dependencies:
 chaste-dependencies : Depends: libvtk9-dev (= 9.3.0+dfsg1-1build1) but it is not going to be installed
E: Unable to correct problems, you have held broken packages.

Is that something you can look at or is it a job for @fcooper8472?

See also:

  1. Merge PyChaste installation steps into Chaste docker images chaste-docker#21
  2. [WIP] Merge PyChaste chaste-docker#22

@kwabenantim
Copy link
Member Author

Hi @bdevans. I think @fcooper8472 has been working on this. Could it be fixed temporarily by something similar to the changes in Chaste#321?

@bdevans
Copy link
Member

bdevans commented Oct 10, 2024

Thanks @kwabenantim, I had no luck with that so I'll wait to see what @fcooper8472 says.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants