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

ENH: move c++ wrapping from pybind11 to nanobind #621

Merged
merged 26 commits into from
May 14, 2024
Merged

Conversation

ncullen93
Copy link
Member

@ncullen93 ncullen93 commented May 8, 2024

This PR primarily moves the C++ wrapping framework from pybind11 to nanobind. This should improve the project by making the build process faster, the wheels smaller, and the initial "import ants" statement quicker. Moreover, any c++ functions that take in only an ants image are now overloaded so the can be called without appending pixel type + dimension suffixes to the name.

The c++ source code has now been moved outside of the python module folder, as it should be, and the package now relies on a modern pyproject.toml file instead of setup.py.

No user-facing functions have been change and internal python code has been minimally altered.

All tests pass locally on mac and on the github actions. However, I have not tested that a windows build would work because only the ITK and ANTs setup scripts for linux are currently called.

@ncullen93 ncullen93 changed the title ENH: move from pybind11 to nanobind DRAFT: move from pybind11 to nanobind May 8, 2024
@coveralls
Copy link

coveralls commented May 8, 2024

Coverage Status

coverage: 80.524% (+0.1%) from 80.382%
when pulling b3c95f2 on nanobuild-v2
into 150e6f6 on master.

@ncullen93 ncullen93 changed the title DRAFT: move from pybind11 to nanobind ENH: move c++ wrapping from pybind11 to nanobind May 12, 2024
@ncullen93 ncullen93 requested a review from cookpa May 13, 2024 13:59
@ncullen93
Copy link
Member Author

@cookpa All the existing unit tests pass with the new wrapping and it should be good to go. The one thing I'm unsure of is the windows build. Particularly these lines

execute_process(COMMAND bash ${PROJECT_SOURCE_DIR}/scripts/configure_ITK.sh)

The CMakeLists.txt either needs to have some conditional statement inside it or the ants + itk setup scripts need to be moved to the pyproject.toml file to be run before the cmakelists.txt (like the current setup), again with conditional logic. The third option is to have the OS conditional logic inside the ants + itk setup scripts... that's maybe easiest.

@cookpa
Copy link
Member

cookpa commented May 13, 2024

@cookpa All the existing unit tests pass with the new wrapping and it should be good to go. The one thing I'm unsure of is the windows build. Particularly these lines

execute_process(COMMAND bash ${PROJECT_SOURCE_DIR}/scripts/configure_ITK.sh)

The CMakeLists.txt either needs to have some conditional statement inside it or the ants + itk setup scripts need to be moved to the pyproject.toml file to be run before the cmakelists.txt (like the current setup), again with conditional logic. The third option is to have the OS conditional logic inside the ants + itk setup scripts... that's maybe easiest.

I think CMake can do it:

if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
  execute_process(COMMAND cmd /c ${PROJECT_SOURCE_DIR}/scripts/configure_ITK.bat)
else
  execute_process(COMMAND bash ${PROJECT_SOURCE_DIR}/scripts/configure_ITK.sh)
endif()

Copy link
Member

@cookpa cookpa left a comment

Choose a reason for hiding this comment

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

Builds on my machine, definitely faster. Many thanks for this huge contribution

@cookpa
Copy link
Member

cookpa commented May 13, 2024

Docker builds too on Linux

@ncullen93
Copy link
Member Author

Ok great, thanks. I will add that.

@ncullen93 ncullen93 merged commit 0903c42 into master May 14, 2024
2 checks passed
@ncullen93 ncullen93 deleted the nanobuild-v2 branch May 16, 2024 15:42
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.

3 participants