python: switch binding library to nanobind - #948
Conversation
|
Sorry for not reacting to your PR so far. This is definitively interesting, I just don't have the time right now to properly look into this. But I will at some point in time. |
|
No worries, take your time! |
|
Does nanobind require switching the whole build backend from setuptools to scikit-build? If not, what are the reasons for this change and which benefits and side effects does this have? |
|
@Doekin Sorry for letting this PR wait so long without feedback. I plan to make a 2.4 release soonish, so I'm trying to catch up... Are you still around to discuss details? Would you be willing to fix the conflicts? Do have you have something to say to the question from @stefan6419846 ? |
|
Thanks for checking in. Yes, I'll update the PR and fix the conflicts. As for the build backend: It's not mandatory, but |
|
With this changes, it seems like the build is broken - before, building the source distribution worked fine. Running |
| # Target the stable ABI for Python 3.12+, which reduces | ||
| # the number of binary wheels that must be built. This | ||
| # does nothing on older Python versions | ||
| STABLE_ABI |
There was a problem hiding this comment.
That sounds interesting. Do you know how the publish-pyhton.yml file would need to change to benefit from that? I'd also like to point out this commit by @chamalgomes. Which is touching on the same issue, at least in terms of build-times.
There was a problem hiding this comment.
Typically, cibuildwheel handles the build process automatically without needing much configuration.
| set(PYBIND11_FINDPYTHON ON) # see https://github.com/pybind/pybind11/issues/4785 | ||
| zxing_add_package(pybind11 pybind11 https://github.com/pybind/pybind11.git v3.0.1) | ||
| # Try to import all Python components potentially needed by nanobind | ||
| find_package(Python 3.8 |
| set(ZXING_PYTHON_INSTALL_BINDIR "${CMAKE_INSTALL_BINDIR}") | ||
| if (SKBUILD) | ||
| set_target_properties(zxingcpp PROPERTIES | ||
| LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} |
There was a problem hiding this comment.
Does this really need to be an in-source-tree build?
There was a problem hiding this comment.
It doesn't strictly have to be an in-source build. I currently set it up this way primarily to mimic the output directory structure of setup.py.
There was a problem hiding this comment.
Why would that be a good thing to aim for? (sorry for the potentially stupid question)
There was a problem hiding this comment.
My intention was to minimize friction for existing users by keeping the output structure consistent with the old setup.py
| LIBRARY DESTINATION "${ZXING_PYTHON_INSTALL_LIBDIR}") | ||
| install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/init.py | ||
| COMPONENT python | ||
| RENAME __init__.py |
There was a problem hiding this comment.
This file is generated by nanobind, right? Why would they name it init.py when it has to be renamed anyway?
There was a problem hiding this comment.
This file is manually added to support the package structure. Since zxingcpp is installed as a package directory containing the extension module, this init.py (renamed to __init__.py during install) ensures import zxingcpp works correctly and avoids import errors when running Python from the source directory.
| "pybind11[global]", | ||
| ] | ||
| build-backend = "setuptools.build_meta" | ||
| requires = ["scikit-build-core >=0.10", "nanobind >=1.3.2", "typing_extensions >=4.12;python_version<\"3.11\""] |
There was a problem hiding this comment.
What is typing_extension used for and why does the python_version need to be < 3.11?
|
OK, I'll try to address these in the next few days. |
|
Re: @stefan6419846 : Perhaps it is the same issue as scikit-build/scikit-build-core#801 ? |
|
I see you have pushed another changeset 4 weeks ago but have not answered any of my questions. Also, as you can see, there have been changes to the python wrapper code due to the new |
|
Apologies for the silence and the delay. I've been trying to adapt setuptools to work with nanobind, but it turned out to be more complicated than expected. I also looked into copying the symlinked folders as an alternative, but that approach has its own issues. I am finalizing my findings and will present two possible solutions shortly (later today/tomorrow) for you to decide which is best. |
1256f0c to
76a8a91
Compare
|
sorry for introducing more conflicts while you are working on it (:/) and thanks for keeping this moving forward. |
|
Strangely, the |
axxel
left a comment
There was a problem hiding this comment.
I went through it line by line again and a few new questions came up.
| return nb::cast<BarcodeFormats>(formats); | ||
| if (nb::isinstance<nb::list>(formats)) { | ||
| std::vector<BarcodeFormat> list; | ||
| for (auto fmt : nb::cast<nb::list>(formats)) |
There was a problem hiding this comment.
This would throw something if the list did actually not contain BarcodeFormat objects, right? Maybe there is an even neater option to directly test if isinstance<nb::vector<BarcodeFormat>> and then cast into it directly?
| .returnErrors(return_errors); | ||
|
|
||
| if (py::isinstance<ImageView>(_image)) { | ||
| if (ImageView _imageview; nb::try_cast(_image, _imageview)) { |
There was a problem hiding this comment.
not a big deal but I'd find the diff (and the code) more readable if it stayed with the old pattern (isinstance -> cast).
| auto adata = ai["data"]; | ||
|
|
||
| if (py::isinstance<py::buffer>(adata)) { | ||
| if (nb::try_cast(ai["data"], arr)) { |
There was a problem hiding this comment.
why not use adata as before? I believe adata is not an unused variable? [here I get the try_cast since you have the arr object already...]
| info.strides = py::detail::c_strides(shape, 1); | ||
| } | ||
| } else if (py::isinstance<py::tuple>(adata)) { | ||
| if (arr.ndim() != Size(shape)) |
There was a problem hiding this comment.
since ndim() seems to be size_t, shape.size() is the better call here?
| } else if (py::isinstance<py::tuple>(adata)) { | ||
| if (arr.ndim() != Size(shape)) | ||
| arr = | ||
| nb::ndarray<nb::ro>{arr.data(), shape.size(), shape.data(), nb::find(arr), nullptr, nb::dtype<uint8_t>()}; |
There was a problem hiding this comment.
I don't understand the full model behind the ownership management of nb::ndarrays but are you sure that tying the lifetime of the newly created object to the one you are destroying with the assignment to 'itself' is doing what you want? I assume there is no option to simply change the shape property of the existing object?
There was a problem hiding this comment.
nanobind::ndarray doesn't support in-place shape modification. The key here is the fourth parameter (owner), which is a Python handle. When casting to ndarray, nanobind internally increments the reference counter, ensuring the original object's lifetime is properly managed.
| true // read-only | ||
| }; | ||
| }); | ||
| PyType_Slot ImageView_slots[] = { |
There was a problem hiding this comment.
I'm not mentally fit enough right now to form an opinion on whether or not this is actually a functional replacement of the old code. I guess I need to make sure the unit tests really cover all corner cases...
There was a problem hiding this comment.
Maybe we could add a few tests to convert Image/ImageView to numpy array and OpenCV images and verify the content, basically where the buffer protocol is used.
| # "setuptools-scm>=8", | ||
| "scikit-build-core >=0.11", | ||
| "wheel", | ||
| "cmake>=3.18", |
There was a problem hiding this comment.
why is that depencency gone?
| OPTIONAL_COMPONENTS Development.SABIModule) | ||
|
|
||
| # Locate nanobind installed via pip or conda | ||
| execute_process( |
There was a problem hiding this comment.
so find_package(nanobind) does not do this properly?
There was a problem hiding this comment.
In my own experiment here, it worked with the Python build jobs, but then failed on the regular build jobs, even with a call to zxing_add_package. I'm not sure why yet. I don't love this either, but that might be the reason for the moment.
In the Python build jobs, the pyproject.toml ensures that nanobind is installed, so find_package just works. It's when the -DZXING_PYTHON_MODULE=ON is set, I think
| RUNTIME DESTINATION "${ZXING_PYTHON_INSTALL_BINDIR}" | ||
| LIBRARY DESTINATION "${ZXING_PYTHON_INSTALL_LIBDIR}" | ||
| ARCHIVE DESTINATION "${ZXING_PYTHON_INSTALL_LIBDIR}") | ||
| COMPONENT python |
There was a problem hiding this comment.
please revert the indentation changes
You mean the [yes, the broken kn ci-build has nothing to do with your code] |
|
Regarding the The built module then becomes
|
| {Py_bf_releasebuffer, (void*)releasebuffer}, | ||
| {0, nullptr}}; | ||
|
|
||
| nb::class_<ImageView>(m, "ImageView", nb::type_slots(Image_slots), nb::is_weak_referenceable()) |
There was a problem hiding this comment.
This is using Image_slots on ImageView
There was a problem hiding this comment.
I have not looked into the nanobind doc for how to most naturally manage to return a 'buffer'. From the outside, it looks like this Py_type_slot approach is way below the abstraction level otherwise provided by the library. If you have knowledge on how to improve the situation. Please let us know.
There was a problem hiding this comment.
Is this what you are looking for?
https://nanobind.readthedocs.io/en/latest/ndarray.html#returning-arrays-from-c-to-python
There was a problem hiding this comment.
Good catch! Fixed. It should indeed be ImageView_slots.
There was a problem hiding this comment.
The link you mentioned describes returning arrays from C++ to Python, which is a different use case. Here, we need to implement the buffer protocol so that Python can access the C++ object's memory directly. This is essential for np.array(image) patterns.
nanobind doesn't provide a higher-level abstraction for this specific functionality.
There was a problem hiding this comment.
Thanks for the info, just to add more background to the others after I did some research:
- Use type slots to expose the native buffer protocol, or implement
__array__only for numpy: Alternative to buffer protocol wjakob/nanobind#478 (comment). The doc link is stale and the latest is https://nanobind.readthedocs.io/en/latest/lowlevel.html#customizing-type-creation - Examples: replacing def_buffer wjakob/nanobind#699 (comment)
| "wheel", | ||
| "cmake>=3.18", | ||
| "pybind11[global]", | ||
| "nanobind >=1.3.2", |
There was a problem hiding this comment.
Maybe just set to the latest?
nanobind >=2.11.0
There was a problem hiding this comment.
Here this is a hard build requirement, right? So is it really the best option to require the latest version even when older (potentially locally installed) versions are just fine as well?
There was a problem hiding this comment.
Perhaps we can lower it to v2.2.0? This version is a good baseline because it introduces support for free-threaded Python 3.13.
We will lose some newer quality-of-life improvements (like the better cast error reporting in v2.9.0), but the core functionality will work just fine. Does v2.2.0 sound like a reasonable compromise?
There was a problem hiding this comment.
Just my 2 cents:
Insteas of hard build requirements, another angle is to treat this as an alignment to the CMakeList/prod build environment, so that local build aligns more with the production artifact, and I don't see the downsides of keeping build dependencies to the latest except for updating local (virtual) Python environments, including cmake. With that being said, using uv with lock file might serve better on version control and reproducibility.
Or if it is indeed intended for minimum build requirements, we could still start with the current latest (the version this project starts to use and tests) and keep it until we have to update it.
|
Hi @axxel, I wonder how to move this PR forward. There were several review comments not marked as resolved but may be outdated, so it is a bit unclear to me whether this is pending from your side or from @Doekin. Are there any particular concerns? Any replies/changes you are waiting on? Or if you were just busy with other changes and need to revisit this later. Hopefully this could be made into the 3.0 release, thanks! |
I need to go over the comments again and see what is still relevant. I'd also like to get @stumpylog's work on this incorporated to make sure this is as good as it can get under the current constraints.
That is unlikely at this point. I am way behind my intended schedule on this and I want to make sure the 3.0 makes it into the next Ubuntu LTS. I consider this PR as non-critical, since the 2 fundamental improvements it brings (compile time reduction and better IDE discoverability) are both cosmetic in nature. While the risk of breaking something is not obviously 0. That said, since the main distribution channel for the python package (to my knowledge) is pypi.org, I can easily release this any time later. |
|
I won't have a lot of time to check anything out for a couple weeks. My main idea would be creating the "proper" package with the init.py file and renaming the binding module to be imported to it. Just to avoid cmake file renames |
|
@Doekin sorry for breaking your PR yet again... Would you be willing to have a look at what @stumpylog was suggesting and already implemented in his fork? |
|
I’ve checked @stumpylog's code and it looks solid. My only hesitation is that the folder naming might be a bit too "clever" for its own good. I actually managed to trip over my own feet for a while, wondering why my tests weren't reflecting my changes, only to find Python was importing old files from the CWD instead. |
This is mainly based on the work of @Doekin from zxing-cpp#948 with additions from @raymondqin and some cherry picked improvemtns from @stumpylog's zxing-cpp#1109. Co-authored-by: Doekin <Leetimemp@gmail.com> Co-authored-by: Trenton H <797416+stumpylog@users.noreply.github.com>
This is mainly based on the work of @Doekin from zxing-cpp#948 with additions from @raymondqin and some cherry picked improvemtns from @stumpylog's zxing-cpp#1109. Co-authored-by: Doekin <Leetimemp@gmail.com> Co-authored-by: Trenton H <797416+stumpylog@users.noreply.github.com>
This is mainly based on the work of @Doekin from zxing-cpp#948 with additions from @raymondqin and some cherry picked improvemtns from @stumpylog's zxing-cpp#1109. Co-authored-by: Doekin <Leetimemp@gmail.com> Co-authored-by: Trenton H <797416+stumpylog@users.noreply.github.com>
* replace pybind11 with nanobind * automatic pyi generation * replace setuptools with scikit_build_core * enable stable ABI builds for python 12+ (only one wheel for those) * drop win32 wheels * move cibuildwheel config to pyproject.toml This is mainly based on the work of @Doekin from zxing-cpp#948 with additions from @raymondqin and some cherry picked improvemtns from @stumpylog's zxing-cpp#1109. Co-authored-by: Doekin <Leetimemp@gmail.com> Co-authored-by: Trenton H <797416+stumpylog@users.noreply.github.com>
* replace pybind11 with nanobind * automatic pyi generation * replace setuptools with scikit_build_core * enable stable ABI builds for python 12+ (only one wheel for those) * drop win32 wheels * drop Python 3.9 wheels * move cibuildwheel config to pyproject.toml This is mainly based on the work of @Doekin from #948 with additions from @raymondqin and some cherry picked improvemtns from @stumpylog's #1109. Co-authored-by: Doekin <Leetimemp@gmail.com> Co-authored-by: Trenton H <797416+stumpylog@users.noreply.github.com>
|
After introducing more conflicts again and with the alternative approach in #1109 to look at as well, I spend the last few days looking closely at the different proposals, the nanobind and the scikit_build_core doc and finally merged something that is based on this PR, takes a few niceties from @stumpylog and simplifies/cleans up the scikit_build_core use and fixes the remaining issues discussed over the last year and a few new ones I discovered while going over it again line by line. I'm really sorry that it took me so long to properly have a go at this. Thanks @Doekin for working on this. Anyone interested: please pull the resulting wheels for your platform from here: https://github.com/axxel/zxing-cpp/actions/runs/25968882753#artifacts and let me know if you find any issues with it. |
|
Pulled the wheels and gave them a spin — looks fine here. Good to see this land. |
This PR migrates the Python bindings for
zxingcppfrompybind11tonanobind.Key Benefits:
Enhanced IDE Support (Autocompletion & Type Hinting):
The primary driver for this change is
nanobind's straightforward support for generating stub files (.pyi). These stubs enable autocompletion and type hinting in IDEs, significantly improving the development experience when using thezxingcpplibrary.Smaller Binary Sizes
Faster Compilation Times
Important Structural Change:
nanobind's typical project structure, thezxingcppPython extension has been refactored from a single-file module (e.g.,zxingcpp.pydorzxingcpp.so) into a Python package (a directory namedzxingcppcontaining an__init__.pyand the compiled extension module).