Generate API clients based on description #373

engelmi · 2023-06-29T09:04:23Z

This PR

introduces a D-Bus API specification to client code generator.
adds generated python client code and reworks the existing python bindings
adds an additional lint step to check if there are API spec changes that are not yet in the generated code
adds a new CI job to publish a new pyhirte package to PyPI (manually triggered), whereas the version is linked to the hirte version defined in meson

dougsland · 2023-06-29T19:31:50Z

[root@control python]# pip3 install pyhirte
Collecting pyhirte
  Downloading pyhirte-0.1.0-py3-none-any.whl (14 kB)
Collecting dasbus>=1.7
  Downloading dasbus-1.7-py3-none-any.whl (63 kB)
     |████████████████████████████████| 63 kB 1.3 MB/s
Installing collected packages: dasbus, pyhirte
Successfully installed dasbus-1.7 pyhirte-0.1.0
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv



# ./ListAllNodes
Traceback (most recent call last):
  File "/root/hirte/src/bindings/examples/python/./ListAllNodes", line 5, in <module>
    from pyhirte import HirteManager
  File "/usr/local/lib/python3.9/site-packages/pyhirte/__init__.py", line 1, in <module>
    from pyhirte.gen import *
  File "/usr/local/lib/python3.9/site-packages/pyhirte/gen/__init__.py", line 1, in <module>
    from .hirte import *
  File "/usr/local/lib/python3.9/site-packages/pyhirte/gen/hirte.py", line 7, in <module>
    class Hirte:
  File "/usr/local/lib/python3.9/site-packages/pyhirte/gen/hirte.py", line 18, in Hirte
    def get_proxy(self) -> InterfaceProxy | ObjectProxy:
TypeError: unsupported operand type(s) for |: 'ABCMeta' and 'ABCMeta'

It's hard to understand what's going on here without having someone that really knows what's going on in the parser/generator side, We will need to dup(mengel)

dougsland

Let me start with: this is a nice work and really remember me a parsing/compiler classes. :)

Now few comments:

In general, good, should work (not there yet as I shared in the comment).
Such parser/generator would deserve a separate repo IMHO
Pro: Automatically generation of python/golang/whatever code.
Cons: Increase the complexity to maintain binding code as we need to understand the generator (instead of pure python as today). What happens if we want to generate golang? rust? A new shine language? Probably we will need to create a class/methods to adapt it?

Another point about simplicity, today we have all methods from Hirte() class which to my eyes seems pretty simple but with this implementation now we have more objects/classes (inherence) like HirteManager(), HirteNode(), HirteUnit() which is normal as the parser reads from the specification. How about we generate a docstring about the API too?

For the future, I would transform this project to generic parser/generator from dbus to whatever language users would like. Today is completely related to hirte but has potential to be generic and used for many of hundreds of projects out there.

Let's see the next round of patch to test it again. Also, would help if you could squash all these commits.

src/bindings/generator/src/dbus_typing.py

src/bindings/generator/src/generator.py

src/bindings/python/pyhirte/gen/metrics.py

src/bindings/python/pyhirte/gen/monitor.py

src/bindings/python/pyhirte/gen/node.py

src/bindings/README.md

engelmi · 2023-06-30T07:24:54Z

[root@control python]# pip3 install pyhirte
Collecting pyhirte
  Downloading pyhirte-0.1.0-py3-none-any.whl (14 kB)
Collecting dasbus>=1.7
  Downloading dasbus-1.7-py3-none-any.whl (63 kB)
     |████████████████████████████████| 63 kB 1.3 MB/s
Installing collected packages: dasbus, pyhirte
Successfully installed dasbus-1.7 pyhirte-0.1.0
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv



# ./ListAllNodes
Traceback (most recent call last):
  File "/root/hirte/src/bindings/examples/python/./ListAllNodes", line 5, in <module>
    from pyhirte import HirteManager
  File "/usr/local/lib/python3.9/site-packages/pyhirte/__init__.py", line 1, in <module>
    from pyhirte.gen import *
  File "/usr/local/lib/python3.9/site-packages/pyhirte/gen/__init__.py", line 1, in <module>
    from .hirte import *
  File "/usr/local/lib/python3.9/site-packages/pyhirte/gen/hirte.py", line 7, in <module>
    class Hirte:
  File "/usr/local/lib/python3.9/site-packages/pyhirte/gen/hirte.py", line 18, in Hirte
    def get_proxy(self) -> InterfaceProxy | ObjectProxy:
TypeError: unsupported operand type(s) for |: 'ABCMeta' and 'ABCMeta'

It's hard to understand what's going on here without having someone that really knows what's going on in the parser/generator side, We will need to dup(mengel)

The generator is not part of the pyhirte package, but the error is linked to the generated code. It seems that the | symbol - which can be used like the typing.Union - has been introduced in python3.10 (which I use) - see python3.10 release notes. So adding from __future__ import annotations should fix this for python3.9.
Good finding! @dougsland

