Add Python bindings for libvillas #884

al3xa23 · 2025-04-04T06:55:39Z

Python Wrapper for VILLASnode
Documentation available VILLASframework/documentation#104 (comment)
Please provide comments what needs to be changed from you point of view :)

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 <vu.te@rwth-aachen.de>

Added python and pybind11 dependencies to Dockerfiles, except for Fedora minimal, to build the Python-wrapper. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

Added Python-Wrapper bindings. Docstrings are still missing. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

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 <vu.te@rwth-aachen.de>

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 <vu.te@rwth-aachen.de>

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 <vu.te@rwth-aachen.de>

Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

The target is `run-python-wrapper-tests` instead of `python-wrapper-tests` Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

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 <vu.te@rwth-aachen.de>

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 <vu.te@rwth-aachen.de>

stv0g · 2025-04-08T17:01:55Z

tests/unit/CMakeLists.txt

@@ -35,6 +35,7 @@ add_custom_target(run-unit-tests
    USES_TERMINAL
 )

+find_package(Python3 REQUIRED COMPONENTS Interpreter)


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.

It was not necessary.
The idea behind it was to have CMake find the path of the Python interpreter.
I wanted to ensure that the Python executable was properly found, but just calling with python or python3 instead of ${PYTHON_EXECUTABLE} does the trick as well.

I ran into some permission issues during testing within in a local docker container and was worried that this could have caused the error, but it was an error during testing on my part.

stv0g · 2025-04-08T17:02:29Z

.gitlab-ci.yml

    - 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


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 · 2025-04-08T17:04:20Z

clients/CMakeLists.txt

@@ -5,4 +5,5 @@
 # SPDX-License-Identifier: Apache-2.0

 add_subdirectory(opal-rt/rtlab-asyncip)
+add_subdirectory(python-wrapper)


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.

addressed in: 365f88b 6806d24

stv0g · 2025-04-08T17:05:18Z

clients/python-wrapper/villas-python-wrapper.cpp

