Support nanobind #20

davidlatwe · 2024-10-13T09:38:22Z

Thank you for this awesome project. This pull request enabled us to generate ImGui binding with nanobind.

In latest 2 commits, we were even able to generate binding from original ImGui source (non-docking) without the need to patch IMGUI_BUNDLE_PYTHON_UNSUPPORTED_API:

At this moment, I have only tested with these two ImGui source:

@pthom 's ImGui fork, the one resides in pthom/imgui_bundle
Original ImGui v1.91.3 (not docking branch)

Did not have all API binding tested, but works in our project so far.

🚨This commit 656a99c bumped version to 0.999.0, we need a proper version for this before merge.

davidlatwe · 2024-10-15T05:44:31Z

Just realized that I have not yet added tests for this. Will see if I can make that happen ASAP.

unit tests (code generation)
integration tests (bindings tests)

pthom · 2024-10-15T09:26:46Z

Hello David,

Thanks a lot for this PR!
You are right that adding tests would be a big plus, especially if we want to merge this to the main branch.

Here are some quick advises about them:

unit tests: there are a lot of them, and most of them will compare the generated code (C++ or stubs) with an expected code. It is not needed to adapt all of the tests to nanobind (however adapting a small selected range of them would be nice). The most important thing would be to add a test dedicated to comparing the differences between nanobind and pybind in a related test file.
integration tests: Those are more important. Here, we do not compare the generated code, but instead we test the behavior of bound C++ code, when called from Python. We could run exactly the same list of integration tests with nanobind and pybind11.

Since integration tests are more important, let me give you some details on how they are implemented:

the Readme for integration_tests It gives a good overall view of the structure

We could patch autogenerate_mylib.py, so that it handles nanobind and pybind11 together.

litgen/src/litgen/integration_tests/autogenerate_mylib.py

Lines 153 to 165 in 5d4eea7

    
           def save_all_generated_codes_by_file() -> None: 
        
               """This is specific to litgen's integration tests. 
        
               It will generate all the files xxx_test.h.pydef.cpp and xxx_test.h.pyi 
        
               :return: 
        
               """ 
        
               options = mylib_litgen_options() 
        
               headers_dir = THIS_DIR + "/mylib/" 
        
               def process_one_file(header_file: str) -> None: 
        
                   # print(header_file) 
        
                   input_cpp_header_file = headers_dir + header_file 
        
                   output_cpp_pydef_file = input_cpp_header_file + ".pydef.cpp" 
        
                   output_stub_pyi_file = input_cpp_header_file + ".pyi"

. For example, it could produce a .pydef.cpp file and a .nano.cpp file, etc.

And the tests could run in succession with nanobind and pybind11

Thanks!

pthom · 2024-10-22T14:13:06Z

Hello,

I added your branch in the repository into a new nanobind branch . I performed a rebase on the main branch before doing so.

The integration went smoothly. I only had to do a few changes, in three commits I added on top of yours:

Commit b5248f4: fix adapt_c_buffers

I had to do a small fix, following the rebase, because I had added a "contiguous check".
Note: you may want to add such a test for nanobind here:

litgen/src/litgen/internal/adapt_function_params/_adapt_c_buffers.py

Lines 402 to 419 in a3c6671

    
                   if self.options.bind_library == BindLibraryType.pybind11: 
        
                       template = f""" 
        
                                   // Check if the array is C-contiguous 
        
                                   if (!{param_name}.attr("flags").attr("c_contiguous").cast<bool>()) {{ 
        
                                       throw std::runtime_error("The array must be contiguous, i.e, `a.flags.c_contiguous` must be True. Hint: use `numpy.ascontiguousarray`."); 
        
                                   }} 
        
                                   // convert py::array to C standard buffer ({mutable_or_const}) 
        
                                   {_._const_space_or_empty(idx_param)}void * {_._buffer_from_pyarray_name(idx_param)} = {_._param_name(idx_param)}.{mutable_or_empty}data(); 
        
                                   py::ssize_t {_._pyarray_count(idx_param)} = {_._param_name(idx_param)}.shape()[0]; 
        
                               """  # noqa 
        
                   else: 
        
                       # TODO: implement contiguous check for nanobind 
        
                       template = f""" 
        
                                   // convert py::array to C standard buffer ({mutable_or_const}) 
        
                                   {_._const_space_or_empty(idx_param)}void * {_._buffer_from_pyarray_name(idx_param)} = {_._param_name(idx_param)}.{mutable_or_empty}data(); 
        
                                   size_t {_._pyarray_count(idx_param)} = {_._param_name(idx_param)}.shape(0); 
        
                               """  # noqa

Commit cbc37fe:
Some very small changes to the unit tests. All unit tests now pass (but they are only testing pybind11 bindings)
Commit a3c6671 : fixed a minor mypy warning

Status as of now:

nanobind support is available in a dedicated branch in this repo
all unit tests with pybind11 work.
It would be necessary to add a unit test dedicated to the changes between pybind11 and nanobind
It would be necessary to add integration tests for nanobind

As you know more than me about nanobind, could you have a look at this? Do not hesitate to ask for help, I'll be happy to answer questions and to provide help.

pthom · 2024-10-22T16:48:40Z

Hum, setting up the integration test for two different binding types is a bit tricky, especially in the build scripts. Give me some time to have a look at it.

davidlatwe · 2024-10-23T04:06:28Z

Thanks! I should be able to join you on this weekend. 👍🏼

pthom · 2024-10-24T14:50:54Z

Hello,

I added some more commits and reorganized the integration tests to account for nanobind and pybind11 working together in the integration_tests folder of the nanobind branch.

I also added some information in the integration test Readme.

Please read it, as it should help you understand the new folder structure and how to run the tests.
I advise you to install just (see just.systems), in order to use the build scripts I provided with it:

 just -l
Available recipes:
    build_integration_tests_nanobind # Builds the integration tests for nanobind
    build_integration_tests_pybind   # Builds the integration tests for pybind
    default                          # List all available commands
    integration_tests                # Runs all tests for pybind and nanobind (after building the integration tests)
    integration_tests_nanobind       # Runs all tests for nanobind, after building the integration tests
    integration_tests_pybind         # Runs all tests for pybind, after building the integration tests
    mypy                             # Runs mypy on the top level folder (see mypy.ini)
    pytest                           # Just runs pytest (requires that the integration tests have been built)

Here is the current status

I took some time to correct the compilation of all tests when using nanobind. This was a good occasion for me to look at what modifications you did (and I was pleased).

What works now:

All tests now compile completely with nanobind and pybind11. You can switch between both with export LITGEN_USE_NANOBIND=ON/OFF before running pip install -v -e . (or use the provided just scripts)
trampoline classes & override now compile correctly: I had to do a few adaptations, see 3219a24
adapt_c_buffers: I did some corrections, see c9a4a7e
with nanobind, custom python constructors shall use "placement new": I did some related corrections in 618c945 and 789b08c
templated buffers: I "lazily corrected" the compilation by disabling this feature at the moment with nanobind. This should be restored in a later step (see 88d2cea)

Status of the integration tests

I made modifications in order to ensure that the tests do compile. I did not check and correct the python tests at the moment (i.e the tests that will check that the bindings do work as expected).

Here is a quick recap of the failing tests as of now:

