Skip to content

feat: Nanobind for python bindings (first steps -- pybind11 still working)#5084

Merged
lgritz merged 10 commits intoAcademySoftwareFoundation:mainfrom
soswow:nanobind-migration
Apr 30, 2026
Merged

feat: Nanobind for python bindings (first steps -- pybind11 still working)#5084
lgritz merged 10 commits intoAcademySoftwareFoundation:mainfrom
soswow:nanobind-migration

Conversation

@soswow
Copy link
Copy Markdown
Contributor

@soswow soswow commented Mar 12, 2026

First PR on the way to a migration from pybind11 to nanobind.

New env flag introduced OIIO_PYTHON_BINDINGS_BACKEND = pybind11 or nanobind or both

Select which Python binding backend(s) to configure. both keeps the existing pybind11 module and also builds the experimental nanobind module.

When it is nanobine one is build it is in PyOpenImageIONanobindExperimental target.

cmake --fresh -S . -B build \
  -DUSE_CCACHE=OFF \
  -Dfmt_DIR=/opt/homebrew/lib/cmake/fmt \
  -DOpenColorIO_DIR=/opt/homebrew/lib/cmake/OpenColorIO \
  -DOIIO_INTERNALIZE_FMT=ON \
  -DOIIO_PYTHON_BINDINGS_BACKEND=both

Checklist:

  • I have read the guidelines on contributions and code review procedures.
  • I have updated the documentation if my PR adds features or changes
    behavior.
  • I am sure that this PR's changes are tested somewhere in the
    testsuite
    .
  • I have run and passed the testsuite in CI before submitting the
    PR, by pushing the changes to my fork and seeing that the automated CI
    passed there. (Exceptions: If most tests pass and you can't figure out why
    the remaining ones fail, it's ok to submit the PR and ask for help. Or if
    any failures seem entirely unrelated to your change; sometimes things break
    on the GitHub runners.)
  • My code follows the prevailing code style of this project and I
    fixed any problems reported by the clang-format CI test.
  • If I added or modified a public C++ API call, I have also amended the
    corresponding Python bindings. If altering ImageBufAlgo functions, I also
    exposed the new functionality as oiiotool options.

This code contribution was assisted by Cursor/Composer-2

@soswow soswow force-pushed the nanobind-migration branch from 695dc8b to 576cb19 Compare March 15, 2026 11:32
@soswow soswow marked this pull request as ready for review March 16, 2026 00:58
@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented Mar 18, 2026

@soswow This is looking good!

The only real concern I have at this stage is that rather than the logic for running the nanobind tests living in runtest.py, I would much rather that it all be in testing.cmake.

You can see (circa testing.cmake:197) how for each of the texture tests, it actually adds them to the list a second time with different arguments to make the same test run again with in batch texturing mode (and modifying the name to append ".batch" to make it look like a separate test). I think this is how it should work for pybind11 vs nanobind as well. There should be minimal logic changes necessary in runtest.py.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented Mar 30, 2026

@soswow Ping. I proposed some changes to the testing strategy, but otherwise would like to merge what you have as a great first step to the nanobind transition.

