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

Hydrogen python bindings #1313

Open
wants to merge 25 commits into
base: development
Choose a base branch
from

Conversation

charbeljc
Copy link
Contributor

Second take at my experiments with pybind11. Most of libhydrogen-core classes are accessible from python. Tested with python3 only.

In a virtualenv with python3

WANT_PYBINDINDS=1 ./build.sh mm
cd build/src/bindings
make install
python
>>> import h2core
>>> help(h2core)

Be ready for segfaults and coredumps. Regards.

@charbeljc charbeljc mentioned this pull request Jul 3, 2021
@charbeljc charbeljc force-pushed the hydragen_bindings branch 2 times, most recently from ef3ebfd to 33d989b Compare July 7, 2021 20:36
@theGreatWhiteShark
Copy link
Contributor

Hey @charbeljc,

We finally managed to talk about this PR internally. Sorry for the delay.

First of all: Do you have an use case your are aiming for or know other people that have? Also, what exactly will these changes encompass when everything is done?

We also wondered where would be the best place to put this code. Inside the main repo does not appear to be a fitting place. After all, we are still in the process of making improvements and modernization of the core (some parts are more than 20 years old. You already have seen the mess 😄 ) and try to keep master clean and working at all times. This would result in introducing changes to the API every other week what would basically render it too unstable to use. Our current release cycle is one year (well, at least in theory). So, making the Python API compatible with only the major releases seems like a good idea.

You are also working on a custom Python binding library, right? Is it because pybind11 is not working properly or appears to be not stable enough? How about hosting the API close to the binding library since it's a direct dependence? AFAIU it only relies on the libhydrogen-core-1.1.0.so (and a potential GUI shared object that is not produced for now. Having it would be a good idea anyway since it would allow to cover parts of the GUI in the unit tests). So, it could be bundled and shipped separately and references in the Hydrogen wiki/doc etc. What do you think? I hope this is not too much of a bummer.

@charbeljc
Copy link
Contributor Author

Hey @charbeljc,

We finally managed to talk about this PR internally. Sorry for the delay.

Hi @theGreatWhiteShark no worries about the delay, I understand that introducing python bindings like this needs careful overview and understanding of the move. I'll try to clarify my goals.

First of all: Do you have an use case your are aiming for or know other people that have? Also, what exactly will these changes encompass when everything is done?

Ok, what's the purpose of those python bindings, after all ? To make it short, QA, refactoring support, API stability tracking, lower the entry point to the code base. It might give some ideas later to some people (including me) to hack something with those bindings, but the main point is to allow us to easily write more unit tests for all aspects of hydrogen core with the ease of use of the pytest framework.

Right now, I did back off from my first draft, where the equivalent of h2core_module.{h,cpp} where automatically generated during the build process, but it should be easily restored. The only additional external build dependencies would be python3-dev, python3-pip, pybind11, and my dedicated bindings generator, those last two one being pip installable.

If you look at my patch, You'll see that if you take apart the parts that are included in my work on H2Core::Object templatification, This patch is pretty additive and for now optional. Once we are done with this PR on H2Core::Object work, I will rebase this one.

We also wondered where would be the best place to put this code. Inside the main repo does not appear to be a fitting place. After all, we are still in the process of making improvements and modernization of the core (some parts are more than 20 years old. You already have seen the mess smile ) and try to keep master clean and working at all times. This would result in introducing changes to the API every other week what would basically render it too unstable to use. Our current release cycle is one year (well, at least in theory). So, making the Python API compatible with only the major releases seems like a good idea.

Honestly Hydrogen code base is not that a mess, I've seen much worse in my (too long) career :-) But that's exactly the purpose of introducing python bindings as an internal quality tool. Having those bindings as an internal consumer of Hydrogen API, used as part of our build process with an easy to augment pytest unit tests collection will actually help us refactoring Hydrogen without breaking it.

You are also working on a custom Python binding library, right? Is it because pybind11 is not working properly or appears to be not stable enough? How about hosting the API close to the binding library since it's a direct dependence? AFAIU it only relies on the libhydrogen-core-1.1.0.so (and a potential GUI shared object that is not produced for now. Having it would be a good idea anyway since it would allow to cover parts of the GUI in the unit tests). So, it could be bundled and shipped separately and references in the Hydrogen wiki/doc etc. What do you think? I hope this is not too much of a bummer.

I was maybe a bit imprecise on this part. I'm working on a custom Python bindings generator tool, targeting stock pybind11, headers only, C++ python binding framework, which is one of the most beautiful, robust and battle field proved way of making python bindings for C++ code (see this for instance). I just didn't found what I wanted in terms of code generation, so I ended up hacking my (for now) quick and dirty one in python, leveraging clang python bindings to parse hydrogen's headers.

