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

Halide pip package for python bindings #6815

Closed
wants to merge 25 commits into from
Closed

Halide pip package for python bindings #6815

wants to merge 25 commits into from

Conversation

lukastruemper
Copy link
Contributor

@lukastruemper lukastruemper commented Jun 24, 2022

@alexreinking Ahh, I had a way too long day. Cleaned up some repos after work and deleted the halide fork with original PR #6794

@steven-johnson
Copy link
Contributor

As a meta-comment here: we will (eventually) need to either augment the Makefile to do this as well, or (preferably) nuke Make support for the Python bindings. (I already did the latter once but it got restored). The build rules for (waves hands, looks around) are just way too complex to have to maintain in tandem unless there is a compelling need for them, and I argue that there isn't.

@steven-johnson
Copy link
Contributor

(Also: HAPPYDANCE, I will be so happy to be able to pip install Halide, but I hope someone will add guidance as to how to use/test this locally -- I presume that pip has a way to 'install' from a local source rather than a canonical repository? -- but adding information to the readme would be super-welcome for obvious reasons)

@alexreinking
Copy link
Member

I will need to make some edits to this that are easier to just do than to explain. Once I make those changes, I'll have you review the PR with my implicit sign-off @steven-johnson

@alexreinking
Copy link
Member

As a meta-comment here: we will (eventually) need to either augment the Makefile to do this as well, or (preferably) nuke Make support for the Python bindings. (I already did the latter once but it got restored). The build rules for (waves hands, looks around) are just way too complex to have to maintain in tandem unless there is a compelling need for them, and I argue that there isn't.

I buy this argument and favor dropping Make support for this. I have no inclination to maintain the Makefiles for building the Python package. Does anyone else on the team?

@lukastruemper
Copy link
Contributor Author

I will need to make some edits to this that are easier to just do than to explain. Once I make those changes, I'll have you review the PR with my implicit sign-off @steven-johnson

Oh wow, I didn't know what I started by adding a setup.py haha :-D

@alexreinking
Copy link
Member

Oh wow, I didn't know what I started by adding a setup.py haha :-D

We really appreciate it! You definitely got us over the activation-energy hump to do this.

@alexreinking alexreinking marked this pull request as draft June 28, 2022 17:44
@halide halide deleted a comment from lukastruemper Jun 28, 2022
@alexreinking
Copy link
Member

alexreinking commented Jun 29, 2022

Still trying to figure out how best to restore the Python tests/stubs now that there's an actual Python module in the way. I can think of a few options:

  1. Change the test code to run directly against the native halide_ module.
  2. Run the tests in tox, which will fully build the module and run in an isolated environment.
  3. Find some way to recreate the final package structure in the CMake build tree (easier said than done).

(1) is more expedient and (2) is more robust. However (1) plays nicer with CTest/existing infrastructure and the best (2) can do is be called from a shim in CTest. Improperly implemented, (2) might also duplicate work. On the other hand, (3) would be a good compromise, but I've had a couple false starts thanks to multi-configuration generators. Might need to wait until after I move into my new house on Friday.

We probably also want (2) to be possible anyway.

@steven-johnson
Copy link
Contributor

Just touching base here, roughly how much more work is needed for this?

@alexreinking
Copy link
Member

Closing in favor of #6886 since main has diverged substantially

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.

None yet

4 participants