@soswow soswow force-pushed the nanobind-migration branch from 11448cc to 583aea9 Compare April 18, 2026 05:19
Comment thread src/python-nanobind/py_imagespec.cpp Outdated
Comment thread src/python-nanobind/py_paramvalue.cpp Outdated
return std::string(spec.serialize(fmt, verb));
},
"format"_a = "text", "verbose"_a = "detailed")
.def(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The old pybind11 has a few other functions right before this and right after serialize. Seems like the following 4 were somehow missed?

        .def("to_xml",
             [](const ImageSpec& spec) { return PY_STR(spec.to_xml()); })
        .def("from_xml", &ImageSpec::from_xml)
        .def(
            "valid_tile_range",
            [](ImageSpec& self, int xbegin, int xend, int ybegin, int yend,
               int zbegin, int zend) {
                return self.valid_tile_range(xbegin, xend, ybegin, yend, zbegin,
                                             zend);
            },
            "xbegin"_a, "xend"_a, "ybegin"_a, "yend"_a, "zbegin"_a = 0,
            "zend"_a = 1)
        .def("copy_dimensions", &ImageSpec::copy_dimensions, "other"_a)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah. fair. I am pretty sure it was minimal impl. to satisfy existing tests to pass.

I will add those in (And couple more things that got left behind) and see if I Can have regression tests that would have failed if these things are not added.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the test coverage! The files looked very complete so it seemed like the 4 were accidentally forgotten and it would have been more difficult to notice later on I think. I think, for future nanobind changes that might come in more than 1 piece, any APIs remaining should be clearly marked as a TODO so it's more obvious we can't quite ship it yet.

... Hmm, just thinking out loud: I wonder if we could add coverage for this actually. We could dump the API surface with another test that does something similar to:

import pprint
pprint.pprint(dir(OpenImageIO.ImageSpec))
pprint.pprint(dir(OpenImageIO.ImageBuf))

And that way we would have a good comparison against the new nanobind API surface and it would clearly show the differences. Unfortunately that would mean having to completely implement each class in a single PR...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

how about using existing stub at src/python/stubs/OpenImageIO/__init__.pyi for that?

@soswow
Copy link
Copy Markdown
Contributor Author

soswow commented Apr 26, 2026

@jessey-git thx for taking a look. See last 2 commits.

Copy link
Copy Markdown
Contributor

@jessey-git jessey-git left a comment

Choose a reason for hiding this comment

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

I'm approving from my side though do fixup that clang-format issue from the last commit.

@soswow
Copy link
Copy Markdown
Contributor Author

soswow commented Apr 27, 2026

I wonder if we want those tests to run with nanobind backend in CI? It only happens when OIIO_PYTHON_BINDINGS_BACKEND is both or nanobind, which it is not in CI.
I mean it obv. should at some point. The Q is - should it be this PR?

Comment thread INSTALL.md Outdated
Comment on lines +45 to +49
* For the experimental nanobind migration backend:
* NumPy (tested through 2.2.4)
* For the nanobind (WIP) migration backend used in source/CMake builds:
* nanobind discoverable by CMake, or installed in the active Python
environment so `python -m nanobind --cmake_dir` works
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this a little messed up? You have two "for the nanobind..." headers, and numpy ends up listed twice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah. will fix that now

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented Apr 27, 2026

These all look fine to me. I made a minor comment or two that you can address in the next round if you wish. This is more or less what I pictured the nanobind bindings looking like.

Let me know if you are ready for me to merge what you have so far, and then we'll do the subsequent rounds on top of it.

Again, does not have to be in this PR, but if not then soon: I think you should check in some kind of design checklist (could be in docs/dev? or as comments at the top of one of the important headers like py_oiio.h?) that enumerates what's done so far and what's left to do, that we can be checking off and adding to with subsequent PRs so it's easy to tell from a glance what is the current status as well as what jobs might be done in parallel by different people, in case anybody is in a position to help.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented Apr 27, 2026

Naive question -- Do any of our CI job variants test the nanobind path?

@soswow
Copy link
Copy Markdown
Contributor Author

soswow commented Apr 27, 2026

Naive question -- Do any of our CI job variants test the nanobind path?

yeah, as I mentioned in above message - I don't think so. I feel like it is rather important

@soswow
Copy link
Copy Markdown
Contributor Author

soswow commented Apr 27, 2026

@lgritz

I made a minor comment or two

which is it? I think I've addressed those made on march 19

@soswow soswow force-pushed the nanobind-migration branch 2 times, most recently from 4efafe0 to 47ec96d Compare April 27, 2026 07:38
Comment thread src/build-scripts/ci-startup.bash Outdated
export CC=${CC:=gcc}
export CXX=${CXX:=g++}
export OpenImageIO_CI=1
export OpenImageIO_CI=true
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not sure what happened here. Checking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

reverted.

@soswow soswow force-pushed the nanobind-migration branch from ba2420f to 0c6daad Compare April 27, 2026 07:56
@soswow
Copy link
Copy Markdown
Contributor Author

soswow commented Apr 27, 2026

I think you should check in some kind of design checklist (could be in docs/dev? or as comments at the top of one of the important headers like py_oiio.h?) that enumerates what's done so far and what's left to do, that we can be checking off and adding to with subsequent PRs so it's easy to tell from a glance what is the current status as well as what jobs might be done in parallel by different people, in case anybody is in a position to help.

Check out src/python-nanobind/MIGRATION_STATUS.md what do you think?

Comment thread src/libutil/hash_test.cpp Outdated
// fill data with random values so we can hash it a bunch of different ways
std::mt19937 rng(42);
data.resize(round_to_multiple(iterations, 4) / sizeof(data[0]), 0);
data.resize(iterations / sizeof(data[0]), 0);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

prob. missed merge mistake. investigating.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeap. reverted that too.

Comment thread src/cmake/testing.cmake Outdated
# The optional ENVIRONMENT is a list of environment variables to set for the
# test.
#
# The optional ENVIRONMENT_MODIFICATION is a list of environment variable
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This feature apperently requires CMake higher then 3.18.2 which is one in this project.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've made some changes not to have dependency on this feature so it will work on 3.18.2

Copy link
Copy Markdown
Collaborator

@lgritz lgritz Apr 27, 2026

Choose a reason for hiding this comment

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

That feature needed 3.22. Something to consider for the OIIO 3.2 release. Requires checking on other projects in the ecosystem and what they need. If lots of others require that or newer, maybe it's time to bump our minimum again. I'm writing this more as a reminder to myself, not as instructions for you.

soswow added 8 commits April 27, 2026 22:03
…I, TypeDesc)