engelmi · 2023-06-30T12:28:12Z

* Cons: Increase the complexity to maintain binding code as we need to understand the generator (instead of pure python as today). What happens if we want to generate golang? rust? A new shine language? Probably we will need to create a class/methods to adapt it?

If we want to generate a client for another language like go, rust, etc. the only thing that is required are proper Jinja2 templates. For example, just passing the path to the rust template to the generator should result in the rust client for the hirte api.

Another point about simplicity, today we have all methods from Hirte() class which to my eyes seems pretty simple but with this implementation now we have more objects/classes (inherence) like HirteManager(), HirteNode(), HirteUnit() which is normal as the parser reads from the specification.

I refactored it a bit so we generate only one file, but still multiple classes - each class represents an interface. This is also a bit clearer in regards to which API "endpoint" is actually used and makes using them rather explicit, e.g. by using

node = HirteNode("laptop")

it makes sense to pass the node name to it. In addition, the lifetime of the dbus proxy is linked to the object - which is important for the monitor, for example - and the connection is reused per instance makes it more efficient than (re-)opening it per function call.

How about we generate a docstring about the API too?

That would be great to have and would be possible if we extend our API description e.g. by annotations or doc tags - these can be parsed by the generator and passed to the templates.

For the future, I would transform this project to generic parser/generator from dbus to whatever language users would like. Today is completely related to hirte but has potential to be generic and used for many of hundreds of projects out there.

Maybe, yes. But for now its rather hirte specific. I am still baffled that there is no such project yet, to be honest.

Also, would help if you could squash all these commits.

I'll rebased them a bit, so it should be easier to review. But since its a pretty big change, I'd not want to put everything in one commit.

dougsland · 2023-06-30T14:48:36Z

Starting with some quick comments:

# ./ListAllNodes
Traceback (most recent call last):
  File "/root/hirte/src/bindings/examples/python/./ListAllNodes", line 9, in <module>
    print(f"Node: {node[0]}, State: {node[3]}")
IndexError: tuple index out of range

and

# shellcheck ./build-scripts/generate-bindings.sh

In ./build-scripts/generate-bindings.sh line 15:
    python3 $generator $data_dir $template_dir $output_file_path
            ^--------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                       ^-------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                 ^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.
                                               ^---------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
    python3 "$generator" "$data_dir" "$template_dir" "$output_file_path"


In ./build-scripts/generate-bindings.sh line 16:
    black $output_file_path
          ^---------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
    black "$output_file_path"

For more information:
  https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

src/bindings/generator/src/dbus_typing.py

engelmi · 2023-07-03T07:15:35Z

./ListAllNodes
Traceback (most recent call last):
File "/root/hirte/src/bindings/examples/python/./ListAllNodes", line 9, in
print(f"Node: {node[0]}, State: {node[3]}")
IndexError: tuple index out of range

Fixed.

shellcheck ./build-scripts/generate-bindings.sh
...
Did you mean:
python3 "$generator" "$data_dir" "$template_dir" "$output_file_path"
...
Did you mean:
black "$output_file_path"

Added the quotes.

src/bindings/generator/src/dbus_typing.py

src/bindings/generator/src/generator.py

src/bindings/python/LICENSE

alexlarsson · 2023-07-03T13:37:01Z

Why "temporarily removed client package from rpm"?

engelmi · 2023-07-03T14:12:10Z

Why "temporarily removed client package from rpm"?

I couldn't figure out how to use the python macros to add it to the rpm, e.g. %pyproject_wheel. I would need help for this and wanted to tackle it in a follow-up PR. If its not difficult, we can do it in this PR, of course. @alexlarsson

alexlarsson · 2023-07-03T14:28:56Z

I guess since it the python code was not in 0.3 we're not regressing if we temporarily remove it.

dougsland · 2023-07-03T15:26:07Z

Why "temporarily removed client package from rpm"?

I couldn't figure out how to use the [python macros](https://docs.fedoraproject.org/en-US/packaging-
guidelines/Python/) to add it to the rpm, e.g. %pyproject_wheel. I would need help for this and wanted to
tackle it in a follow-up PR. If its not difficult, we can do it in this PR, of course. @alexlarsson

I had the same problem that why I used %install instead of these pre-defined macros. You see the same problem using the %install?

dougsland · 2023-07-03T15:26:38Z

I guess since it the python code was not in 0.3 we're not regressing if we temporarily remove it.

Agreed, if required, spec can always wait a bit. It will not hurt.

Signed-off-by: Michael Engel <[email protected]>

- generated dbus api client for python - refactored current python client - extended the generated client - refactored the python client examples - temporarily removed client package from rpm Signed-off-by: Michael Engel <[email protected]>

- added a lint CI step to check for api spec changes - added a manually triggered job to publish pypi package Signed-off-by: Michael Engel <[email protected]>

dougsland · 2023-07-06T20:53:30Z

Great patch a few comments, let's merge and you can continue the journey with the minor changes to improve this solution.

