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

DRAFT: rewrite c++ wrappers using nanobuild #591

Closed
wants to merge 39 commits into from
Closed

Conversation

ncullen93
Copy link
Member

@ncullen93 ncullen93 commented Mar 22, 2024

This draft PR completely revamps the c++ library wrappers for antspy. It uses nanobuild which is a greatly improved version of pybind11.

The positive effects of this PR are the following:

  • all c++ code moves to its own src/ dir instead of ants/lib/ dir... having c++ code in the python directory is an anti-pattern and confused a lot of people
  • all lib code hidden from users.. almost no chance they accidentally import it
  • wheel size greatly decreased. i estimate it will be between 50 - 100 mb instead of 250+ mb now.
  • much faster to import antspy in the console. with the existing pybind11 wrappers it takes 30+ seconds the very first time antspy is installed and maybe 5 seconds each subsequent time. this will drop that to an estimated <15 seconds and <2 seconds to import. it's very quick.
  • all pixeltypes and dimensions for itk will be supported. previously we had to compromise because more pixeltypes meant much larger build sizes. Now that this moves away from the templating paradigm, we can support everything
  • on that note... greatly simplified library interface. previously there were a million different library calls for the same function (e.g., lib.ThresholdImageF2(img), lib.ThresholdImageF3(img), etc... now it will be one function, e.g. lib.ThresholdImage(img, 'F2')
  • uses modern built system (pyproject.toml) instead of relying solely on setup.py

Potential negative effects:

  • unsure how this will affect windows builds or if it will make it harder for people to install antspy... however, we are not perfect at that anyways right now
  • CI / GHA workflows may need to be reworked a bit

Current difficulties:

  • Because this uses a very modern build system, I have NO idea how to run prebuild bash scripts to configure ITK and ANTs. I have added those bash calls into the CMakeLists.txt file for now but I'd like a better solution. The current method we use of calling two bash scripts in setup.py seems like a hacky anti-pattern though, so ideally ITK would be built together with this project (like a superbuild?). I have tried a ton of things with no success.

Testing this:

pip install . 

CMakeLists.txt Outdated
@@ -0,0 +1,51 @@
cmake_minimum_required(VERSION 3.15...3.26)
Copy link
Member

Choose a reason for hiding this comment

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

I would make the minimum 3.16.3, because that's the minimum for ITK. ITK also specifies a maximum of 3.19 but I have very rarely had trouble building with newer versions

@cookpa cookpa mentioned this pull request Apr 2, 2024
@ncullen93 ncullen93 closed this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants