OpenImageIO icon indicating copy to clipboard operation
OpenImageIO copied to clipboard

build: Add support for `pip install OpenImageIO`

Open aclark4life opened this issue 2 years ago • 31 comments
trafficstars

Description

Adds support forpip install OpenImageIO requested in #3249

Tests

Checklist:

  • [x] I have read the contribution guidelines.
  • [ ] I have updated the documentation, if applicable.
  • [ ] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • [ ] If I added or modified a C++ API call, I have also amended the corresponding Python bindings (and if altering ImageBufAlgo functions, also exposed the new functionality as oiiotool options).
  • [ ] My code follows the prevailing code style of this project. If I haven't already run clang-format before submitting, I definitely will look at the CI test that runs clang-format and fix anything that it highlights as being nonconforming.

aclark4life avatar Oct 11 '23 17:10 aclark4life

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: aclark4life / name: Jeffrey A. Clark (3e80b3acad69e00ecfe199f4e7d90790bd0d4883, 66c39c4de7e4cb6dc143b8f38e46f0ce8f4d9a50, 4d7b0da17fe3f682d45e5fbe0e419c17dee749d4, ac74a20e8cbb3635c9f45f9139c34d69a78cb5a1, 1500b40efa09be1fcb2c8af0a36ab75bc3969b32, 6ee58aa24cd6a59878384eaa9ce091692e18c2a2)
  • :white_check_mark: login: JeanChristopheMorinPerso / name: Jean-Christophe Morin (8ea405ad4ffa55b77d1d9e126f38324789d6612f)

Hi @jessey-git 👋, I'm the Jean-Christophe mentioned in the description. I volunteered to help with https://github.com/AcademySoftwareFoundation/OpenImageIO/issues/3249, mainly to review PRs, but I'll be happy to help in other things ways too if there is a need. Obviously, that is if you want me to review PRs.

@JeanChristopheMorinPerso Hi! I did eventually find you within the ASWF umbrella after searching and you're of course more than welcome to help out in whatever capacity you'd like :) And ah! The description was missing a link back to the issue which would have made things a lot more clear where this was coming from.

jessey-git avatar Oct 11 '23 20:10 jessey-git

@jessey-git Sorry, this is a WIP I created to get the CLA signed before tomorrow!

aclark4life avatar Oct 11 '23 22:10 aclark4life

@JeanChristopheMorinPerso Can you please identify which of the commits listed here are most important to cherry-pick? For example, we don't need to build with setup.py so not going to cherry-pick that one. Thanks

aclark4life avatar Oct 11 '23 22:10 aclark4life

Happy sprint day 1 folks!

Please correct me if any of these assumptions are wrong:

  • VFX industry uses OpenImageIO as part of the VFX Platform and ASWF supports development of the VFX Platform
  • There's currently no formal binary distribution of OpenImageIO from ASWF, thus leaving the community with ad hoc attempts to provide them e.g.:
    • https://github.com/MethanePowered/OpenImageIOBinary
    • https://www.patreon.com/posts/openimageio-oiio-53939451
  • DCCs may include binary distributions of OpenImageIO thus lowering the burden of installation for VFX industry
    • Binary distributions may include the oiiotool command line tool as well as "much of its functionality" available via Python bindings
    • Python wheels would further reduce OpenImageIO installation burden by making its Python API available via pip install OpenImageIO

Most importantly, if I understand correctly, the oiiotool and related runtime libraries are not needed for Python API usage when OpenImageIO is installed via pip install OpenImageIO because all of the dependencies will be included in the wheel.

aclark4life avatar Oct 12 '23 15:10 aclark4life

VFX industry uses OpenImageIO as part of the VFX Platform and ASWF supports development of the VFX Platform

OIIO is not in the VFX Platform. ASWF is not the originator of the VFX Platform. But the VFX Platform largely specifies an important subset of the permutations of versions that we (OIIO and the other ASWF projects) definitely need to be able to build against.

There's currently no formal binary distribution of OpenImageIO from ASWF, thus leaving the community with ad hoc attempts to provide them e.g.:

Correct.

