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
aclark4life
force-pushed
the
python-wheels
branch
2 times, most recently
from
f398cf1
to
5891842
Compare
|
@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? |
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). |
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
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!) |
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.