Build as optional oiio_python_nanobind target; share tests with the pybind11
module. Documents migration status in src/python-nanobind/MIGRATION_STATUS.md.

Assisted-by: Cursor/Composer-2

Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Made-with: Cursor
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Made-with: Cursor
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
…ther table

Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
@soswow soswow force-pushed the nanobind-migration branch from 84d64c0 to b2ef3a2 Compare April 27, 2026 12:06
@soswow
Copy link
Copy Markdown
Contributor Author

soswow commented Apr 27, 2026

@jessey-git Unfortunately I've found some more divergent / missed / gaps. Feel free to see latest commits. I've squished (I believe before I Started making new changes) and other commits are all new stuff.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented Apr 27, 2026 via email

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented Apr 27, 2026

Thanks for the migration status document. Looks good.

The failing tests (which hit an assertion) are the only thing left to fix before I think we should merge and then build on top of it. I'd like to stop the scope creep on this initial PR as soon as we can.

After the merge, I believe the very highest priority is rigging it so that we are testing the nanobind path for at least some of the CI test (ideally at least one variant for each OS platform). Then we can move forward with confidence, knowing that downstream users who try building with nanobind will at least never have broken builds.

soswow added 2 commits April 29, 2026 20:21
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
Signed-off-by: Aleksandr Motsjonov <soswow@gmail.com>
@soswow soswow force-pushed the nanobind-migration branch from 1c23a9c to 562f84e Compare April 29, 2026 10:22
@soswow
Copy link
Copy Markdown
Contributor Author

soswow commented Apr 29, 2026

ok folks, take a look at two last commits.

  • one is about fixing build on windows
  • other contains actual bugs (that's why pybind is touched) that got revieled when I've added more test cases. And also I had to add some more migration into py_oiio.h to make tests pass.

@lgritz
Copy link
Copy Markdown
Collaborator

lgritz commented Apr 29, 2026

All tests passing, looks good to me as a first step to nanobind.
Does anybody think this shouldn't get merged today?

@lgritz lgritz changed the title Nanobind migration POC feat: Nanobind for python bindings (first steps -- pybind11 still working) Apr 29, 2026
@soswow
Copy link
Copy Markdown
Contributor Author

soswow commented Apr 29, 2026

I don't mind. Happy to start looking into making nanobind tests run in CI next

@lgritz lgritz merged commit 409621b into AcademySoftwareFoundation:main Apr 30, 2026
63 checks passed
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