> just integration_tests_nanobind
...
...
...
======================================================================================================== short test summary info =========================================================================================================
FAILED src/litgen/integration_tests/mylib/c_style_array_test.py::test_const_array2_add - TypeError: const_array2_add(): incompatible function arguments. The following argument types are supported:
FAILED src/litgen/integration_tests/mylib/c_style_buffer_to_pyarray_test.py::test_add_inside_buffer - Failed: DID NOT RAISE <class 'RuntimeError'>
FAILED src/litgen/integration_tests/mylib/c_style_buffer_to_pyarray_test.py::test_templated_mul_inside_buffer - AttributeError: module 'lg_mylib' has no attribute 'templated_mul_inside_buffer'
FAILED src/litgen/integration_tests/mylib/class_adapt_test.py::test_adapt_constructor - TypeError: __init__(): incompatible function arguments. The following argument types are supported:
FAILED src/litgen/integration_tests/mylib/class_adapt_test.py::test_modify_array_member_via_numpy_array - TypeError: __init__(): incompatible function arguments. The following argument types are supported:
FAILED src/litgen/integration_tests/mylib/mix_adapters_class_test.py::test_template_method - AttributeError: 'lg_mylib._lg_mylib.some_namespace.Blah' object has no attribute 'templated_mul_inside_buffer'
FAILED src/litgen/integration_tests/mylib/modifiable_immutable_test.py::test_adapt_modifiable_immutable_to_return - TypeError: Unable to convert function return value to a Python type! The signature was
FAILED src/litgen/integration_tests/mylib/return_value_policy_test.py::test_return_value_policy - AssertionError: assert 0 == 3
FAILED src/litgen/integration_tests/mylib/template_class_test.py::test_template_class_int - TypeError: __init__(): incompatible function arguments. The following argument types are supported:
FAILED src/litgen/integration_tests/mylib/template_class_test.py::test_template_class_string - TypeError: __init__(): incompatible function arguments. The following argument types are supported:
FAILED src/litgen/integration_tests/mylib/template_function_test.py::test_sum_vector_and_c_array - TypeError: sum_vector_and_c_array_int(): incompatible function arguments. The following argument types are supported:
FAILED src/litgen/integration_tests/mylib/template_function_test.py::test_sum_vector_and_c_array_method - TypeError: sum_vector_and_c_array_int(): incompatible function arguments. The following argument types are supported:
==================================================================================================== 12 failed, 224 passed in 10.76s =====================================================================================================
call_guard_tester
error: Recipe `integration_tests_nanobind` failed on line 25 with exit code 1

12 tests out of 224 fail. Seems reasonable.

The road ahead

Let me summarize here what remains to be done.

