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

Add python packaging support #1377

Conversation

peytondmurray
Copy link

@peytondmurray peytondmurray commented May 11, 2022

Summary

This PR adds python packaging support, allowing the python OpenVDB bindings to be installed with pip. It also includes github actions to automatically publish releases to the Python Package Index (PyPI). See #161 for original discussion.

Changes

  • Added pyproject.toml, which specifies build-time dependencies for the python package.
  • Added setup.py, which makes pyopenvdb installable by pip, and specifies the python side of the build recipe.
  • Added a new github action to build a source distribution of the pyopenvdb package, and upload it to PyPI. The build can be triggered either by publishing a new OpenVDB release, or manually by navigating on GitHub to Actions->Build and Release Python Bindings to PyPI->Run Workflow and providing a branch and ref to build.

Additional Info

I explored a couple of options for generating binary python package distributions (wheels), but eventually opted only to generate source distributions (sdists). Here's what I tried:

  1. cibuildwheel, a project by the Python Packaging Authority, allows a user to easily build wheels for python packages for a range of python versions, platforms, and system architectures. It does this by using docker containers to provide build environments. These manylinux docker containers can generate wheels for a wide group of linux distributions, but in opting for widespread compatibility they sacrifice being up to date:
  • We can't easily use manylinux2014 because it's based on CentOS 7, which only has boost 1.53 available in the official repositories, so we'd have to build it from scratch
  • We can't easily use manylinux_2_24 because it's based on Debian 9, and uses GCC 6.3 which can't build our dependencies.

I eventually gave up on cibuildwheel to explore other options.
2. Using ubuntu:22.04 to build wheels for Python 3.8, 3.9, and 3.10 seems like a good idea, and in fact I think it should in principle work - except we still need to build OpenVDB from source, which takes long enough on the github actions runners that the job times out.

So for this reason that I think it is best just to release an sdist to PyPI, which means that when a user installs the package with pip install pyopenvdb, pip will trigger a build of the shared library and copy it to the appropriate site-packages. sdists are commonly uploaded as "fallback" distributions on PyPI anyway, because they should be compatible with any system that has the appropriate dependencies, and are compiled at install time rather than at the time the package is published.

Update 2023-02-03

I've reworked this PR so that it now works with the new pybind11 bindings.

Changes

  • Added a workflow to build and publish an sdist package to either test.pypi.org (if a workflow_dispatch event is used ad the appropriate checkbox is clicked) or by default to pypi.org, the main python package index. Publishing requires the presence of a github secret with a PYPI token, named PYPI_API_TOKEN. If publishing to test.pypi.org, the secret that is used should be called TEST_PYPI_API_TOKEN I can meet to help out with this if the process is unclear. When a new release is made, this workflow will automatically build and publish the release to PYPI.
  • At the moment, there's no way to dynamically get the version with the build backend we are using; see Dynamic versioning scikit-build/scikit-build-core#116 for more info. So whoever does the release will have to update the version number in pyproject.toml before publishing :/ hopefully this feature gets added to the build backend soon.
  • The python bindings will be published under the openvdb package name, as discussed before. To install, you'd just do pip install openvdb and to use them you'd do import openvdb.

Todo

  • CLA
  • Sign-off the commits
  • Document requirements around using the github action to publish to PyPI (need to set up a PyPI token as a secret on the repository. OpenVDB maintainers should have access to the secrets on my fork, so you can see how to set it up.)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 11, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@Idclip
Copy link
Contributor

Idclip commented May 12, 2022

Hey @peytondmurray thanks a lot for your work on this, this is looking great! I need to spend a bit more time looking at what you have here but here's some initial comments:

  • Interesting that the cibuildwheel approach is locked to those older versions, though I see the reasons behind it. Out of interest what dependencies failed to build with GCC 6.3 with the manylinux_2_24 container?
  • Regrading the CI timeouts for ubuntu:22.04 that you were experiencing; we should be able to solve this. You can try turning -D USE_EXPLICIT_INSTANTIATION=OFF as this isn't really needed for the python bindings. Additionally, our actions support the use of a shared ccache to speed up subsequent builds. IIRC each job has a 6hr timeout limit? If we had a build matrix of a wheel per job (if such a workflow make sense with the wheel approach), combined with the above, I'd very much hope we wouldn't hit timeout limits!

