WIP: Added setup.py for python_bindings #6794
Conversation
| if (WITH_PYTHON_BINDINGS) | ||
| if (Halide_ENABLE_RTTI AND Halide_ENABLE_EXCEPTIONS) | ||
| message(STATUS "Building Python bindings enabled") | ||
| add_subdirectory(python_bindings) |
There was a problem hiding this comment.
PR's description could use some more words.
Why is this an improvement?
There was a problem hiding this comment.
Sorry, wasn't meant to be shared yet. Converted to draft.
Testing something and needed a PR that I can reference in requirements.txt in a different project
There was a problem hiding this comment.
Probably removed more than necessary
There was a problem hiding this comment.
This is a pretty clear improvement to me. We just need to make sure that the rtti/exceptions variables are exported by our package (I think they are already)
There was a problem hiding this comment.
For future reference, as long as the python bindings remain buildable
the way they are built right now (as part of the upper halide build),
i'm indifferent to the changes.
There was a problem hiding this comment.
@LebedevRI - yeah, I have a comment about that elsewhere
lukastruemper
changed the title
|
@alexreinking is the best person to comment on this |
Cool, I added some description of what I am struggling with and why I added a setup.py. You guys probably know some better solution. |
|
I would love to review this, but movers come on Friday and my workstation has been packed already. I should be settled into Somerville next Monday. Can this wait until then? |
Of course, not pressing |
There was a problem hiding this comment.
I think this is a clear improvement and, honestly, I've been meaning to do it for a while. I have outlined a few changes in the comments that I think need to be done to avoid breaking existing workflows. There are also some safety checks that need to be restored.
I also really do want to see this using scikit-build. It's meant to be "glue between setuptools and CMake" so should be able to replace a lot of the bespoke build code here.
| if (WITH_PYTHON_BINDINGS) | ||
| if (Halide_ENABLE_RTTI AND Halide_ENABLE_EXCEPTIONS) | ||
| message(STATUS "Building Python bindings enabled") | ||
| add_subdirectory(python_bindings) |
There was a problem hiding this comment.
This is a pretty clear improvement to me. We just need to make sure that the rtti/exceptions variables are exported by our package (I think they are already)
| set(CMAKE_CXX_EXTENSIONS OFF) | ||
|
|
||
| #SET(HALIDE_MIN_VERSION "14.0.0") | ||
| find_package(Halide REQUIRED) |
There was a problem hiding this comment.
Need to check that exceptions / RTTI are enabled.
There was a problem hiding this comment.
Also need to disable this find_package call when it is included by the parent project
The best way to do this is probably to create a ./cmake/FindHalide.cmake module that just prints something like "${PROJECT_NAME}: Using in-tree Halide". The ./cmake folder is already added to CMAKE_MODULE_PATH, so it should take precedence over any installed Halide instances in the users' system.
|
@alexreinking Sorry for the delay, I had to fix some other problems first :D I think scitkit build simplified a lot. I pushed a minimum draft, but I don't have the time to fix the missing parts (I also don't understand the stub-thing fully). Maybe you can re-introduce the parts that I removed like "correctness", "tutorials", etc.?
|
|
FWIW, i still don't particularly understand how packaging just the python bindings So unless the latter is also bundled, the python bindings at the mercy of the distro's package, |
That is correct, but I think this would work if you install halide-python through https://github.com/conda-forge/halide-python-feedstock. Currently, the problem is that this does not install a proper python package (and is very outdated it seems). This should be fixed then, shouldn't it? |
|
It *should* be the case that our python extension statically links LLVM and
libHalide, and strips all exported symbols except for the Python entry
point(s), to avoid issues of this sort ... I thought I had both Make and
CMakr builds set up this way. If not, we should fix it. (Can't easily check
right now, responding via email)
…On Fri, Jun 24, 2022, 7:07 PM Lukas Trümper ***@***.***> wrote:
FWIW, i still don't particularly understand how packaging *just* the
python bindings in whatever native python way is, actually helps, because
they still depend on the actual halide lib, which depends on an actual llvm
lib/etc.
So unless the latter is also bundled, the python bindings at the mercy of
the distro's package, and by that point one might as well tell distro to
also package the bindings...
That is correct, but I think this would work if you install halide-python
through https://github.com/conda-forge/halide-python-feedstock.
Currently, the problem is that this does not install a proper python
package (and is very outdated it seems). This should be fixed then,
shouldn't it?
—
Reply to this email directly, view it on GitHub
<#6794 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACQJ65Y34D3NB5ESV34E43VQX2NVANCNFSM5XRTNDOQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
@LebedevRI can correct me if I'm wrong but, most likely, Debian and other centralized package managers will want LLVM and Halide to be shared libraries.
Yeah, the Python module should export only the entry points, regardless. |
Right. Though much like you can configure halide to statically link the llvm, But yes, static linking is, uh, not always the only/right solution. |
I think that already happens when you set |
I don't disagree, but in this case I'd personally argue that even though LLVM is pretty large by library standards, the potential dependency mess doesn't pay for itself, given the tight coupling between Halide and LLVM and the relative fragility of a C++ ABI dependency. That said, I guess we have to play by the rules we are given, so if that's a requirement, we have to follow along. |
|
@steven-johnson -- In the context of a curated package archive, like the Debian repository, these aren't really issues. There is one true LLVM for the one true Halide linked to it and its set of separately installable Python bindings. We should give package maintainers the flexibility to produce shared libraries when that makes sense, as opposed to making the link-type decision for them as an upstream. Obviously, to create a PIP-distributable manylinux-compatible module, we will have to statically link everything. |
4 participants
Problem: Using the python_bindings in another python project isn't very user-friendly right now.
So I tried some fix in this PR and added a setup.py which creates a package with the .so included (building against an existing Halide installation, note the find_package(Halide) in this CMakeLists.txt). I think it would be a good idea to decouple the build systems (second commit) and have a separate CI to run the tests with pytest and publish to pypi with some proper versioning (in a separate repo maybe?).
And have you discussed putting the "original" Halide part on conda? This would simplify the whole thing even more. I currently have a script where I call the AOT compilation from a python script (the "g++ ... " command) and then dynamically import the resulting .so in the same script. If I could depend on a conda package for Halide, this part would also be a bit more user-friendly as I can be sure that the library is at a specific location in my conda env. But this is a bit off-topic.