correct integration tests
restore templated buffers: I "lazily corrected" the compilation by disabling this feature at the moment with nanobind. This should be restored in a later step (see 88d2cea)
add info about the bindings in the doc.
add pybind11 & nanobind pydef in the documentation site
inform nanobind team, and create a PR so that their doc site (https://nanobind.readthedocs.io/en/latest/) mentions the existence of litgen (which is already mentioned in the pybind11 site)

Please keep me informed on your progress, and do not hesitate to ask for help, or to inform me if you hit a road block or if you feel you do not have enough time.

Many thanks for your help!

davidlatwe · 2024-10-27T14:51:27Z

Hey @pthom 👋🏼

I pulled your nanobind branch and had a spin, the integration test is working here (nanobind tests are not 100% passed yet).

Here are my feedbacks/progress. 😃

Use `venv`

I think we should mention Python venv setup in integration_tests /Readme.md before calling just recipe.

cd src/litgen/integration_tests
python -m venv ./venv
./venv/Scripts/Activate.ps1
pip install -r requirements-dev.txt

Add a few more dependencies in `requirements-dev.txt`

After I setup my venv under integration_tests, the dependency installed from original requirement file was not enough for me to run tests.

It installed litgen from git main branch, which does not have latest changes we want to test with.
We need pytest and numpy for testing.

So I modified it like so:

# Install litgen in edit mode (you need to be in the same directory as this requirements-dev.txt)
-e ../../..

# Testing
black
mypy
pytest
numpy

A few changes for making `justfile` to support Powershell

Since I am on Windows and mostly using Powershell, I did a few tweaks for me to run on Powershell. 😄

set windows-shell := ["powershell.exe", "-c"]

[unix]
_pip_install use_nanobind:
    export LITGEN_USE_NANOBIND="{{use_nanobind}}" && cd src/litgen/integration_tests && pip install -v -e . && cd -

[windows]
_pip_install use_nanobind:
    $env:LITGEN_USE_NANOBIND="{{use_nanobind}}"; pushd $pwd; cd src/litgen/integration_tests; pip install -v -e .; popd

# Builds the integration tests for pybind
build_integration_tests_pybind:
    python src/litgen/integration_tests/autogenerate_mylib.py no_generate_file_by_file pybind11
    just _pip_install OFF
    python -c "import lg_mylib"

# Builds the integration tests for nanobind
build_integration_tests_nanobind:
    python src/litgen/integration_tests/autogenerate_mylib.py no_generate_file_by_file nanobind
    just _pip_install ON
    python -c "import lg_mylib"

Tests still run when build failed

The step I took:

Run just integration_tests_pybind
lg_mylib generated and installed
pybind11 tests passed 100%
Run just integration_tests_nanobind
Build failed but tests still passed 100%

Clearly the lg_mylib being used in step 5 was the one installed from pybind11 test, which can be confusing. We should ensure tests always run on demanded binding. 🤔

I am thinking if we can name each binding differently, like lg_mylib_pybind and lg_mylib_nanobind. And in test module, we import them like this:

if test_pybind:
    import lg_mylib_pybind as lg_mylib
else:
    import lg_mylib_nanobind as lg_mylib

Does this make sense?

Renamed `pybind_mylib.cpp`

I renamed _pydef_nanobind/pybind_mylib.cpp to _pydef_nanobind/nanobind_mylib.cpp.

Which branch should I continue?

Should I merge your nanobind branch into this one to continue?

Or... I have never used this "Allow edits by maintainers" feature before, but it sounds like you can push your commits into this branch too?

Let me know what you prefer!

Thanks for your help on this! 🚀

pthom · 2024-10-27T15:03:30Z

Hello @davidlatwe

I do not have much time today, but to answer your question about which branch to use, you wrote:

Or... I have never used this "Allow edits by maintainers" feature before, but it sounds like you can push your commits into this branch too?

I never used this too. However, it might work ;-) Could you integrate the changes from my nanobind branch to your nanobind branch and we will continue working on yours? Please note that I had done a rebase of your changes initially.

I am thinking if we can name each binding differently, like lg_mylib_pybind and lg_mylib_nanobind.

Sound like a good idea!

With original C++ enum name preserved. E.g. `imgui.ImGuiWindowFlags_MenuBar` instead of `imgui.WindowFlags_.MenuBar`

This reverts commit e247b5f.

This allows excluding specific function. Use case: IMGUI_BUNDLE_PYTHON_UNSUPPORTED_API definition was added in ImGui source to opt-out un-bindable API. With this new option, we can achieve the same goal without having to alter source code.

This allows excluding specific class/struct method and member. Use case: IMGUI_BUNDLE_PYTHON_UNSUPPORTED_API definition was added in ImGui source to opt-out un-bindable API. With this new option, we can achieve the same goal without having to alter source code. E.g. ImVector.

…iguous check for nanobind

…yet)

…ember_exclude_by_name_and_class__regex

davidlatwe · 2024-11-03T23:59:44Z

Hi @pthom

I have integrated your commit into this branch. It was done by these steps:

Rebase this branch onto main branch, with conflict resolved
Cherry pick your commits

Before doing afore mentioned integration, I tried force-push your branch into this PR directly. But the git diff become un-readable so I did another force-push to correct it.

About supporting Powershell for just on Windows, I had a second thought. Since everything works well if using Git-Bash, so I figure it is best to keep it as is.

Will continue the nanobind tests as soon as I can.

… type)

pthom · 2024-11-06T08:33:18Z

Hi David,

I had a look at your changes and they look good. I pushed some small modifications:

Added a bit of documentation explaining how to switch between nanobind and pybind11 in the integration tests
Some minor corrections and refactoring in lg_mylib/use.py and __init__.py
Reformatted code (using black)
GitHub CI: use just

Good job, Cheers!