BTW, could you please also verify this one? This is the only one example not working for me now.

./EnableUnit

Traceback (most recent call last):
File "/root/hirte/src/bindings/examples/python/./EnableUnit", line 10, in
if response.carries_install_info:
AttributeError: 'tuple' object has no attribute 'carries_install_info'

dougsland · 2023-07-06T20:53:34Z

LGTM

dougsland · 2023-07-06T20:53:55Z

sorry the delay to review, this week I took PTO days.

src/bindings/generator/src/generator.py

src/bindings/generator/test/test_dbus_typing.py

* aligned hirte api description with implementation Signed-off-by: Michael Engel <[email protected]> * implemented dbus api client generator Signed-off-by: Michael Engel <[email protected]> * rework of python client for hirte dbus api - generated dbus api client for python - refactored current python client - extended the generated client - refactored the python client examples - temporarily removed client package from rpm Signed-off-by: Michael Engel <[email protected]> * extended github workflow for dbus api changes - added a lint CI step to check for api spec changes - added a manually triggered job to publish pypi package Signed-off-by: Michael Engel <[email protected]> --------- Signed-off-by: Michael Engel <[email protected]>

dougsland marked this pull request as ready for review June 29, 2023 19:32

dougsland requested review from alexlarsson, mkemel, rhatdan and sdunnagan as code owners June 29, 2023 19:32

dougsland reviewed Jun 29, 2023

View reviewed changes

dougsland reviewed Jun 30, 2023

View reviewed changes

src/bindings/generator/src/dbus_typing.py Outdated Show resolved Hide resolved

src/bindings/generator/src/dbus_typing.py Outdated Show resolved Hide resolved

src/bindings/generator/src/dbus_typing.py Outdated Show resolved Hide resolved

alexlarsson reviewed Jul 3, 2023

View reviewed changes

src/bindings/generator/src/dbus_typing.py Outdated Show resolved Hide resolved

alexlarsson reviewed Jul 3, 2023

View reviewed changes

src/bindings/generator/src/generator.py Outdated Show resolved Hide resolved

alexlarsson reviewed Jul 3, 2023

View reviewed changes

src/bindings/python/LICENSE Outdated Show resolved Hide resolved

engelmi added 4 commits July 6, 2023 11:37

aligned hirte api description with implementation

a151813

Signed-off-by: Michael Engel <[email protected]>

implemented dbus api client generator

3b40a1e

Signed-off-by: Michael Engel <[email protected]>

rework of python client for hirte dbus api

faa1497

- generated dbus api client for python - refactored current python client - extended the generated client - refactored the python client examples - temporarily removed client package from rpm Signed-off-by: Michael Engel <[email protected]>

extended github workflow for dbus api changes

f521165

- added a lint CI step to check for api spec changes - added a manually triggered job to publish pypi package Signed-off-by: Michael Engel <[email protected]>

dougsland approved these changes Jul 6, 2023

View reviewed changes

src/bindings/generator/src/generator.py Show resolved Hide resolved

src/bindings/generator/src/generator.py Show resolved Hide resolved

src/bindings/generator/test/test_dbus_typing.py Show resolved Hide resolved

dougsland merged commit 47a6038 into eclipse-bluechi:main Jul 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate API clients based on description #373

Generate API clients based on description #373

engelmi commented Jun 29, 2023 •

edited

Loading

dougsland commented Jun 29, 2023 •

edited

Loading

dougsland left a comment

engelmi commented Jun 30, 2023

engelmi commented Jun 30, 2023

dougsland commented Jun 30, 2023

engelmi commented Jul 3, 2023 •

edited

Loading

alexlarsson commented Jul 3, 2023

engelmi commented Jul 3, 2023 •

edited

Loading

alexlarsson commented Jul 3, 2023

dougsland commented Jul 3, 2023

dougsland commented Jul 3, 2023

dougsland commented Jul 6, 2023

dougsland commented Jul 6, 2023

dougsland commented Jul 6, 2023

Generate API clients based on description #373

Generate API clients based on description #373

Conversation

engelmi commented Jun 29, 2023 • edited Loading

dougsland commented Jun 29, 2023 • edited Loading

dougsland left a comment

Choose a reason for hiding this comment

engelmi commented Jun 30, 2023

engelmi commented Jun 30, 2023

dougsland commented Jun 30, 2023

engelmi commented Jul 3, 2023 • edited Loading

alexlarsson commented Jul 3, 2023

engelmi commented Jul 3, 2023 • edited Loading

alexlarsson commented Jul 3, 2023

dougsland commented Jul 3, 2023

dougsland commented Jul 3, 2023

dougsland commented Jul 6, 2023

./EnableUnit

dougsland commented Jul 6, 2023

dougsland commented Jul 6, 2023

engelmi commented Jun 29, 2023 •

edited

Loading

dougsland commented Jun 29, 2023 •

edited

Loading

engelmi commented Jul 3, 2023 •

edited

Loading

engelmi commented Jul 3, 2023 •

edited

Loading