DCCs may include binary distributions of OpenImageIO thus lowering the burden of installation for VFX industry

Many DCCs and other applications use OIIO internally. The OIIO components are thus generally compiled in statically, or put in libraries with custom symbol namespaces so as to not interfere with any other OIIO used in the environment. I'm not aware of any DCC that ships OIIO in a form that's exposed to and useful to the users of the DCC.

Python wheels would further reduce OpenImageIO installation burden

Yes.

Most importantly, if I understand correctly, the oiiotool and related runtime libraries are not needed for Python API usage when OpenImageIO is installed via pip install OpenImageIO because all of the dependencies will be included in the wheel.

I would disagree with this. I think there is one set of users who are only after the Python APIs and thus view OIIO like any other Python library that they want to import. But a second set of users merely want "pip install openimageio" as the simplest and least painful way to get a full binary install of the entire package and its dependencies. Those people want python, C++, oiiotool, the whole schmear. Also, I think that somebody who starts out only wanting/needing the python interface could easily find themselves in a position of later needing oiiotool, and I wouldn't want them to suddenly be stuck without any way to get it without slogging through a full set of builds from source.

lgritz avatar Oct 12 '23 18:10 lgritz

A crucial thing to understand about VFX Platform is that it is not a list of recommendations, nor is it a list of all the things needed in a VFX studio. It's a list of packages that have historically been a such a compatibility versionitis nightmare (often because of deficiencies in their design or packaging) that we needed to get the major DCC vendors to agree to all use the same version in the same year, and this is the list of which versions they negotiated. Nothing more and nothing less.

lgritz avatar Oct 12 '23 18:10 lgritz

Copy that, thanks!

aclark4life avatar Oct 12 '23 18:10 aclark4life

I would disagree with this. I think there is one set of users who are only after the Python APIs and thus view OIIO like any other Python library that they want to import. But a second set of users merely want "pip install openimageio" as the simplest and least painful way to get a full binary install of the entire package and its dependencies. Those people want python, C++, oiiotool, the whole schmear. Also, I think that somebody who starts out only wanting/needing the python interface could easily find themselves in a position of later needing oiiotool, and I wouldn't want them to suddenly be stuck without any way to get it without slogging through a full set of builds from source.

Indeed @lgritz. The work I did last year included adding the tools into the wheels for this exact reason (because people need th tools or the API or both).

OK so in Python lingo, we have console_scripts = oiiotool so that folks have access to the same command line they'd have available if they compiled the C++?

aclark4life avatar Oct 12 '23 19:10 aclark4life

Yep, and it's already there, see https://github.com/AcademySoftwareFoundation/OpenImageIO/pull/4011/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R37-R44

We should probably talk at some point about which are important enough to include in the python wheel. Definitely oiiotool and maketx. Probably not testshade, which I think is only used in the testsuite for unit tests but isn't used by users (am I right about that?). Technically, iinfo, iconvert, and idiff don't do anything that oiiotool can't do (they exist mostly because they predate oiiotool), though some people find them useful because they each do one simple thing well without having to look up the oiiotool commands needed to do the same thing. I honestly don't know if anybody uses igrep (it seemed cool on the day I wrote it, but I also can't recall the last time I needed it). And iv... maybe? (Though also maybe it should be renamed, it's so short and generic it does scare me a little with it being confused with or hiding other things.)

lgritz avatar Oct 12 '23 21:10 lgritz

OK rebased and fixed issues raised by @lgritz and it looks like @JeanChristopheMorinPerso signed DCO :rocket: so today I will continue with cherry-picking remainder of applicable @JeanChristopheMorinPerso commits.

aclark4life avatar Oct 13 '23 10:10 aclark4life

@aclark4life Can you please mark this as a draft PR, and then remove the draft status when you think it's ready to go? I'm afraid that my knowledge about what you're trying to do is so thin that I'm not sure it will be obvious to me when this is ready to review or complete.

Alternately, if you know how to break it into tasks or milestones, you could amend the description to have that checklist and tick them off as they get done, then it'll be easy to see which pieces are done and which are still in progress.

lgritz avatar Oct 13 '23 16:10 lgritz