# The type checking must be done in two steps (at least) # - check the generic type (int, float, uint, etc) # - check the size of the type (int8, int16, int32, int64, etc) # Knowing that for a given ndarray: # ndarray::dtype() returns a # struct dtype { # uint8_t code = 0; # uint8_t bits = 0; # uint16_t lanes = 0; # }; # and that the code is defined as: # enum class dtype_code : uint8_t { Int = 0, UInt = 1, Float = 2, Bfloat = 4, Complex = 5, Bool = 6 };

Added a lambda _nanobind_buffer_type_to_letter_code that will compute the letter code for the buffer type

… pybind or nanobind

pthom · 2024-11-06T22:54:04Z

Hello again,

I had another look at the failing tests, and as I was suspecting, the code to adapt c buffers is quite complex (and a bit fragile). Making it compatible for pybind together with nanobind is not an easy at all, especially because the ndarray API was completely rewritten for nanobind.

I am working on some corrections on this part, I will keep you informed.

…ypes)

pthom · 2024-11-07T16:38:09Z

Hi David,

Big thanks for all the great work on this PR!
Your efforts to add nanobind support and handle the differences with pydef syntax have been incredibly useful, and I’m thrilled with how it turned out! I’ve been testing the new bindings, and they’re working great!

Since the last steps were a bit complex (and needed testing on multiple platforms), I went ahead and wrapped up the remaining tasks to get this PR fully completed.

I hope you’re not discouraged by this — I mainly stepped in because I’m juggling quite a few projects (imgui-bundle, litgen, hello-imgui, cvnp, etc.) and wanted to get this feature integrated into imgui-bundle for some upcoming experiments.

I will merge this PR now, and you will see that the documentation site, now shows bindings for both pybind11 and nanobind.

I’m also planning to propose an update to the nanobind documentation site to mention litgen as a compatible generator.

Your work here has really made an impact!

Please do not hesitate to add comments in this PR: if you feel more changes are needed, we may pursue it, or open a new PR.

Also, I hope to be able to exchange with you concerning your other open PR on imgui_bundle nanobind support!

Thanks again! 😊

PS: you will see that I added a thank you note in the README

Best,
Pascal

pthom · 2024-11-07T17:17:28Z

See wjakob/nanobind#780 where I propose an update to the nanobind doc

davidlatwe · 2024-11-11T19:03:45Z

Hey @pthom , big thanks for all the kind words and your recognition! ❤️ ❤️

I hope you’re not discouraged by this

Not at all! My progress was a bit slow due to my limited resources these days, so I am really glad that you were able to push it forward.

And yeah, I will move on to the other PR on imgui_bundle. I expect I can jump in this weekend.

Cheers! 🍻

This was referenced Oct 13, 2024

Support nanobind pthom/imgui_bundle#266

Closed

How to support nanobind? #3

Open

davidlatwe force-pushed the nanobind branch 2 times, most recently from 697c60b to 78c7ff5 Compare November 3, 2024 19:41

davidlatwe added 11 commits November 4, 2024 03:42

Workable nanobind generating code

89ced69

Bump litgen version for local dev

896a357

Enable exporting enum entries to parent scope

104c64a

With original C++ enum name preserved. E.g. `imgui.ImGuiWindowFlags_MenuBar` instead of `imgui.WindowFlags_.MenuBar`

Keep enum binding in a variable and extends it in another .inl

f466b1e

Fix binding function with const char* = NULL argument

9758325

Revert "Workable nanobind generating code"

de6fc6a

This reverts commit e247b5f.

Add switch to disable nanobind specific patch

cced4c2

Make binding library an option

4b2721c

Fix pybind11 build

d2bb3c6

davidlatwe force-pushed the nanobind branch from 78c7ff5 to 5085245 Compare November 3, 2024 19:43

pthom added 6 commits November 4, 2024 03:46

adapt_c_buffers: fix following rebase nanobind / TODO: implement cont…

e0491e8

…iguous check for nanobind

