Add Python bindings for libvillas #884
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 7 commits
April 4, 2025 08:52
Places the wrapper .so file into the Python lib-dynload folder. This should always be in the Path when using Python. The Python Wrapper only builds if necessary build dependencies are found. Signed-off-by: Kevin Vu te Laar <[email protected]>
Added python and pybind11 dependencies to Dockerfiles, except for Fedora minimal, to build the Python-wrapper. Signed-off-by: Kevin Vu te Laar <[email protected]>
Added Python-Wrapper bindings. Docstrings are still missing. Signed-off-by: Kevin Vu te Laar <[email protected]>
The tests are not yet integrated into the pipeline and are primarily focused on the signal_v2 node. Not all functions work as expected. node_name_short() appears to be print a freed string. node_stop() and node_restart() do not function properly. However, they both use the same underlying function to stop a node, which may be the cause of their failure. Signed-off-by: Kevin Vu te Laar <[email protected]>
Fixed logic error. Threw an error if json_t *json is a json object, but it should throw an error if that *json weren't a valid json_object. Signed-off-by: Kevin Vu te Laar <[email protected]>
Integrated the Python-Wrapper test "/test/unit/python_wrapper.py" into CMake and the CI. Testing within the Fedora dev container resulted in the issue of missing permissions. Manually running: chmod +x /villas/test/unit/python_wrapper.py helped. Not sure if this issue requires changes for the CI. The build integration with CMake had a little issue. Due to the OPTIONAL flag, pybind11 was never set to found and was therefore never built even though it may have been found by CMake. Changed the target for the python-wrapper build from villas_node to python-wrapper and instead changed its OUTPUT_NAME PROPERTY to villas_node, so that the Python module can still be imported with: "import villas_node". This also avoids confusion with CI and CMake integration. Signed-off-by: Kevin Vu te Laar <[email protected]>
Signed-off-by: Kevin Vu te Laar <[email protected]>
The target is `run-python-wrapper-tests` instead of `python-wrapper-tests` Signed-off-by: Kevin Vu te Laar <[email protected]>
cppcheck threw a warning during the pipeline. Since copy and assignment are not used and should not be used, this is fine and won't result in any difference. Calling .copy() in python would have also thrown an error earlier, as a copyconstructor wasn't explicitly exposed to pybind via bindings. Signed-off-by: Kevin Vu te Laar <[email protected]>
Use find_package(Python3 REQUIRED) to set Python3_EXECUTABLE. Moved the `REQUIRED` to align with CMake style guidelines. Signed-off-by: Kevin Vu te Laar <[email protected]>
stv0g
reviewed
Apr 8, 2025
| @@ -35,6 +35,7 @@ add_custom_target(run-unit-tests | |||
| USES_TERMINAL | |||
| ) | |||
|
|
|||
| find_package(Python3 REQUIRED COMPONENTS Interpreter) | |||
There was a problem hiding this comment.
Are you sure this is needed? I dont see the variables/targets defined by find_package being used in this file.
Normally, such transitive dependencies should be defined using the PUBLIC keyword in the target_link_library, so they propagate downstream.
stv0g
reviewed
Apr 8, 2025
Comment on lines
172
to
+173
| - cmake --build build ${CMAKE_BUILD_OPTS} --target run-unit-tests run-unit-tests-common | ||
| - cmake --build build ${CMAKE_BUILD_OPTS} --target run-python-wrapper-tests |
There was a problem hiding this comment.
Suggested change
| - cmake --build build ${CMAKE_BUILD_OPTS} --target run-unit-tests run-unit-tests-common | |
| - cmake --build build ${CMAKE_BUILD_OPTS} --target run-python-wrapper-tests | |
| - cmake --build build ${CMAKE_BUILD_OPTS} --target run-unit-tests run-unit-tests-common run-python-wrapper-tests |
stv0g
reviewed
Apr 8, 2025
| @@ -5,4 +5,5 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| add_subdirectory(opal-rt/rtlab-asyncip) | |||
| add_subdirectory(python-wrapper) | |||
There was a problem hiding this comment.
Usually such language support interfaces are called "bindings". Hence also the name "pybind".
I propose to rename this to "python-binding". Its not really wrapping anything in a sense that it builds on top or extends VILLASnode's functionality.
stv0g
reviewed
Apr 8, 2025
| @@ -0,0 +1,246 @@ | |||
| /* Python-wrapper. | |||
There was a problem hiding this comment.
Suggested change
| /* Python-wrapper. | |
| /* Python bindings. |
stv0g
reviewed
Apr 8, 2025
Comment on lines
+8
to
+12
| #include <jansson.h> | ||
| #include <pybind11/pybind11.h> | ||
| #include <uuid/uuid.h> | ||
| #include <villas/node.hpp> | ||
| #include <villas/sample.hpp> |
There was a problem hiding this comment.
Please separate headers in the following three sections:
- System headers
- Third-party library headers
- VILLASnode headers
Suggested change
| #include <jansson.h> | |
| #include <pybind11/pybind11.h> | |
| #include <uuid/uuid.h> | |
| #include <villas/node.hpp> | |
| #include <villas/sample.hpp> | |
| #include <jansson.h> | |
| #include <pybind11/pybind11.h> | |
| #include <uuid/uuid.h> | |
| #include <villas/node.hpp> | |
| #include <villas/sample.hpp> |
stv0g
reviewed
Apr 8, 2025
| @@ -51,7 +51,10 @@ RUN apt-get update && \ | |||
| liblua5.3-dev \ | |||
| libhiredis-dev \ | |||
| libnice-dev \ | |||
| libmodbus-dev | |||
| libmodbus-dev \ | |||
| python3 \ | |||
There was a problem hiding this comment.
Depending on python3 here is not required as its a direct dependency of python3-dev.
See: https://debian.pkgs.org/12/debian-main-arm64/python3-dev_3.11.2-1+b1_arm64.deb.html
E.g. we are also not depending on libmodbus, and only on libmodbus-dev.
stv0g
reviewed
Apr 8, 2025
| @@ -58,7 +58,10 @@ RUN apt-get update && \ | |||
| libusb-1.0-0-dev:${ARCH} \ | |||
| liblua5.3-dev:${ARCH} \ | |||
| libhiredis-dev:${ARCH} \ | |||
| libmodbus-dev:${ARCH} | |||
| libmodbus-dev:${ARCH} \ | |||
| python3:${ARCH} \ | |||
stv0g
reviewed
Apr 8, 2025
| @@ -59,7 +59,10 @@ RUN apt-get update && \ | |||
| libmodbus-dev \ | |||
| libre2-dev \ | |||
| libglib2.0-dev \ | |||
| libcriterion-dev | |||
| libcriterion-dev \ | |||
| python3 \ | |||
stv0g
reviewed
Apr 8, 2025
| # SPDX-FileCopyrightText: 2014-2025 Institute for Automation of Complex Power Systems, RWTH Aachen University | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| cmake_minimum_required(VERSION 3.15...3.29) |
There was a problem hiding this comment.
Could we move this to the top-level CMakeLists.txt? Or is there a reasoning for having this also here?
Building this as a dedicated project wont work due to the missing villas library target which you use below..
|
Hi @al3xa23 do you or your student need support in getting this merged? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Python Wrapper for VILLASnode
Documentation available VILLASframework/documentation#104 (comment)
Please provide comments what needs to be changed from you point of view :)