New Boost.Python interface

[rodrigob]

This branch is now feature complete, see bottom of the thread for updates/discussions.

Following the spirit of other repositories this is a pull request of an ongoing branch. The code is not ready for merge, but this pull request enables to have an ongoing discussion to guide the development.

The rationale for this new branch is explained in the readme.text, basically current python bindings rely in a broken tool (swig), has too much spagethi (see __init__.py) and not enough link to the C++ codebase.

The current status is:
a) Proof of concept of the approach is valided (see d),
b) 70% 80% 90% all of the ground work is in place (most notable missing pieces is the gpu API, Tuple, RDom),
c) Code compiles and runs,
d) blur.py runs and blurs the image as desired (erode.py, bilateral_grid.py also works).
e) No real unit-tests in place (see Q4).

On principle code should work perfectly fine on python 2 and 3; but I have only tested for python 3.

Some of feedback I would like to have:
Q1) Likelihood of a merge with master once API/tests coverage is good enough.

Q2) Suggested strategy to better address the drift between Python bindings and C++ code base (i.e. how to include in continuous integration).

Q3) Current code adds dependency with Boost.Numpy (which is not part of boost) for convenient I/O. I am considering including a copy into the repository, opinions on the cleanest way of handling the dependency?

Q4) Suggestions for the best testing approach. For now I will focus on porting the demonstration apps from the old python bindings, and covering the areas I will be using; but I guess we could do better.

Q5) Anyone interested on giving a hand ? Second brain helps make code cleaner. Also, I would not mind delegating the gpu API part (which I am not planning to use in the short term).

[abadams]

Sorry for delay, I was away at a conference:

1. High. Thanks a lot for this work.
2. I'm willing to add python tests to the nightlies on linux at least. Before I was unwilling because I didn't understand the swig bindings well enough to fix them.
3. What does this dependency buy you? Is numpy alone not sufficient? I'd rather not include it in the repo if it's easy to build and install via a git clone.
4. Porting the tutorials to python would have pretty good coverage for testing the syntax. Testing the internals is probably less important because the C++ tests do that.
5. I'm willing to keep it current with changes to Halide, but I can't volunteer to help finish it.

[rodrigob]

1. Great.

2. Fantastic. Yes one of the goals of this new binding is removing "dark magic" (or at last converting it into C++1x templates magic).

3. Right now it is used for ndarray_to_image and raw_buffer_to_image. I used it because that is what I am familiar with. Yes Boost.Numpy works out of the box.
   If time/motivation allows I might "take the minimal pieces that we need" instead of having one more dependency. But for now, it stays at it is (e.g. not included in the repo).

4. Sounds good to me. Porting the tutorial should be easy.

5. No problem. For my use case I need Tuple, so that will be done this week. The gpu API is quite small, so it should be not problem to finish that part up. Point (4) will make sure I have a decent coverage of the API.

I will keep posted on the progress.

[rodrigob]

Progress status:

• Normal text: to be done
• Italic: ported but not yet working properly
• strike through: done, works as expected

(will keep editing this message as things evolve)

apps/
blur.py
erode.py
bilateral_grid.py

tutorial/
lesson_01_basics.py
lesson_02_input_image.py
lesson_03_debugging_1.py
lesson_04_debugging_2.py
lesson_05_scheduling_1.py
lesson_06_realizing_over_shifted_domains.py
lesson_07_multi_stage_pipelines.py
lesson_08_scheduling_2.py
lesson_09_update_definitions.py
lesson_10_aot_compilation_generate.py
lesson_10_aot_compilation_run.py
lesson_11_cross_compilation.py
lesson_12_using_the_gpu.py

Known issue: python object reference counting is not properly handled in ndarray_to_image (e.g. if the input array is deleted, the Image object points to de-allocated memory).

[rodrigob]

Quick status update: all tutorials ported. Some of them pass, some do not.

Next steps: bugs ironing and covering the missing corners of the API.

[rodrigob]

Quick status update: The bulk of the API coverage is done (all bit and pieces used in the lessons, only left out areas I have never seen used, e.g. Stage API). All lessons are converted too.