fix unit tests after nanobind adaptations (no nanobind specific test …

64f4b52

…yet)

options: fix mypy warning / init fn_exclude_by_name_and_signature & m…

0e2dc66

…ember_exclude_by_name_and_class__regex

Work on adding integration tests for nanobind

75539e1

integ test: lib does not depend on pybind

63d35be

integration tests: generate also for nanobind

2fac4fa

davidlatwe added 2 commits November 4, 2024 07:02

Be specific on which binding type to test

853acd6

Don't import binding before configured

ef0df84

pthom added 3 commits November 6, 2024 09:10

integration tests: added doc / switch between nanobind and pybind

c3628d1

renamed nanobind_mylib.cpp

0e940fd

integration tests: refactor __init__.py (better handling / switch lib…

37fc948

… type)

pthom added 8 commits November 6, 2024 09:44

Github CI: use just

1713d8c

Reformat code with black

5a008d5

CI: use just everywhere

32922d4

harden validate_bindings_compilation

2119bd2

adapt_c_buffers: fix template buffers

870dacb

Added a lambda _nanobind_buffer_type_to_letter_code that will compute the letter code for the buffer type

return value policy: find_return_policy_in_comments works with either…

5a80d34

… pybind or nanobind

nanobind: fix adapted ctor stub signature (remoe redundant self)

5379b53

pthom added 10 commits November 7, 2024 09:41

nanobind: use nb:: instead of py:: in pydef code

b6553ae

nanobind: update all docs -> now show nanobind + pybinb c++ code

041afb8

integration_test: litgen_cmake => remove bindings_folder

965c734

update litgen_template

2043685

update doc

c75db40

nanobind: fix adapt_c_buffer on windows (we can use precisely sized t…

27f06bf

…ypes)

adapt_c_buffers: Check if the array is 1D and C-contiguous

ba8ff59

validate_bindings_compilation: add param enable_hack_code

9853f71

update doc

106f32a

Readme: acknowledge @davidlatwe for nanobind support

a795aac

pthom merged commit 1f2c071 into pthom:main Nov 7, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support nanobind #20

Support nanobind #20

davidlatwe commented Oct 13, 2024

davidlatwe commented Oct 15, 2024 •

edited by pthom

Loading

pthom commented Oct 15, 2024

pthom commented Oct 22, 2024

pthom commented Oct 22, 2024

davidlatwe commented Oct 23, 2024

pthom commented Oct 24, 2024

davidlatwe commented Oct 27, 2024

pthom commented Oct 27, 2024

davidlatwe commented Nov 3, 2024

pthom commented Nov 6, 2024 •

edited

Loading

pthom commented Nov 6, 2024

pthom commented Nov 7, 2024

pthom commented Nov 7, 2024

davidlatwe commented Nov 11, 2024

Support nanobind #20

Support nanobind #20

Conversation

davidlatwe commented Oct 13, 2024

davidlatwe commented Oct 15, 2024 • edited by pthom Loading

pthom commented Oct 15, 2024

pthom commented Oct 22, 2024

pthom commented Oct 22, 2024

davidlatwe commented Oct 23, 2024

pthom commented Oct 24, 2024

davidlatwe commented Oct 27, 2024

Use venv

Add a few more dependencies in requirements-dev.txt

A few changes for making justfile to support Powershell

Tests still run when build failed

Renamed pybind_mylib.cpp

Which branch should I continue?

pthom commented Oct 27, 2024

davidlatwe commented Nov 3, 2024

pthom commented Nov 6, 2024 • edited Loading

pthom commented Nov 6, 2024

pthom commented Nov 7, 2024

pthom commented Nov 7, 2024

davidlatwe commented Nov 11, 2024

davidlatwe commented Oct 15, 2024 •

edited by pthom

Loading

Use `venv`

Add a few more dependencies in `requirements-dev.txt`

A few changes for making `justfile` to support Powershell

Renamed `pybind_mylib.cpp`

pthom commented Nov 6, 2024 •

edited

Loading