@lgritz Almost done with something that can get merged!

In #3249 you'll see a list of commits from @JeanChristopheMorinPerso that I've cherry-picked and fixed conflicts. A few more to go, then I'll add the trusted publisher configuration and we'll call it a sprint.

aclark4life avatar Oct 13 '23 19:10 aclark4life

I think a lot of the CI failures are from having broken the build_foo.bash scripts, no longer able to do the build dependencies in the designated directories but instead forcing the builds to be in ./build.

lgritz avatar Oct 13 '23 21:10 lgritz

@lgritz @JeanChristopheMorinPerso Can either of you summarize the build directory regression? I just fixed a bunch of issues, but not that one because I'm hoping there's a consistent fix that can be applied everywhere we have that issue.

aclark4life avatar Oct 28 '23 20:10 aclark4life

@lgritz @JeanChristopheMorinPerso Can either of you summarize the build directory regression? I just fixed a bunch of issues, but not that one because I'm hoping there's a consistent fix that can be applied everywhere we have that issue.

Just compare it directly to the current master. I understand that you added a few of these build scripts for dependencies where we lacked them entirely. But for the ones that we already had, nothing should change except the absolute minimum additions you need to get the python wheel build, right?

lgritz avatar Oct 28 '23 21:10 lgritz

Wondering what the progress on this is, or what roadblocks exist that we can help with. It might be nice to have this solved once and for all by the time of the 3.0 release this fall.

lgritz avatar Mar 20 '24 05:03 lgritz

@lgritz Sorry, got sidetracked but still interested in completing, and fall is super reasonable. I'm pitching Pillow project to TAC today during which time I'll mention my status:

  • Pillow has no C++ and does not use CMake, but we do have some C code and we do have an elaborate GHA infrastructure already in place for deploying wheels to PyPI via Trusted Publisher: https://github.com/python-pillow/Pillow/blob/main/.github/workflows/wheels.yml
  • Other ASWF projects seem to be slightly ahead in publishing wheels e.g. OpenEXR https://github.com/AcademySoftwareFoundation/openexr/blob/main/.github/workflows/python-wheels-publish.yml
  • All ASWF projects should share a common infrastructure for publishing wheels via Trusted Publisher and I'd like to continue to help build that infrastructure.

aclark4life avatar Mar 20 '24 11:03 aclark4life