Only two lessons and one demonstration application do not work as desired (either generate error, or do not pass the result correctness test). These three programs hit proper bugs that need to be investigated.

~~Other than that the other two known issues are:

1. ndarray_to_image is not handling python reference counting as it should (it works, but creates oportunities for segfault which could be avoided)
2. "42 + FuncRefVar" does not work ( "FuncRefVar + 42" works fine); need to double check if this solvable at all in python (from boost.python) or a intrinsic limitation of the language.~~

[rodrigob]

All python demonstration application and lessons now work as desired.
Only ndarray_to_image reference counting issue is remaining before this branch is ready for a review.

ndarray_to_image issue is not a show stopper, since code is fully usable already. But it creates chances to create "C++ style memory errors" that should not occur in Python world.

[rodrigob]

This branch is now feature complete.
It compiles, runs, covers previous python demonstration apps, and includes all tutorial lessons ported.

All the known issues are solved.
The API coverage is >90%, some pieces and bits are left "when someone needs them", they seem rare corner cases.

Most of the code is pretty straightforward, with some pieces of C++ magic (recursive calls over list types) to avoid too much copy&paste code. Stage and Func have some copy&paste code (between them), but so does the C++ codebase.

Code has been tested on Ubuntu 14.04 with python 3.4, and llvm 3.5.
Code should be tested at least once on different platform, and in particular with python 2.
Boost.python should work flawlessly across platforms and python versions, to be verified.

Please let me know if there is any feature/code change that I should do/consider.

[abadams]

I get stuck here:

/usr/local/google/home/abadams/projects/Halide-python/boost_python_bindings/python/Param.cpp:352:11: error: specialization of ‘template<class T> struct Halide::{anonymous}::type_of_helper’ in different namespace [-fpermissive]
 struct h::type_of_helper<end_of_recursion_t> {
           ^
In file included from /usr/local/google/home/abadams/projects/Halide-python/boost_python_bindings/python/../../src/Buffer.h:12:0,
                 from /usr/local/google/home/abadams/projects/Halide-python/boost_python_bindings/python/../../src/IR.h:11,
                 from /usr/local/google/home/abadams/projects/Halide-python/boost_python_bindings/python/../../src/Param.h:9,
                 from /usr/local/google/home/abadams/projects/Halide-python/boost_python_bindings/python/Param.cpp:7:
/usr/local/google/home/abadams/projects/Halide-python/boost_python_bindings/python/../../src/Type.h:158:8: error:   from definition of ‘template<class T> struct Halide::{anonymous}::type_of_helper’ [-fpermissive]
 struct type_of_helper;
        ^
make[2]: *** [CMakeFiles/halide.dir/python/Param.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/halide.dir/all] Error 2
make: *** [all] Error 2

Looks like the local type_of_helper specialization can't be in a different anonymous namespace to Halide's. Is there some simpler way to do this switch (e.g. by passing a list of Halide::Type instead of a variadic type pack)? If variadic templates are required then perhaps a wrapper of type_of in a local namespace is necessary.

[abadams]

Also, in the branch you should delete the old python_bindings and rename boost_python_bindings to python_bindings.

[rodrigob]

just for info (so I can try reproduce the issue) which compiler (and compiler options) are you using ?

[abadams]

gcc 4.8.2

Followed the instructions in the readme, so no special compiler flags. Let
me know if you want me to try anything else

On Mon, Jun 22, 2015 at 10:42 AM, Rodrigo Benenson <notifications@github.com

wrote:

just for info (so I can try reproduce the issue) which compiler (and
compiler options) are you using ?

—
Reply to this email directly or view it on GitHub
#821 (comment).

[rodrigob]

Okidoki. I can reproduce the issue indeed, clang-3.5 and gcc-4.8 do not agree on what is permitted or not. Param.cpp seems to be the only source of trouble (with make -i everything else compiles).

I did not like that much that piece of code anyway, will try a variant based on boost/mpl (which should provide robust support across compilers).

[rodrigob]

The last two pushes should fix the gcc 4.8 issue (while still working on clang 3.5) and replace the old swig python bindings with the new boost.python bindings.

[rodrigob]

I just noticed that I missed two additional python demonstration applications from the old swig version (somehow I though I had copied them all, I did not). Will add then as soon as possible. (done)

interpolate.py (Lambda.cpp API to be exposed)
local_laplacian.py

[abadams]

I'm having a hell of a time trying to get boost_numpy to compile for python3 instead of python2. Any hints?

[rodrigob]

Ahhh, for me it was a breeze (maybe I have done it too many times already...).
But this brings back the question of "do we want the extra dependency or not?".

For python3 you need to make sure cmake is happy.
The key in Boost.Numpy CmakeLists.txt

if(${PYTHON_VERSION_STRING} GREATER 3.0)
  message(STATUS "Using Python3")
  find_package(Boost COMPONENTS python-py34 REQUIRED)

that piece of code assumes you have python3.4 installed. Change to the adequate python 3 version if needed (and make sure it matches your boost installation).

Boost.Numpy also needs numpy installed.

That is for generic advice. For more help, can you point me to the specific error you are having ?

[rodrigob]

Quick status update:

• Finished adding the missing two applications (from old code base). All applications work and tutorial lessons pass.
• Found and fixed a couple of bugs, and some missing bits of the API (used in last applications).
• Updated docs, readme, created link to docs hosting site.

From my point of view the branch is feature complete and usable as is.
The only three items that would make sense to check before merge into master are:

1. At least try once with python2 (would be lovely if someone else tries, see point 2)
2. That someone else gives it at least once a trial (to make sure instructions are fine, that nothing is broken)
3. Some code review/feedback on the style?
   These are 11k lines of code; piece of cake to review, right 😄?

[rodrigob]

@abadams any help I can provide with boost.numpy ?

[rodrigob]

To make things easier I have imported the bits of Boost.Numpy we need into the repository (under python_bindings/numpy).

These are eight files total, summing up < 1k lines of code (and I could strip more, but I decided to strip files, not lines of code).

To let the user choose I changed the semantics of the cmake USE_BOOST_NUMPY option.

• If ON the Boost.Numpy is required and things will run as usual.
• If OFF only Numpy is required, and halide_numpy is compiled instead of boost_numpy.

Bottom line, now things should work well even if Boost.Numpy is not installed.
Numpy is a dependency, but the code is guarded under a USE_NUMPY definition (not yet exposed in cmake), so it is easy to disable. Albeit I think using numpy for I/O is the most convenient use case for pythonistas.

Let me know if python_bindings/numpy is fine or if I should remove it and stay with the boost.numpy dependency ( @abadams already tasted that dependencies can be a pain).

[rodrigob]

@abadams Anything that I can do to help the review/merge consideration process ?
(will be offline next week)

[abadams]

Apologies, I got caught up with other stuff over the last week and let this
slide. I still fully intend to merge it. I'll take another look tomorrow.

On Wed, Jul 1, 2015 at 8:55 PM, Rodrigo Benenson notifications@github.com
wrote:

@abadams https://github.com/abadams Anything that I can do to help the
review/merge consideration process ?
(will be offline next week)

—
Reply to this email directly or view it on GitHub
#821 (comment).

[abadams]

Here's my experience on OS X with macports:

First I install a bunch of relevant-seeming stuff:

sudo port install python34
sudo port select --set python python34
sudo port install boost +python34
sudo port install py34-numpy

Then I try to build

$ mkdir build
$ cd build
$ cmake ..
CMake Error at /opt/local/share/cmake-3.2/Modules/FindPackageHandleStandardArgs.cmake:138 (message):
  Could NOT find PythonLibs: Found unsuitable version "2.7.6", but required
  is at least "3.4" (found PYTHON_LIBRARY-NOTFOUND)
Call Stack (most recent call first):
  /opt/local/share/cmake-3.2/Modules/FindPackageHandleStandardArgs.cmake:372 (_FPHSA_FAILURE_MESSAGE)
  /opt/local/share/cmake-3.2/Modules/FindPythonLibs.cmake:205 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  CMakeLists.txt:16 (find_package)


-- Configuring incomplete, errors occurred!
See also "/Users/abadams/projects/Halide-rodrigob/python_bindings/build/CMakeFiles/CMakeOutput.log".

After much messing around, I'm still unable to get cmake to see my python3.4 installation. This is the same problem I had on linux. I'll proceed manually and record the list of changes I needed to make to get it to build on OS X:

change forward declaration of "class Type" to "struct Type" in Type.h

change reinterpret_cast<size_t>(that.dev) to just that.dev in Buffer.cpp (You can't reinterpret a uint64_t as a size_t - size_t might be 32-bit).

include boost/format.hpp before boost/python.hpp in Type.cpp (see http://stackoverflow.com/questions/29443497/cannot-include-both-boost-python-and-boost-any)

include boost/format.hpp before python.hpp in make_array.h

Seems like rtti is required for boost python to do its thing, but llvm is built without it, so Halide fails to build if I turn it off in the Halide makefile. At this point I'm stuck. Is rtti supposed to be on or off? I don't see any mention of it in the python_bindings folder.

Undefined symbols for architecture x86_64:
  "typeinfo for Halide::ImageParam", referenced from:
      boost::python::detail::signature_arity<1u>::impl<boost::mpl::vector2<Halide::Func, Halide::ImageParam> >::elements() in py_BoundaryConditions.o
      boost::python::converter::expected_pytype_for_arg<Halide::ImageParam>::get_pytype() in py_BoundaryConditions.o
      boost::python::detail::signature_arity<2u>::impl<boost::mpl::vector3<Halide::Func, Halide::ImageParam, Halide::Expr> >::elements() in py_BoundaryConditions.o
      ___cxx_global_var_init22 in py_BoundaryConditions.o
      defineImageParam() in py_Param.o
      boost::python::detail::signature_arity<1u>::impl<boost::mpl::vector2<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, Halide::ImageParam&> >::elements() in py_Param.o
      boost::python::converter::expected_pytype_for_arg<Halide::ImageParam&>::get_pytype() in py_Param.o
      ...
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [build/halide.so] Error 1

Here's the makefile I ended up with:

CCFLAGS=$(shell python-config --cflags) -I /opt/local/include -std=c++11
LDFLAGS=$(shell python-config --ldflags) -L /opt/local/lib -lboost_python3-mt -lz

PY_SRCS=$(shell ls python/*.cpp)
PY_OBJS=$(PY_SRCS:python/%.cpp=build/py_%.o)
NUMPY_SRCS=$(shell ls numpy/*.cpp)
NUMPY_OBJS=$(NUMPY_SRCS:numpy/%.cpp=build/numpy_%.o)

NUMPY_PATH=$(shell python -c "import numpy; print(numpy.__path__[0] + '/core/include')")

build/py_%.o: python/%.cpp
    $(CXX) $(CCFLAGS) -c $< -o $@

build/numpy_%.o: numpy/%.cpp
    $(CXX) $(CCFLAGS) -I $(NUMPY_PATH) -c $< -o $@

build/halide.so: $(PY_SRCS) $(PY_OBJS) $(NUMPY_SRCS) $(NUMPY_OBJS)
    $(CXX) $(PY_OBJS) $(NUMPY_OBJS) $(LDFLAGS) ../bin/libHalide.a -shared -o $@

[rodrigob]

Wow, that is quite a bit of pain indeed.
To double check some of the issues, which versions are you using ?
I see cmake-3.2, python3.4.
Which version of clang and boost are you using ?
I suspect the "boost format versus boost python" might be related to older boost versions (or newer compilers ?),

For the python experience it is quite a bit better if Halide raises exceptions (as mentioned in the python_bindings readme).
When Halide raises exception then python will raise an exception "as usual".
When Halide is compiled without exception, then any error on the python Halide code will cause python to quit (because Halide called "end of game"), which is unusual and undesirable (would break a python debugging session).

I understand C++ exceptions rely on RTTI.

You ask "is rtti supposed to be on or off?", why should it be off ? Given your error it clearly should be on, but why would you want it to be off ? (other than for leaner executables )

[rodrigob]

(by the way just noticed that Halide's Makefile does not work properly when WITH_EXCEPTIONS?= not-emtpy; had not noticed because I build it via cmake)

(the fix is to add -fno-exceptions in the omit part of the LLVM_CXX_FLAGS; I also added the -fexceptions by hand, not sure if was needed or not)

[abadams]

I'm not against rtti per-se, but my llvm was compiled with -fno-rtti (3.6 built with cmake with default settings), which is why it had to be off for the Halide build. If I remove -fno-rtti from the Halide Makefile I get the following error while building libHalide

Undefined symbols for architecture x86_64:
  "typeinfo for llvm::ModulePass", referenced from:
      typeinfo for (anonymous namespace)::WriteBitcodePass in BitcodeWriterPass.o
  "typeinfo for llvm::SectionMemoryManager", referenced from:
      typeinfo for Halide::Internal::(anonymous namespace)::HalideJITMemoryManager in JITModule.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [bin/libHalide.so] Error 1

I turned on Halide exceptions for the reasons you state. Based on some googling, I think you can have exceptions without rtti. My llvm-config flags didn't include -fno-exceptions, so WITH_EXCEPTIONS=1 worked for me.

Here are my versions:

$ c++ --version
Apple LLVM version 6.1.0 (clang-602.0.49) (based on LLVM 3.6.0svn)
Target: x86_64-apple-darwin14.3.0
Thread model: posix

$ cat /opt/local/include/boost/version.hpp | grep BOOST_LIB_VERSION
//  BOOST_LIB_VERSION must be defined to be the same as BOOST_VERSION
#define BOOST_LIB_VERSION "1_58"

I'll try building Halide with cmake to see if it ends up differently... no, same error about missing reference to typeinfo for ImageParam.

This page claims I can use boost without rtti (or more accurately, with its own internal rtti):
http://www.boost.org/doc/libs/1_58_0/libs/exception/doc/configuration_macros.html

However I still get the same error if I add those flags to my CCFLAGS. I guess boost-python really does need rtti.

what does "make VERBOSE=1" look like for you when compiling the python bindings with cmake? Maybe there are some clever flags I'm missing.

[rodrigob]

It looks like this

/usr/bin/c++   -Dhalide_EXPORTS -O2 -g -DNDEBUG -fPIC -I/home/rodrigob/code/references/Halide_master/python_bindings/../build/include -I/home/rodrigob/code/references/Halide_master/python_bindings/./numpy -I/usr/include/python3.4m -I/home/rodrigob/.local/lib/python3.4/site-packages/numpy/core/include    -std=c++11 -o CMakeFiles/halide.dir/python/IROperator.cpp.o -c /home/rodrigob/code/references/Halide_master/python_bindings/python/IROperator.cpp

the only options I spot are -Dhalide_EXPORTS -O2 -g -DNDEBUG -fPIC -std=c++11 (compiled on RelWithDebInfo), nothing fancy.

[rodrigob]

My boost version is 1.55, so the "format.hpp before python.hpp" might be compiler specific.

[rodrigob]

Fixed the merge conflict. I think this version should be considered for merging into Master.

All tutorials and apps run.
The only missing features considered are:

1. A way to install the created python library. But I think this is best served as a latter pull request.
2. define_extern API is present, but not clear "how to make it work". Again, I think this can be addressed in a latter pull request.

[abadams]

Looks like some debugging code left in.

[abadams]

Never mind, I want to merge this now, so I'll just cut this line myself during the merge.

[abadams]

Merged. Thanks for all this work - I owe you a beer. I've included it in the nightly tests too.

[rodrigob]

Yey ! Well thank you for Halide.

I am happy now that I can do more/faster Halide hacking from Python directly.

[rodrigob]

Issues #888 and #887 opened as follow-up for the features yet to be created.

[samueljohn]

❤️ @rodrigob and @abadams. I followed this in silence. Congrats to this immense effort. I hope to use this in the future. 👍 for going Python 3 first.