Skip to content

Add a Python wheel build for the imath module (#432) - #596

Open
mshooter-ilm wants to merge 4 commits into
AcademySoftwareFoundation:mainfrom
mshooter-ilm:wheels-432
Open

mshooter-ilm wants to merge 4 commits into
AcademySoftwareFoundation:mainfrom
mshooter-ilm:wheels-432

Conversation

@mshooter-ilm

Copy link
Copy Markdown
Contributor

Adds a cibuildwheel workflow and pyproject.toml for the Boost.Python
imath module, modeled on OpenEXR's. Boost.Python is downloaded and built
from source inside the CMake build (IMATH_FORCE_INTERNAL_BOOST, pinned to
boost-1.92.0), so the wheel is a single self-contained module with no
dependencies. Scope as discussed with @cary-ilm: imath only, static; no
imathnumpy, no publish workflows.

Green on the full OpenEXR matrix (Linux x64/arm64, macOS x86_64/arm64/universal2,
Windows x64/arm64; CPython 3.9–3.13):
https://github.com/mshooter-ilm/Imath/actions/runs/34525829207

Notes for review:

  • New CMake options: IMATH_FORCE_INTERNAL_BOOST, IMATH_BOOST_URL /
    IMATH_BOOST_URL_HASH, PYIMATHNUMPY. Defaults leave normal builds
    unchanged; CI and CI-vfx pass on this branch.
  • pyImathTest.py: testQuatArrays uses a small tolerance on arm64-type
    architectures (issue testQuatArrays tests fail on some arches #515), exact elsewhere. Should probably be separate PRs
    and submit the tolerance first, and then the wheel.
  • Follow-ups, not here: publish workflows, and the PyPI name imath is
    already taken by an unrelated package.

In collaboration with Claude AI

…SoftwareFoundation#515); /bigobj for imath_module on MSVC

Signed-off-by: Moira Shooter <mshooter@ilm.com>
@cary-ilm

Copy link
Copy Markdown
Member

This looks great, thanks! The FetchContent looks like a cleaner approach than what I was imagining. Although it adds some complexity directly to the cmake configuration rather than the python wheel CI, that could be generally helpful, since it means anyone can build PyImath without needing to first install Boost.Python.

This does introduce a lot of long CI jobs, since Boost.Python gets built for each combination of arch and python version. I did a little investigating, and I think it should be possible to introduce a cache using  actions/cache  keyed on OS/arch + the pinned Boost version, so the tarball is downloaded/extracted once and reused across the CPython builds. Can you try asking Claude to explore this approach? If it works, great, but if not, the current setup is fine.

Comment thread src/python/PyImathTest/pyImathTest.py Outdated
def equalWithAbsErrorScalar(x1, x2, e):
return abs(x1 - x2) < e

# Architectures on which array quaternion operations and the equivalent

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix for the test would be better in an independent PR. It's necessary for the wheel build/test to succeed, but it's a separate issue, so it would be better tracked separately.

Could you move this change to a new branch and submit it as a separate PR?

Also, I would recommend switching the comparison to the fuzzy variety for all platforms, not just the ones that currently fail. After studying it in a bit more depth, I think it's actually a bit arbitrary that the existing bit-exact comparisons happen to work given what's going on (vectorized vs. non-vectorized computations). Just eliminate the platform test in quatEqual.

So:

  1. copy the changes to pyImathTest.py to a separate brance/commit, eliminate the platform conditional, and submit as a separate PR.
  2. revert the changes to pyImathTest.py on this branch. That will mean the CI test for this PR will fail, but we'll merge the other PR, then update this branch, and effectively be back where we are now.

Thanks!

@mshooter

Copy link
Copy Markdown

This looks great, thanks! The FetchContent looks like a cleaner approach than what I was imagining. Although it adds some complexity directly to the cmake configuration rather than the python wheel CI, that could be generally helpful, since it means anyone can build PyImath without needing to first install Boost.Python.

This does introduce a lot of long CI jobs, since Boost.Python gets built for each combination of arch and python version. I did a little investigating, and I think it should be possible to introduce a cache using  actions/cache  keyed on OS/arch + the pinned Boost version, so the tarball is downloaded/extracted once and reused across the CPython builds. Can you try asking Claude to explore this approach? If it works, great, but if not, the current setup is fine.

Hi @cary-ilm , thanks for the review.

  1. Test change: done as you asked. It's now its own PR, pyImathTest: compare array vs scalar quaternion results with a tolerance (#515) #597, with the tolerance on all platforms. I reverted it on the wheels branch, so the arm64 job on Add a Python wheel build for the imath module (#432) #596 will fail until pyImathTest: compare array vs scalar quaternion results with a tolerance (#515) #597 merges.

  2. Build time: I measured where the ~13 minutes per wheel go. Downloading Boost is about 10 seconds. Compiling Boost is about 2 minutes. The other 10 minutes are compiling PyImath itself, and that has to happen once per Python version. So a Boost cache can only save a minute or two per job. I'm happy to add it anyway if you'd like.

What would actually make the job short: build the five Python versions in parallel, as separate CI jobs, instead of one after the other on the same machine. Same total work, but the run finishes in about 15 minutes instead of an hour. It's a workflow-only change; the cost is 30 small jobs instead of 6 big ones in the checks list.

On top of that, a compile cache (sccache) can make re-runs almost free when the code hasn't changed: I tested it locally and a rebuild went from 222 seconds to 4. That needs a bit more plumbing, so I'd do it as a follow-up PR.

Which would you like: the parallel jobs in this PR, the Boost cache, both, or leave it as is?

@cary-ilm

Copy link
Copy Markdown
Member

Thanks! I merged the test pr, so you can rebase the branch on that and pick up the change.

Thanks also for the speed analysis. I had the impression that Boost took longer to build, so that's good to know.

Let's split it out as 30 jobs, one per pair of architecture/python version. And I don't think the cache is worth it. Imath doesn't change very often, so the run time is not actually all that critical.

@cary-ilm

Copy link
Copy Markdown
Member

And one more thing: you should copy over the python-wheels-publish.yml and python-wheels-publish-test.yml from OpenEXR. Those are the workflows that run when a release candidate is tagged and when a release is published, which actually upload the wheels to PyPI and PyPI.test. Those are hard to test, since you have to fake a release. I'll handle that. There may be some additional setup with the Imath PyPI project. I'll look into that as well. But the first step is to replicate for Imath exactly what OpenEXR does. Thanks!

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.

3 participants