Yes, I agree that the only sensible path is to use the same mechanisms as other ASWF projects that are publishing python wheels, unless there are serious deficiencies in their methodologies (in which case we should fix that, then copy what they've done).

lgritz avatar Mar 20 '24 17:03 lgritz

Just compare it directly to the current master. I understand that you added a few of these build scripts for dependencies where we lacked them entirely. But for the ones that we already had, nothing should change except the absolute minimum additions you need to get the python wheel build, right?

I think I fixed these

aclark4life avatar Mar 22 '24 20:03 aclark4life

Not sure how much actual progress I've made but I just rebased and fixed a bunch of issues raised in comments (or at least tried to!)

aclark4life avatar Mar 22 '24 20:03 aclark4life

What's the status of this PR?

Is this waiting for me specifically?

It's still marked as a draft, but also it seems out of date with many merge conflicts if it were to be merged now.

And most importantly, I should not be the reviewer here. I don't know anything about python wheels. Ideally, this should be reviewed by (a) somebody who has made python wheels before and will be consuming them; and also (b) somebody who set up the python wheels for another ASWF project and therefore knows if this is conforming to a common methodology.

lgritz avatar Jun 02 '24 01:06 lgritz

What's the status of this PR?

Just lazy-rebased by resolving conflicts with master using master branch files. That means we've lost changes made to those files for this PR because they are now identical to the master branch files. We could just close this for now, however there are still some changes that could be reviewed by anyone interested helping with this issue. Not to mention this is an awesome learning process for me! So I'm going to leave it open for the time being if that's OK with you.

Is this waiting for me specifically?

No, not especially, unless you are interested in helping with this issue.

It's still marked as a draft, but also it seems out of date with many merge conflicts if it were to be merged now.

Right this is a fork of JC's branch in which I've tried to finish what JC started but have not managed to do so yet. Since then, the issue of "how do we do this consistently across all projects" was raised.

And most importantly, I should not be the reviewer here. I don't know anything about python wheels.

I would not feel obligated to review, but also note wheels are just zip files and they are installed by unzipping the contents to Python's site-packages directory. When Python is run, >>> import OpenImageIO "just works" via Python's import mechanism. In the case of libraries compiled against C++, apparently that mechanism includes loading so files e.g.

Screenshot 2024-06-02 at 9 57 54 AM

Ideally, this should be reviewed by (a) somebody who has made python wheels before and will be consuming them; and also (b) somebody who set up the python wheels for another ASWF project and therefore knows if this is conforming to a common methodology.

The general consensus from various discussions I've had and observed IIUC is "big studios don't need this" and "many small studios do". So, we probably need more input from "many small studios" on #3249 particularly when it comes time to testing. You mentioned timing this with the fall release and I'm still tentatively planning to do that. However I'm mostly preoccupied with job search now, hoping the need to search will subside by beginning of summer.🤞

aclark4life avatar Jun 02 '24 13:06 aclark4life

I'm not qualified to review this PR either but … just wanted to note that OpenVPCal installation instructions say that they'd be able to make that project available on PyPi as soon as OIIO is on PyPi.

https://github.com/Netflix/OpenVPCal/tree/main?tab=readme-ov-file#why-not-pypi

JGoldstone avatar Jun 02 '24 17:06 JGoldstone

Right this is a fork of JC's branch in which I've tried to finish what JC started but have not managed to do so yet. Since then, the issue of "how do we do this consistently across all projects" was raised.

I'm not sure to get what you mean by "how do we do this consistently across all projects". Build backends (setuptools, scikit-build-core, etc) are an implementation detail that users and the tooling will never see and building wheels for bindings nowadays is a well defined process and standardized process. All ASWF projects that have bindings do approximately the same thing which is to use cibuildwheel. So I don't know what consistency means here since our projects are already consistent in how they build their wheels (minus build backends, but again that doesn't matter).

I would not feel obligated to review, but also note wheels are just zip files and they are installed by unzipping the contents to Python's site-packages directory. When Python is run, >>> import OpenImageIO "just works" via Python's import mechanism. In the case of libraries compiled against C++, apparently that mechanism includes loading so files e.g.

Reviewing wheels is more involved than just looking at if importing the library works. A reviewer has to check what files are included, licenses bundled, the linking of libraries (static vs shared), the size of the wheels, etc. The method of how the wheels are uploaded, the validity and accuracy of the metadata also has to be reviewed. And then there are per project things to check that will differ from project to project.

As I said in the past, I'll be happy to review once this PR is complete and ready to review. Right now what's missing is the glue. There is a disconnect between the actual build and where dependencies are fetched from. There is also no CI to test the wheels and test that the builds actually work.

The general consensus from various discussions I've had and observed IIUC is "big studios don't need this" and "many small studios do". So, we probably need more input from "many small studios" on https://github.com/AcademySoftwareFoundation/OpenImageIO/issues/3249 particularly when it comes time to testing.

The question is not really big studios vs small studios. The main argument of creating wheels and more importantly making wheels available on PyPI is to make projects easily accessible for all users, including other projects (like @JGoldstone highlighted above).

This needs DCOs on all commits

lgritz avatar Jun 02 '24 18:06 lgritz

"how do we do this consistently across all projects".

The goal is not a blind consistency across all ASWF projects for its own sake, but rather, we observe that some projects have already worked this task out and have been making wheels long enough to have shaken out most of the rookie mistakes. Therefore, using their methodology as a starting point seems like a way to greatly reduce the risk of unforced errors on our part, and also increases the chances that process improvements in one project can be ported to the others, and that anyone who understands the wheel creation in one project is already mostly up to speed on the others.

There is also no CI to test the wheels and test that the builds actually work.

It seems vital to have a comprehensive and fully automated test of the wheel as a gating factor for publishing it to PyPI.

lgritz avatar Jun 02 '24 18:06 lgritz