I do like the idea of the sdist approach, though the idea of deploying binaries was pretty appealing to reduce the barrier to entry w.r.t build issues. I'm not sure if this is still enough of a reason to not go the sdist approach. Are their genral guidelines to choose one or the other? Do most pypi packages support both types of installations?

Finally we can absolutely credit Alex for their work! In case I haven't mentioned, the TSC have weekly catch-ups every Tuesday (10:00am to 11:00am (UTC-07:00)). They're minuted, but informal and open to anyone should you or Alex wish to attend and discuss anything here. Reminders are posted here: https://lists.aswf.io/g/openvdb-dev/topics. Either way I will feedback your progress, thanks again!

@peytondmurray
Copy link
Author

@Idclip Thanks for the info - I think based on your feedback I'll try once more to build wheels using ubuntu:22.04, knowing that it may lower the barrier to entry for python users.

Out of interest what dependencies failed to build with GCC 6.3 with the manylinux_2_24 container?

OpenVDB itself depends on GCC>=6.3.1.

Are their general guidelines to choose one or the other? Do most pypi packages support both types of installations?

Most large PyPI projects publish both wheels and sdists; the wheels are faster and smaller to install, but the sdists have better compatibility because the compilation step happens at install time. pip has flags for letting users choose whether to install sdists (--no-binary) or bdists (--only-binary) if they have a preference.

In case I haven't mentioned, the TSC have weekly catch-ups every Tuesday (10:00am to 11:00am (UTC-07:00)). They're minuted, but informal and open to anyone should you or Alex wish to attend and discuss anything here.

Hey, thanks for the invite! I saw that these meetings were on the calendar, but wasn't sure whether anyone would have questions about all this if I showed up. I'm traveling next week, but I think I might be able to make the meeting anyway. Even if nobody has any questions about this packaging stuff I'll come and say hello.

@peytondmurray peytondmurray force-pushed the python-packaging branch 11 times, most recently from c232888 to fed266e Compare May 13, 2022 06:24
@peytondmurray
Copy link
Author

peytondmurray commented May 13, 2022

Okay, so I spent some time fiddling about getting wheels to build on ubuntu:22.04, and I'm getting close but I'm still not exactly sure how long it could take. There are still some difficulties getting cmake to find dependencies 🙃 so if sdists are acceptable for now, I would like to break off support for building wheels into a separate branch to be added in a later PR. For now, I'll mark this as ready for review.

Looks like the DCO workflow has failed, but I've done git commit -s on the only commit on this branch. What am I missing?

@peytondmurray peytondmurray marked this pull request as ready for review May 13, 2022 06:28
@peytondmurray peytondmurray changed the title [WIP] Add python packaging support Add python packaging support May 13, 2022
@diiigle
Copy link

diiigle commented Jun 20, 2022

Looks like the DCO workflow has failed, but I've done git commit -s on the only commit on this branch. What am I missing?

Just glancing through the error message (at the bottom):

Expected "pdmurray peynmurray@gmail.com", but got "Peyton Murray peynmurray@gmail.com".

Great work you're doing there, pip support is a much awaited relief to the (in the past) tedious installation process of openvdb and especially pyopenvdb with its boost dependencies.

@peytondmurray
Copy link
Author

Ahh 😅 sorry! @Idclip Would it be possible to retrigger CI?