@@ -0,0 +1,246 @@
+/* Python-wrapper.


Suggested change

/* Python-wrapper.

/* Python bindings.

addressed in: 365f88b

stv0g · 2025-04-08T17:06:25Z

clients/python-wrapper/villas-python-wrapper.cpp

+#include <jansson.h>
+#include <pybind11/pybind11.h>
+#include <uuid/uuid.h>
+#include <villas/node.hpp>
+#include <villas/sample.hpp>


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>

addressed in: 365f88b

stv0g · 2025-04-08T17:08:20Z

packaging/docker/Dockerfile.debian

@@ -51,7 +51,10 @@ RUN apt-get update && \
 		liblua5.3-dev \
 		libhiredis-dev \
 		libnice-dev \
-		libmodbus-dev
+		libmodbus-dev \
+		python3 \


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 · 2025-04-08T17:08:29Z

packaging/docker/Dockerfile.debian-multiarch

@@ -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} \


Some as above.

stv0g · 2025-04-08T17:08:43Z

packaging/docker/Dockerfile.ubuntu

@@ -59,7 +59,10 @@ RUN apt-get update && \
 		libmodbus-dev \
 		libre2-dev \
 		libglib2.0-dev \
-		libcriterion-dev
+		libcriterion-dev \
+		python3 \


Same as above.

stv0g · 2025-04-08T17:11:39Z

clients/python-wrapper/CMakeLists.txt

+# 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)


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..

addressed in: 6806d24

stv0g · 2025-04-24T06:04:26Z

Hi @al3xa23 do you or your student need support in getting this merged?

al3xa23 · 2025-05-07T08:52:50Z

Hi, sorry, we need some more time here. If we need help, we come back to you. Thanks :)

Based on suggestions from PR#884. Removed the `python` dependency because `python-devel` in Debian and Rocky also depend on the base `python` package. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

As suggested `wrapper` was substituted by `binding`. The minimum required version and project naming in `CMakeLists.txt` were removed, as this is not necessary. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

- Removed unnecessary `find_package()` call in `CMakeLists.txt`. - Changed naming to match substitution of `wrapper` with `binding`. - Bumped the version requirement from `3.14` to `3.15` matching the removed minimum in `binding` `CMakeLists.txt`. - Dropped version range syntax `cmake_minimum_required(A...B)`, introduced in `3.19`, in favor of a simple minimum requirement of `3.15`. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

Signed-off-by: al3xa23 <140614263+al3xa23@users.noreply.github.com>

Manually formatted the Python binding test with black. The pipeline formatting resulted in an invalid escape sequence `\m`. Oddly enough there is no such character sequence within the code. Especially not at the reported line 108 when looking at: https://git.rwth-aachen.de/acs/public/villas/node/-/jobs/6248036 . Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

Manually formatted the Python binding test with black-jupyter. The pipeline formatting resulted in an "invalid escape sequence \m" error. Oddly enough there is no such character sequence within the code. Especially not at the reported line 108 when considering: https://git.rwth-aachen.de/acs/public/villas/node/-/jobs/6248036 https://git.rwth-aachen.de/acs/public/villas/node/-/jobs/6249567 `black-jupyter` seems to enforce stricter formatting rules than plain `black`. Formatting with plain `black` caused the pipeline to fail, as `black-jupyter` made additional changes. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

KeVteL · 2025-05-22T12:45:46Z

.gitlab-ci.yml

@@ -173,7 +173,8 @@ test:unit:
  image: ${DOCKER_IMAGE_DEV}:${DOCKER_TAG}
  script:
    - cmake -S . -B build ${CMAKE_OPTS}
-    - cmake --build build ${CMAKE_BUILD_OPTS} --target run-unit-tests run-unit-tests-common
+    - export PYTHONPATH=$PYTHONPATH:${PWD}/build/clients/python-binding
+    - cmake --build build ${CMAKE_BUILD_OPTS} --target run-unit-tests run-unit-tests-common run-python-binding-tests


regarding:
#884 (comment)

I am not quite sure whether or not it is better to export the path of the binding .so as is done here or
to make use of something like this:

paths: -build/clients/python-binding

I am not sure whether or not this would even work.

KeVteL · 2025-05-22T12:55:09Z

regarding: #884 (comment) #884 (comment) #884 (comment)

I also removed the package python from Dockerfile.fedora, as python-devel also depends on it. b1e9a2f

stv0g

Great work :) I have only minor points regarding packaging..

Please let me know if you need some support on this.

stv0g · 2025-05-22T14:12:43Z

packaging/docker/Dockerfile.rocky

@@ -25,7 +25,8 @@ RUN dnf --allowerasing -y install \
 	flex bison \
 	texinfo git git-svn curl tar patchutils \
 	protobuf-compiler protobuf-c-compiler \
-	clang-tools-extra
+	clang-tools-extra \
+	python-devel


Do you mind moving this below to the pybind11-devel package, as for the other Dockerfiles?

Sure, before I do a short question:
I realized during a debian install, that the dependency list of the Dockerfiles debian, debian-multiarch and rocky are missing libwebsockets(-dev), which is required to build VILLASnode.

Should I add them? I dislike creating a lot of small commits and changing history with cherry picking/squishing is somewhat... a pain and just causes troubles.

Hi @KeVteL,

I realized during a debian install, that the dependency list of the Dockerfiles debian, debian-multiarch and rocky are missing libwebsockets(-dev), which is required to build VILLASnode.

This maybe due to the version of libwebsocket shipped in the distributions package repository to be too old. If a package is not already installed (e.g. by the Dockerfile), we will install the missing dependencies (using the correct newer versions) via the deps.sh shell script.

However, I would be surprised if for the particular case of libwebsocket, the distro packages are too old. We should check here.

We also have upgrade some of the base distro versions (e.g. Debian 12 -> 13) in the last months without checking all the other dependencies. It might be that we now have the required versions already provided by the distros.

The exact version requirements are documented here:

https://villas.fein-aachen.org/docs/node/installation#prerequisites

node/CMakeLists.txt

Line 95 in 9b81d28

pkg_check_modules(JANSSON IMPORTED_TARGET REQUIRED jansson>=2.13)

I checked the Docker containers regarding libwebsockets.
As for:

Rocky: libwebsockets-4.3.2-1.el9.x86_64 is installed,

Debian: libwebsockets is not installed.

Running the binding test in the Debian container works.
Since it involves sockets, I am assuming that it is working fine.
Because the Rocky build fails (on this branch and on Master), I unfortunately could not test it.

This is probably better to be moved to an #Issue, but for the time being I'll leave it here:

The error is the same for all of the following include lines:

/usr/include/linux/pkt_cls.h:250:9: error: ‘struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr’ invalid; an anonymous union may only have public non-static data members -fpermissive]

pkt_cls.h appears to be pulled in via:

lib/node.cpp:9 #include <regex>

lib/kernel/if.cpp:12 #include <netlink/route/link.h>

lib/kernel/tc.cpp:10 #include <netlink/route/cls/fw.h>

lib/kernel/tc_netem.cpp:13 #include <netlink/route/qdisc/netem.h>

and possibly more.

It seems unusual to me that the regex include in node.cpp would be related to this issue.

This is probably introduced by the rocky update to 9.3?
I remember having tested the Rocky docker build with the python-binding before the update from version 9.
But it could be that other changes could have caused it.

stv0g · 2025-05-22T14:15:51Z

tests/unit/CMakeLists.txt

@@ -35,5 +35,13 @@ add_custom_target(run-unit-tests
    USES_TERMINAL
 )

+add_custom_target(run-python-binding-tests


I am not so sure if this is really a unit-test, as its not testing individual and isolated parts of the code (e.g. functions).

Instead the Python test seems a bit more elaborate.

What do you think about adding it as an integration test? Or maybe as a dedicated test in itself?

I am interested in your point of view here :)

I agree that the mentioned tests extend beyond unit tests.
I would consider splitting them up into unit and integration tests.

While discussing tests, I would like to mention the Python tests in python/villas/node.

The folder structure node/python/villas/node appears a little confusing to me.
That said, I noticed that there seems to be test code as well as production code within that folder.

Therefore I would propose either creating a separate location for Python related tests or to also split them up into unit and integration tests. I believe this would benefit the clarity of the projects structure.

stv0g · 2025-05-22T14:21:18Z

clients/python-binding/CMakeLists.txt

Can we move this to python/binding?

The clients folder was created to collect external code which interacts with VILLASnode. But, I would consider the Python bindings to be a more integral part of VILLASnode as its actually executing code from libvillas here.

Furthermore, this would keep all the VILLASnode / Python-related code in on place (the python directory).

This brings also up another question. I've seen that you named the native Python extension villas_node. This does not seem to fit well in the naming schema of the existing Python packages for VILLASnode:

villas

villas.node

villas.controller

villas.dataprocessing

See also here: https://pypi.org/search/?q=villas-

Ideally, I would like to bundle your bindings into a villas.node.pybind package and also publish it to PyPi at some point.

That would allow for installing the bindings, including the native extension, via pip install villas-node`.

What do you think about it? Let me know if I can support you here.

Moving the folder to python/binding seems like a good idea to me.
I suppose that the name of the module should be changed to villas-node-binding or villas-node-pybind?

About the integration with CMake and packaging:

I initially tried integrating the python-binding into the CMake build system to generate a package/wheel and install it during the process.
This turned out to be quite messy from the install/build side of things - besides me failing to get it working.

But during all the trial and error some concerns came up, which I believe to be very relevant when considering pybind11, Python bindings in general and packaging it.

Python bindings are tightly coupled to the Python version used when compiling against the Python C API.

They also link - at least to my understanding - against a particular version of the library (in this case libvillas), which may vary depending on the system libraries during the build.

As a pip package can not control system libraries and dependencies - not that it ever should the case - this makes packaging the binding problematic to say the least.
Unfortunately you can not make pip install or build VILLASnode itself, which would solve some a lot of issues in this case, can you?

What is your take on this?

Moved `python-devel` to align with the Ubuntu and Debian Dockerfile. The Fedora one remains the exception. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

al3xa23 · 2025-07-10T09:54:22Z

Hi @stv0g, can you please give some (final?) comment here so that we can proceed? Thanks :)

Kevin Vu te Laar added 7 commits April 4, 2025 08:52

Build system for python-wrapper

40c9103

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 <vu.te@rwth-aachen.de>

Added wrapper dependencies to Dockerfiles

7302be2

Added python and pybind11 dependencies to Dockerfiles, except for Fedora minimal, to build the Python-wrapper. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

Python-Wrapper bindings

3703c20

Added Python-Wrapper bindings. Docstrings are still missing. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

node.cpp Bugfix

24abdb7

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 <vu.te@rwth-aachen.de>

Applied changes as per feedback.

862d498

Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

al3xa23 requested review from stv0g and n-eiling as code owners April 4, 2025 06:55

al3xa23 and others added 4 commits April 7, 2025 16:56

Merge branch 'master' into python-wrapper

c48be8f

Fixed typo

73ce98f

The target is `run-python-wrapper-tests` instead of `python-wrapper-tests` Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

CMakeLists.txt fix

e710de5

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 <vu.te@rwth-aachen.de>

stv0g reviewed Apr 8, 2025

View reviewed changes

stv0g changed the title ~~Python wrapper~~ Add Python bindings for libvillas Apr 24, 2025

stv0g added python enhancement New feature or request labels Apr 24, 2025

al3xa23 marked this pull request as draft May 5, 2025 07:26

Kevin Vu te Laar added 2 commits May 15, 2025 08:15

Removed unnecessary dockerfile dependencies

b1e9a2f

Based on suggestions from PR#884. Removed the `python` dependency because `python-devel` in Debian and Rocky also depend on the base `python` package. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

Renamed wrapper -> binding, LLVM format

365f88b

As suggested `wrapper` was substituted by `binding`. The minimum required version and project naming in `CMakeLists.txt` were removed, as this is not necessary. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

Kevin Vu te Laar and others added 4 commits May 15, 2025 08:15

Merge branch 'master' into python-wrapper

c5026a2

Signed-off-by: al3xa23 <140614263+al3xa23@users.noreply.github.com>

KeVteL reviewed May 22, 2025

View reviewed changes

KeVteL marked this pull request as ready for review May 22, 2025 12:56

stv0g requested changes May 22, 2025

View reviewed changes

Align python-devel package for consistency

07cffec

Moved `python-devel` to align with the Ubuntu and Debian Dockerfile. The Fedora one remains the exception. Signed-off-by: Kevin Vu te Laar <vu.te@rwth-aachen.de>

		- 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

Add Python bindings for libvillas #884

Are you sure you want to change the base?

Add Python bindings for libvillas #884

Uh oh!

Conversation

al3xa23 commented Apr 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stv0g commented Apr 24, 2025

Uh oh!

al3xa23 commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KeVteL commented May 22, 2025

Uh oh!

stv0g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KeVteL Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KeVteL Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

About the integration with CMake and packaging:

Uh oh!

al3xa23 commented Jul 10, 2025

Uh oh!

Uh oh!

KeVteL Jun 12, 2025 •

edited

Loading

KeVteL Jun 5, 2025 •

edited

Loading