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

Add python bindings #6

Merged
merged 12 commits into from
Dec 6, 2024
Merged

Add python bindings #6

merged 12 commits into from
Dec 6, 2024

Conversation

ll-nick
Copy link
Collaborator

@ll-nick ll-nick commented Oct 18, 2024

This contributes python bindings. Since this is a template C++ library, the python bindings have to be compiled at a later point in time, when the types are known. Therefore, we don't ship the bindings directly but rather add a convenience function that allows binding the types once the template arguments are known.

When pybind is available and BUILD_TEST=TRUE, a python module is built that allows running unit tests (almost) equivalent to the C++ ones.

#minor

@ll-nick ll-nick requested a review from orzechow October 18, 2024 14:59
@ll-nick ll-nick self-assigned this Oct 18, 2024
@ll-nick ll-nick force-pushed the add_python_bindings branch from 826bb80 to bd1b2a3 Compare October 18, 2024 15:09
@ll-nick
Copy link
Collaborator Author

ll-nick commented Oct 21, 2024

One idea I had one the weekend which might be a nice solution for allowing to add more comparison policies then the already implemented ones would be to add a template parameter pack and running

        .def(
            "cached",
            [](CacheT& self, const NumberT& key, const ApproximateNumberT& policy) {
                return self.template cached<ApproximateNumberT>(key, policy);
            },

for each passed policy. I'll set this PR back to draft and test that out first as it would solve the issue with the missing tests (I think).

@ll-nick ll-nick marked this pull request as draft October 21, 2024 06:02
@ll-nick ll-nick marked this pull request as ready for review October 21, 2024 09:05
@ll-nick
Copy link
Collaborator Author

ll-nick commented Oct 21, 2024

Ok, that actually worked out quite well.

I rearranged the bindings a little and added the missing unit tests. It is now possible to add a comparison policy as a template parameter to the bindCache function which will the overload the cached function for it. Check the comments in the code for a detailed explanation, I tried documenting all of it :)

@ll-nick ll-nick force-pushed the add_python_bindings branch from 3106fac to fd4a2fe Compare October 21, 2024 13:14
@ll-nick
Copy link
Collaborator Author

ll-nick commented Oct 21, 2024

Rebased on current master.

@ll-nick ll-nick force-pushed the add_python_bindings branch from fd4a2fe to 2d872aa Compare October 21, 2024 13:16
@ll-nick ll-nick force-pushed the add_python_bindings branch from 2d872aa to fbeec5a Compare November 11, 2024 11:03
@ll-nick
Copy link
Collaborator Author

ll-nick commented Nov 11, 2024

Rebased to main and adjusted to updated cmake structure.

@ll-nick ll-nick force-pushed the add_python_bindings branch from fbeec5a to 3b5261d Compare November 14, 2024 13:50
@ll-nick
Copy link
Collaborator Author

ll-nick commented Nov 14, 2024

Rebased to main

Copy link
Member

@orzechow orzechow left a comment

Choose a reason for hiding this comment

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

Alright, thanks a lot for this contribution!

I guess, the advantage and usability for the Python end user is nice (much like the C++ interface).
But – one of the reasons this review took so long – getting there is honestly not that great of an experience 😅 – especially, if you're not (yet?) familiar with pybind etc.

Anyways, I guess it's still helpful, especially as you mainly mean it as a base to add Python support for the arbitration_graphs in order to integrate these in an Python-based evaluation framework. 🤞

And you did a great job in enriching these parts with good documentation and useful unit tests 👌

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
configure_file(${_py_test} ${PY_TEST_NAME} COPYONLY)

add_test(NAME ${PY_TEST_NAME}
COMMAND ${Python3_EXECUTABLE} ${PY_TEST_NAME}
Copy link
Member

Choose a reason for hiding this comment

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

CMake for Python… 🫣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know 😅

But I figured since you need to compile the bindings anyway, this is the easiest way to run the tests (they need to be run inside the build directory, because the compiled bindings are not installed or something like that).

test/python_bindings.cpp Outdated Show resolved Hide resolved
test/util_caching.py Show resolved Hide resolved
@ll-nick
Copy link
Collaborator Author

ll-nick commented Dec 6, 2024

Thanks for the review and the feedback! I know that the pybind stuff is a little hard to read at times (even though it's much better than the equivalent Boost.Python implementation 😂). However, since most of that stuff should never need to be seen or touched by a user of the library, it shouldn't matter too much.

I'll rewrite the PR description quickly (since it's now used for the release notes), then merge this with a minor version bump.

@ll-nick ll-nick merged commit db9e97c into main Dec 6, 2024
1 check passed
@ll-nick ll-nick deleted the add_python_bindings branch December 6, 2024 10:51
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