.github/workflows/pypi_release.yml Outdated Show resolved Hide resolved
openvdb/openvdb/python/pyproject.toml Outdated Show resolved Hide resolved
openvdb/openvdb/python/pyproject.toml Outdated Show resolved Hide resolved
Signed-off-by: pdmurray <peynmurray@gmail.com>
Signed-off-by: pdmurray <peynmurray@gmail.com>
Signed-off-by: pdmurray <peynmurray@gmail.com>
Signed-off-by: pdmurray <peynmurray@gmail.com>
.github/workflows/wheels.yml Show resolved Hide resolved
openvdb/openvdb/python/CMakeLists.txt Outdated Show resolved Hide resolved
openvdb/openvdb/python/pyproject.toml Show resolved Hide resolved
.github/workflows/pypi_release.yml Outdated Show resolved Hide resolved
@jdumas
Copy link
Contributor

jdumas commented Jul 21, 2023

@peytondmurray have you beem able to address @JeanChristopheMorinPerso's lastest comments? We still need a maintainer approval to run the github action workflows for this PR.

Signed-off-by: pdmurray <peynmurray@gmail.com>
@JeanChristopheMorinPerso
Copy link
Member

@AcademySoftwareFoundation/openvdb

Signed-off-by: pdmurray <peynmurray@gmail.com>
@peytondmurray
Copy link
Author

@jdumas Yep, I think the comments above are addressed.

@chrisflesher
Copy link

@Idclip Can you re-trigger CI please?

@Idclip
Copy link
Contributor

Idclip commented Aug 23, 2023

@Idclip Can you re-trigger CI please?

Worflow flie seems to be invalid: https://github.com/AcademySoftwareFoundation/openvdb/actions/runs/5639306194/workflow

@jdumas
Copy link
Contributor

jdumas commented Sep 12, 2023

It looks like the github action file is missing a jobs: section before the publish: job.

@jdumas
Copy link
Contributor

jdumas commented Sep 22, 2023

@peytondmurray sorry to ping you again, but would you be able to update your github action file to correct the syntax by adding the jobs: section?

@peytondmurray
Copy link
Author

Yep! Sorry, the last notification got buried, I'll do it now!

Signed-off-by: pdmurray <peynmurray@gmail.com>
@jdumas
Copy link
Contributor

jdumas commented Sep 22, 2023

Argh it looks like it needs approval again from a maintainer to run :/. Hope it works this time.

@peytondmurray
Copy link
Author

Yeah, I wish I'd made a smaller/easier to merge PR before submitting this one so we didn't have to wait for the workflow checks - I think they're only guarded against first time contributors. Thanks for your patience!

@jdumas
Copy link
Contributor

jdumas commented Sep 22, 2023

I'm not sure that it needs to be approved again for every commit of the same PR though, that seems a bit too much.

@Idclip
Copy link
Contributor

Idclip commented Sep 23, 2023

I'm not sure that it needs to be approved again for every commit of the same PR though, that seems a bit too much.

Not sure we can change this (?) but I'll try to remember to press the button when I see notifications

@peytondmurray
Copy link
Author

Hey folks, it's been a while since I updated this PR. At this point I'm open to PRs against this branch, or if someone wants to take over pushing this forward. At the moment I've been preoccupied with other work, and don't have much time to press this forward much more. What's the best way to tie this up? I'd be happy to do something here if it's small, but otherwise would it be best to close it out? I'm not sure how much need there is for this right now.

I'd also like to point out that meson-python has been really useful in a couple of projects that I've worked on in building compiled python extensions; it's much easier to deal with than cmake IMO. It might be worth looking into if fiddling with cmake is a challenge here.

@JeanChristopheMorinPerso
Copy link
Member

I don't think cmake is causing any unusual troubles here. Moving to another build system would be a huge task.

@jdumas
Copy link
Contributor

jdumas commented Dec 9, 2023

@peytondmurray could you give me write access to your fork? I don't have a lot of time right now, but I'd be happy to contribute when I can. One thing that makes it slow to iterate on this PR is that each commit requires approval from a maintainer before running GitHub actions. However, I think it only does that for first-time contributors, and I already have existing contributions to this repo, so it's possible that any of my commit triggers GitHub action right away (haven't been able to test that on @peytondmurray's branch since I don't have write access to it).

@peytondmurray
Copy link
Author

Invite sent!

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.

7 participants