-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
[GSoC 2020] Extending PCL for use with Python: Bindings generation using Pybind11 #4366
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
base: master
Are you sure you want to change the base?
Changes from all commits
7981a1c
cab3031
c7498c1
ed344c6
066a5ce
65f3d19
40eab68
afbc1c7
4f37afd
b1e3cc9
ff4a689
37acd51
1e19e67
b432122
4a2a548
0b2dfdf
0994867
421cf62
3598b20
5bdd5ca
ff92a9e
fedded1
29eb197
a4ab71f
6f4bf43
f2bdfad
b8c1a6b
c915cd1
8c307e9
5d7b8cf
82dd17c
8eab1d8
6b2e569
42aaa43
3aedbfe
eeffca5
48f729e
c95ac4e
884e733
9f5df82
15089b3
0dd20ff
f424ea8
c1ab9a2
7f9e0af
0c31b37
a6dd4f8
4797bdb
aaab507
d9f2243
10ee1fe
321edc9
fcfc69e
ff0d19d
35ff1fd
72b22c5
a5baeb8
bab60ba
42c3648
4148184
73d895e
9730365
5e89ada
fa79863
428e074
586d127
efbf9f9
47cd10c
84db804
377bf23
0ca9cbc
a0bef42
67db056
5e01ee6
7e6719d
4acfb38
01ca66a
ad34e5f
c212300
db68b45
098d915
4886e82
59a4920
ffd8041
04a5de4
3683aa5
05e10d5
69cacc7
45378d8
6f6586d
7d04ace
7ffc2d3
52b10b6
d41a0f7
a2e530e
12c19cc
09885dc
20aee2d
7559807
70690db
4dae2b5
ffb8d60
8bf2c7b
1e18a0f
02de83f
fcd0c14
cfce950
242d057
8c6e53a
9712956
8fd421f
f8a51f9
bdd94bc
a5f592f
bebc780
b7b6f07
2196bcd
17b41a2
31b21e9
3a8f29a
365d98b
214b654
24e9d30
f9404e9
7f52454
e172799
09a5b2b
19915ca
8b6585c
c9f1125
998bef6
bcb514e
f53e3e9
5cf5eb9
5ef3164
e7eb3a5
fd55e5c
fdaad6f
7093ac7
9903317
6aca243
a47f24a
1bc9655
48f3856
4d21a35
7f47c28
f3dd722
3278307
f4d92ef
23ed6a3
0beb1ce
48e792e
d6b8cd4
e59d2b3
f1bca02
1268d44
e5fbc9c
e3440f9
89dbbab
03121d8
061fc77
a09ab58
b574125
ffe92ec
8b53911
25692eb
0a9624b
2f97370
2995ba6
54491a9
ae5dcd4
24dafcd
1f0055e
387dab3
d440805
7a0e69d
d8c0816
a3b9c81
f09f4ad
527555c
f1b9923
c7027ae
c496539
581e847
69e4944
9443be5
9c43bf2
58d5f31
d129de9
7eba647
e2caad9
9695248
e40a17b
d31691f
0d09658
232dd98
12617c2
28a1673
5875c31
86b10f0
ba275b7
9a70fa1
95016c2
40a5c7e
d7f8f4b
3521b35
999172c
86483cd
8515a7b
a88b261
4b32ed9
4b545f0
eea0e90
ed97be5
ea8b109
26b6de8
2b85984
6b09e86
a9092f0
d074f13
17298d2
fa2b67b
68bfb14
933da99
9a78047
ae1c9ce
f41e9e0
4a1c70b
244d49a
f887f04
3f4ae5e
053c28c
8a78e9f
276dcda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
name: Black Formatting | ||
|
||
on: [push] | ||
|
||
# A workflow run is made up of one or more jobs that can run sequentially or in parallel | ||
jobs: | ||
Black: | ||
# The type of runner that the job will run on | ||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
# Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it | ||
- uses: actions/checkout@v2 | ||
|
||
# Setup python version | ||
- name: Set up Python | ||
uses: actions/setup-python@v2 | ||
with: | ||
python-version: "3.8" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? Can we have something like "latest" so we don't need to update the version? |
||
|
||
# Install pip and packages | ||
- name: Install pip | ||
run: python -m pip install --upgrade pip | ||
|
||
- name: Install black | ||
run: pip install black | ||
|
||
# Format with black | ||
- name: Format with black | ||
run: | | ||
cd bindings/python | ||
black . | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we run black on .dev/scripts too? |
||
|
||
# Diff check | ||
- name: Check git diff | ||
# Output the diff in ${GITHUB_WORKSPACE} | ||
run: git diff > black_formatting_diff.patch | ||
|
||
# Exit if diff | ||
- name: Set job exit status | ||
run: "[ ! -s black_formatting_diff.patch ]" | ||
|
||
# Artifacts | ||
- name: Upload formatting diff | ||
uses: actions/upload-artifact@v2 | ||
with: | ||
# We are in ${GITHUB_WORKSPACE} | ||
# ${GITHUB_SHA} won't work: use ${{ github.sha }} | ||
name: black_formatting_diff-${{ github.sha }} | ||
path: black_formatting_diff.patch | ||
# Use always() to always run this step to publish test results when there are test failures | ||
if: ${{ always() }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
name: Pytest | ||
|
||
on: [push] | ||
|
||
# A workflow run is made up of one or more jobs that can run sequentially or in parallel | ||
jobs: | ||
Pytest: | ||
# The type of runner that the job will run on | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: [3.6, 3.7, 3.8] | ||
|
||
steps: | ||
# Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it | ||
- uses: actions/checkout@v2 | ||
|
||
# Setup python version | ||
- name: Set up Python ${{ matrix.python-version }} | ||
uses: actions/setup-python@v2 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
- name: Display python version | ||
run: python -c "import sys; print(sys.version)" | ||
|
||
# Install pip and packages | ||
- name: Install pip | ||
run: python -m pip install --upgrade pip | ||
|
||
- name: Install pytest | ||
run: pip install pytest | ||
|
||
# Install clang and it's python inteface via apt | ||
- name: Add llvm keys | ||
run: | | ||
wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add - | ||
echo 'deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-11 main' | sudo tee -a /etc/apt/sources.list | ||
echo 'deb-src http://apt.llvm.org/bionic/ llvm-toolchain-bionic-11 main' | sudo tee -a /etc/apt/sources.list | ||
Comment on lines
+37
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kunaltyagi can/should these two There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace the name with |
||
- name: Install libclang and its python bindings | ||
run: | | ||
sudo apt-get update | ||
sudo apt-get install -y libclang-11-dev python3-clang-11 | ||
|
||
# Add dist-package to path to enable apt installed python3-clang import | ||
- name: Add dist-packages to PYTHONPATH | ||
run: echo "::set-env name=PYTHONPATH::${PYTHON_PATH}:/usr/lib/python3/dist-packages" | ||
- name: Display PYTHONPATH | ||
run: python -c "import sys; print('\n'.join(sys.path))" | ||
|
||
# Test with pytest | ||
- name: Test with pytest | ||
run: cd ${_PYTHON_BINDINGS_PATH} && pytest --junitxml=${GITHUB_WORKSPACE}/result_${{ matrix.python-version }}.xml | ||
env: | ||
_PYTHON_BINDINGS_PATH: bindings/python | ||
|
||
# Artifacts | ||
- name: Upload pytest test results | ||
uses: actions/upload-artifact@v2 | ||
with: | ||
# We are in ${GITHUB_WORKSPACE} | ||
# ${GITHUB_SHA} won't work: use ${{ github.sha }} | ||
name: pytest_py${{ matrix.python-version }}-${{ github.sha }} | ||
path: result_${{ matrix.python-version }}.xml | ||
# Use always() to always run this step to publish test results when there are test failures | ||
if: ${{ always() }} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,158 @@ | ||
## Python bindings | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this file be included in the PR as is or modified? |
||
|
||
### Shared pointer usage | ||
#### [05-06-2020] | ||
- > When we want `cloud = pcl.PointCloud[pcl.PointXYZ]()` to be a shared_ptr by default, we set the holder class as shared_ptr. This is needed in some cases because the interface otherwise would be non-idomatic: | ||
> ```py | ||
> import pcl | ||
> | ||
> filter = pcl.filters.PassThrough[pcl.PointXYZ]() | ||
> filter.setInput(cloud) | ||
> ``` | ||
> | ||
> Here, cloud needs to be a shared_ptr. That can be done in 2 ways | ||
> 1. `cloud = pcl.PointCloud[pcl.PointXYZ]()` and the holder class as shared_ptr | ||
> 2. `cloud = pcl.make_shared[pcl.PointXYZ]()` and holder class as void | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
> | ||
> The issue is the ease-of-use and expected semantics | ||
> ```py | ||
> cloud2 = cloud1 # Python user will assume this is a shallow copy | ||
> ``` | ||
> | ||
> This will only be true for a variable held in a shared_ptr. This is the semantics in Python. | ||
> | ||
> However, wrapping everything in shared_ptr has downsides for C++ wrapper: | ||
> ```py | ||
> import pcl | ||
> | ||
> point = pcl.PointXYZ() | ||
> cloud[100] = point | ||
> ``` | ||
> | ||
> If PointXYZ is held in a shared_ptr... things go south. If not, things go south | ||
|
||
### Handling unions | ||
#### [04-06-2020] | ||
- > given `assert(&(union.r) == &(union.rgb));` does the following hold: | ||
> `assert id(union_wrapper.r) == id(union_wrapper.rgb) ?` | ||
Yes. Tested. | ||
- Working example: | ||
```cpp | ||
#include <pybind11/pybind11.h> | ||
namespace py = pybind11; | ||
|
||
union RGB { | ||
int rgb; | ||
struct { | ||
int r; | ||
int g; | ||
int b; | ||
}; | ||
}; | ||
|
||
PYBIND11_MODULE(pcl, m) | ||
{ | ||
py::class_<RGB>(m, "RGB") | ||
.def(py::init<>()) | ||
.def_property( | ||
"rgb", | ||
[](RGB& self) -> int { return self.rgb; }, | ||
[](RGB& self, int value) { self.rgb = value; }) | ||
.def_property( | ||
"r", | ||
[](RGB& self) -> int { return self.r; }, | ||
[](RGB& self, int value) { self.r = value; }) | ||
.def_property( | ||
"g", | ||
[](RGB& self) -> int { return self.g; }, | ||
[](RGB& self, int value) { self.g = value; }) | ||
.def_property( | ||
"b", | ||
[](RGB& self) -> int { return self.b; }, | ||
[](RGB& self, int value) { self.b = value; }); | ||
} | ||
``` | ||
|
||
### General | ||
#### [05-06-2020] | ||
- MetaCPP relies on Clang's LibTooling to generate all the metadata: https://github.com/mlomb/MetaCPP | ||
|
||
#### [04-06-2020] | ||
- > Was reading this yesterday: https://peerj.com/articles/cs-149.pdf | ||
> | ||
> Summary: | ||
> - They too, automatically generate Python bindings for C++ code using Boost::Python. | ||
> - The whole process is python based. | ||
> - Same design as to what we have in mind i.e., parse, process, and generate. | ||
> - Their data structure of choice for the whole process is Abstract Semantic Graph: https://github.com/StatisKit/AutoWIG/blob/master/src/py/autowig/asg.py | ||
> - The architecture is plugin-based, and somebody added a pybind plugin some months back: https://github.com/StatisKit/AutoWIG/blob/master/src/py/autowig/pybind11_generator.py | ||
> - They use libclang for frontend parsing(python API). The project was done some time back so they wrote their own py-libclang code: https://github.com/StatisKit/AutoWIG/blob/master/src/py/autowig/libclang_parser.py | ||
> - Repo: https://github.com/StatisKit/AutoWIG | ||
> | ||
> I think it can act as a good reference for the project. Have a look at the pdf and the source if you wish. | ||
> The libclang python part can be explored from their repo as of now (as python-libclang has no documentation whatsoever and the 1-2 example articles are outdated) | ||
- Problems: | ||
> Templates: | ||
> * suffixing doesn't work well. Unless you're a fan of the pseudo-Hungarian notation espoused by older devs (and by MS). It's ok for 1 (or maybe 2) arguments. | ||
> * Templates in Python represent an instantiation of C++ template. It should be easy to add/remove an instantiation without affecting needless code. It should also be visually easy to switch the template type without having to lookup the notation or count underscores | ||
> * Python has a strong syntax for this: index lookup via __getitem__ and __setitem__ | ||
> * Using strings as keys is bad because the editor can't help if spelling is wrong. Pandas MultiKey failed here. | ||
- Use of a templating engine for pybind11 code gen (jinja2>mako) | ||
|
||
#### [03-06-2020] | ||
- Ambiguity in the phrase: "full control over clang's AST" | ||
|
||
#### [02-06-2020] | ||
- Use of python bindings of libclang, for faster prototyping: https://github.com/llvm/llvm-project/blob/master/clang/bindings/python/clang/cindex.py | ||
- > protyping and exploring python bindings in which everything is runtime and can be done interactively would usually be my first approach | ||
|
||
#### [28-05-2020] | ||
- > While reading lsst's documentation, came to find out they use a __str__ method: | ||
> ```py | ||
> cls.def("__str__", [](Class const& self) { | ||
> std::ostringstream os; | ||
> os << self; | ||
> return os.str(); | ||
> }); | ||
> ``` | ||
- > the << operator with ostreams is the most common way in C++ of extracting a string representation of a given object (I have no idea why there's no practice of implementing the cast to string method), | ||
That being said I believe you can use << with std::stringstreams, effectively allowing you to fetch a string representation of PCL objects which have operator<< (std::ostream, ....) implemented. | ||
|
||
#### [15-05-2020] | ||
- > You can create docstring from \brief part and copy the function signature via libtooling. | ||
|
||
#### [09-05-2020] | ||
- Start with binding PointTypes. | ||
- AST parsing helps in cases of convoluted code. | ||
- > We can keep 2 approaches in parallel: | ||
> 1. header parser on a limited number of files | ||
> 2. libtooling to replace it | ||
> 1st will allow the pipeline to be developed | ||
> 2nd will replace that | ||
- > We can make a prototype which works on manually provided API points | ||
- > From my understanding: | ||
> 1. Code -> AST -> JSON: use some tool for it first, then replace with libtooling | ||
> 2. JSON -> cpp: Python tool, language dependent | ||
> 3. CMake + compile_database.json: rest of toolchain | ||
> 4. organize properly so usage in LANG is nice | ||
|
||
#### [05-05-2020] | ||
- > I'd put PyPi integration immediately after we get 1 module working. That'd allow us to keep shipping improved bindings after GSoC (in case the timeline gets delayed) | ||
The order in which modules are tackled should be the dependency order (because we don't have the popularity data from our users) | ||
|
||
*** | ||
|
||
## Javascript Bindings | ||
#### [05-05-2020] | ||
- Webassembly as an option: https://en.wikipedia.org/wiki/WebAssembly | ||
- Emscripten as an option: https://emscripten.org/ | ||
- > * Getting clang to compile to WebAsm will be the best "performance". | ||
> * Using Emscripten on the other hand is a well-travelled road, but the performance will be similar to hand written JS (or worse). | ||
> Both approaches need clang so that's a milestone we need to conquer first. | ||
|
||
*** | ||
|
||
## Jupyter Visualisation | ||
#### [05-05-2020] | ||
- > WebGL view straddles JS bindings and Python bindings. It should come before JS bindings in terms of priority keeping in mind the popularity of Python as a second language for the kind of C++ users PCL attracts (academia) | ||
- > https://jupyter.org/widgets has pythreejs which is embedding js in python. .... That's 1 step above webgl, involves using JS in Py, but is faster to get up and running.... We can tackle this based on time when we reach some stage |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
cmake_minimum_required(VERSION 3.5) | ||
project(bindings) | ||
|
||
# find_package(PCL REQUIRED) | ||
find_package(PCL COMPONENTS common REQUIRED) | ||
|
||
# We can replace `find_package` with `add_subdirectory`, depending on usage. | ||
# https://pybind11.readthedocs.io/en/stable/compiling.html#find-package-vs-add-subdirectory | ||
find_package(pybind11) | ||
|
||
pybind11_add_module(pcl ${CMAKE_CURRENT_SOURCE_DIR}/pybind11-gen/common/include/pcl/impl/point_types.cpp) | ||
|
||
target_link_libraries(pcl PRIVATE ${PCL_LIBRARIES}) | ||
# add_dependencies(pcl_demo some_other_target) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
## Python bindings for PCL | ||
|
||
### common-archive | ||
- `point_types.json`: contains JSON based meta of `point_types.h` and `point_types.hpp`. | ||
- `py_point_types.cpp`: file generated by `generate_bindings.py` taking meta info from `point_types.json`. | ||
- `union_test.cpp`: testing file for handling of unions. | ||
|
||
### generate_bindings.py | ||
- Converts JSON data to pybind11 C++ code. | ||
- Run: | ||
```py | ||
python3 generate_bindings.py <path/to/json> | ||
``` | ||
|
||
### CMakeLists.txt | ||
- Finding PCL and pybind11, then adding the python module(s) via `pybind11_add_module` (which is a wrapper over `add_library`). | ||
|
||
### setup.py | ||
- For using setuptools. Uses `CMakeLists.txt`. | ||
|
||
### libclang.py | ||
- Using python libclang to parse C++ code, for generation of metadata by static analysis. | ||
- Run: | ||
```py | ||
python3 libclang.py <path/to/file> | ||
``` | ||
|
||
### json/* | ||
- JSON output generated by `libclang.py` | ||
|
||
### pybind11/* | ||
- pybind11 C++ outupt generated by `generate_bindings.py` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import os | ||
import sys | ||
|
||
sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) | ||
|
||
import scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restrict to directories with python files