So to be clear, maintaining those python bindings outside Hydrogen would defeat the main goal as an internal QA tool. Besides, If I can make guaranties on my ability to maintain just the binding generator part, I can't give you any assurance that I could have enough time to maintain the actual python bindings as a standalone package, with the burden of publishing them on pypi and so on.

As for having an libhydrogen-gui-1.1.0.so, that's also part of the plan, yes, although I must take the time to experiment more on the binding side, because, when it comes to Qt heavy code base, making use of shiboken would be maybe more natural. Anyway, i will extract a PR with just the libhydrogen-gui.so part, It won't hurt anyway to have it merged quick.

To conclude, having python bindings as integral part of Hydrogen to support unit testing is the way to go, as I can assert from my recent patches. (I have on my machine some pretty impressive demos regarding automated gui testing of pan and velocity envelope editing, not ready for prime time, alas).

So the plan would be. Heat our cake via pytest in the first place without bothering to publish those bindings to pypi. Then, if there is interest from someone to actually use them to make noise in an innovative way, reconsider our position and take the burden to actually publish them.

Regards,
Charbel

@theGreatWhiteShark
Copy link
Contributor

Ah, I see. We mistook the testing use case for an arbitrary example instead for the main purpose.

So, pytest is so much more powerful than the cppunit test "framework" we use right now? When using pytest during my last job it felt pretty much the same. But I probably missed some magic.

@charbeljc
Copy link
Contributor Author

Ah, I see. We mistook the testing use case for an arbitrary example instead for the main purpose.

So, pytest is so much more powerful than the cppunit test "framework" we use right now? When using pytest during my last job it felt pretty much the same. But I probably missed some magic.

There is, in fact a great ecosystem around pytest which can bring quite some magic to the framework, but honestly it's hard to explain why cppunit and pytest, having on the surface roughly the same capabilities leads to so different experiences. I guess, from my long practice, that's because there is a natural flow from what you can thinker playing with your API on an ipython REPL or jupyter notebook to a pytest unit test. Having your C++ API available in an interactive REPL leads naturally to more tests. Just because there is no delay between your "what if ?" and the actual realization of it.

I did have a look at Hydrogen, maybe 10 or 15 years ago, and, as an amateur Batucada player I love it much because it actually helped me understand many things in percussion. We have only 53 tests under the cppunit framework. For a 20 year old project,it speaks for itself 😁

I'm currently extending my python binding generator so that it also generate mypy stubs so you can have full completion of the API in any editor supporting python language servers. I think this will be a great addition to the python bindings, lowering the barrier of entry to Hydrogen contributions.

(So, to conclude, yes, I'm a QA boy, I guess, but a serious one, bringing actual tooling, and not using excel or making slides :-) )

@cme
Copy link
Contributor

cme commented Jul 19, 2021

Ah, I see, this makes a bit more sense of the motivation. Thanks for clarifying!

I think the lack of existing tests speaks more to the nature of the project rather than the available testing infrastructure, namely:

  1. Hydrogen is driven by enthusiasm. That naturally tends to skew towards spending time implementing features rather than testing, and
  2. Many of the things we'd want unit or feature regression tests for are difficult to observe and verify (audio, timing, GUI appearance) are difficult to observe and verify in a robust automated way.

If Pytest can help with any of the things in bucket number 2, I would like to hear about it! :)

@charbeljc
Copy link
Contributor Author

charbeljc commented Jul 22, 2021

Ah, I see, this makes a bit more sense of the motivation. Thanks for clarifying!

I think the lack of existing tests speaks more to the nature of the project rather than the available testing infrastructure, namely:

1. Hydrogen is driven by enthusiasm. That naturally tends to skew towards spending time implementing features rather than testing, and

Granted, writing tests is not as fun as adding features... That's a reason why if we can bring fun in hydrogen testing with python bindings, it should be a good thing.

2. Many of the things we'd want unit or feature regression tests for are difficult to observe and verify (audio, timing, GUI appearance) are difficult to observe and verify in a robust automated way.

If Pytest can help with any of the things in bucket number 2, I would like to hear about it! :)

Yes, I know, audio and realtime properties can be hard to test, especially if your code base is not prepared for this kind of kung-fu. For the GUI aspect, I think we could leverage pytest-qt at least to unit test our dedicated knob widgets and likes.

I did some extensive work on Sample/Instrument Editor, and my python bindings actually helped me in the process (see this for instance).

But let me insist. The magic don't comes from the pytest framework, but from having an actual other consumer of Hydrogen API, and one that you can thinker with in an interactive way.

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.

3 participants