build: Add support for pip install OpenImageIO
#4011
Conversation
There was a problem hiding this comment.
To fix the failing "DCO" check, the last time this happened to me I had to amend my commit and force-push my branch again (the PR will auto-update):
git commit --amend -s
git push --force origin <your branch> # or similar...
The -s (which you could have used on any commit) appends the "Signed-off-by" line to your commit message.
I feel this could maybe have a better description too, since this typically ends up in git history in some form.
What is setup.py? That file doesn't exist in the repo at the moment. Who is Jean-Christophe? They don't seem to be any former contributor here; is that information important? I had to do a bit of internet searching to realize who this was :) Some of the results are, of course, interesting.
pyproject.toml
Outdated
| @@ -0,0 +1,52 @@ | |||
| [project] | |||
| name = "OpenImageIO" | |||
| version = "2.4.0.dev1" | |||
There was a problem hiding this comment.
Should this version match what OIIO itself declares? Saying 2.4 is weird since that's an old version now etc.
There was a problem hiding this comment.
Yes, I updated with 2.6.0.2 because that looked reasonably close. It doesn't really matter yet because even after this is merged, and we can see GitHub actions building wheels, we'll need to integrate GitHub Actions with Trusted Publishers. Or in the short term, if you wanted to publish to PyPI before that happens, we could make sure the version number is correct in the release branch then manually upload wheels with twine.
pyproject.toml
Outdated
| dependencies = [ | ||
| "setuptools>=68.2.2", | ||
| ] | ||
| requires-python = ">2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*, !=3.4.*, !=3.5.*, !=3.6.*, !=3.9.0" |
There was a problem hiding this comment.
Is there a particular reason to exclude 3.9.0 exactly? Also, would attempting to use python 2.7.18 pass this check? If so, I have a meta question/comment around if we want to continue to support and encourage such nonsense :)
There was a problem hiding this comment.
3.9.0 is known to be buggy in when used with Pybind11.
There was a problem hiding this comment.
And no, I don't think we should support 2.7. We'll need at least Pybind11 2.10, which doesn't support python 2.7.
There was a problem hiding this comment.
VFX Platform 2022 uses Python 3.9, so it's definitely in the mix of things we support, and I don't recall any pybind11 problems (for OIIO anyway).
There was a problem hiding this comment.
Removed all Python version restrictions. Definitely not going to support 2.7 or anything older than 3.9 at this point, but I'm not sure if we need to be explicit about it here.
There was a problem hiding this comment.
This may help: https://docs.google.com/spreadsheets/d/1EwRlz5ZYObEOdBfIk8iTX5thlpTyEAfp3bxOgAfFOiU/edit#gid=1592527777
That shows (near the bottom) what VFX Platform years the different studios are using. Prior to 2020 is definitely not needed, and I think even 2020 (maybe 2021) is on the way to being phased out. This also matches with VFX Platform's own recommendation of trying to support up to 3 years back from the present.
There was a problem hiding this comment.
I'll put it in even more stark terms: my own studio will definitely be transitioned away from Python 2.7 by the end of this year, I hope. (And onto an assortment of 3.7, 3.9, 3.10, depending on the specific one of the several DCCs we are depending on.)
There was a problem hiding this comment.
In terms of priority, it's likely that the users who need a simple "pip install" approach most desperately (i.e., want to avoid building anything from source) are very likely on a very recent python. So that's probably top priority. But I'm playing devil's advocate here to make sure we don't back ourselves into a corner, design wise, that will make it difficult to eventually support python versions as far back as the VFX years we claim to support.
There was a problem hiding this comment.
I do agree that we should definitely not support python older than 3.7. Whether 3.7 or 3.9 is the minimum we want to support for "pip install" is reasonable for debate, and I don't have a firm opinion on it, other than that we should choose among them for a principled reason.
There was a problem hiding this comment.
The code base already supports 3.7 and up thanks to Pybind11, so building for 3.7 isn't more difficult than building for 3.11. 3.12 is a different story though because it's still pretty recent (it's only one week old). In other words, once you get a build running and working wheels for one version, you get the other versions for free without any additional work (in most cases, since Pybind11 abstracts everything for us).
|
Hi @jessey-git 👋, I'm the Jean-Christophe mentioned in the description. I volunteered to help with #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. |
aclark4life
force-pushed
the
python-wheels
branch
from
2e29bd8
to
010d023
Compare
|
@jessey-git Sorry, this is a WIP I created to get the CLA signed before tomorrow! |
|
@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
force-pushed
the
python-wheels
branch
from
010d023
to
3a059eb
Compare
aclark4life
changed the title
pip install OpenImageIO
|
Happy sprint day 1 folks! Please correct me if any of these assumptions are wrong:
Most importantly, if I understand correctly, the |
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.
Correct.
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.
Yes.
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. |
|
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. |
|
Copy that, thanks! |
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 |
|
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.) |
aclark4life
force-pushed
the
python-wheels
branch
from
3a059eb
to
79b8486
Compare
src/build-scripts/ci-startup.bash
Outdated
| @@ -73,4 +73,4 @@ elif [[ "${RUNNER_OS}" == "macOS" ]] ; then | |||
| fi | |||
|
|
|||
| # Save the env for use by other stages | |||
| src/build-scripts/save-env.bash | |||
| # src/build-scripts/save-env.bash | |||
There was a problem hiding this comment.
I think this line is going to turn out to be important.
aclark4life
force-pushed
the
python-wheels
branch
2 times, most recently
from
b961cbe
to
e77ace4
Compare
|
OK rebased and fixed issues raised by @lgritz and it looks like @JeanChristopheMorinPerso signed DCO 🚀 so today I will continue with cherry-picking remainder of applicable @JeanChristopheMorinPerso commits. |
aclark4life
force-pushed
the
python-wheels
branch
from
e77ace4
to
6b04c99
Compare
|
@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. |
aclark4life
force-pushed
the
python-wheels
branch
2 times, most recently
from
8ff082f
to
d8ce54a
Compare
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? |
aclark4life
force-pushed
the
python-wheels
branch
from
5891842
to
c704487
Compare
|
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 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:
|
|
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). |
aclark4life
force-pushed
the
python-wheels
branch
from
c704487
to
98bc312
Compare
I think I fixed these |
aclark4life
requested review from
lgritz,
jessey-git and
JeanChristopheMorinPerso
|
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!) |
|
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. |
Signed-off-by: Alex Clark <aclark@aclark.net>
- Add build scripts for wheel dependencies. - Fix OpenColorIO. - Working wheel build. - Remove unused scripts. - Cleanup CentOS build. - Require Python Development.Module instead of Development since the libpython is not required to build the extension. - Remove unnecessary changes. - Working windows build! - Build from pyproject.toml with scikit.build Signed-off-by: Alex Clark <aclark@aclark.net>
aclark4life
force-pushed
the
python-wheels
branch
from
364cfb3
to
0a23f30
Compare
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.
No, not especially, unless you are interested in helping with this issue.
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 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 
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.🤞 |
|
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 |
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).
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 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 |
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.
It seems vital to have a comprehensive and fully automated test of the wheel as a gating factor for publishing it to PyPI. |
Description
Adds support for
pip install OpenImageIOrequested in #3249Tests
Checklist:
(adding new test cases if necessary).
corresponding Python bindings (and if altering ImageBufAlgo functions, also
exposed the new functionality as oiiotool options).
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.