Add command line apps to Python wheel#1802
Conversation
|
Tagging @JeanChristopheMorinPerso in case you have time to check these changes, your feedback would be appreciated! |
|
Thanks for the ping @remia! I'll take a look as soon as I have time. I'm guessing you'll want to merge this before the next release? If so, when is the next release planned (so that I can prioritize on my side). |
4c2ea52 to
2db9194
Compare
JeanChristopheMorinPerso
left a comment
There was a problem hiding this comment.
This looks great @remia ! It's exactly what I would have done.
The wheel size is growing significantly with this change, I took some actions to reduce it like striping binaries and removing the test suite but it is still around 20MB per wheel. We might gain some more by using -Os and/or LTO to compile the apps but the most efficient way would most likely be to compile OCIO dynamically and have Python package and apps link to it. I haven't tried that and expect it will bring complexity to the build process.
What I did in the past was to compile dynamically, but linking the dependencies statically. So you end up with a single library that you can now link to the executables.
graph TD
A[dep1.a] -->C(libocio.so)
B[dep2.a] -->C(libocio.so)
C --> D(app1)
C --> E(app2)
C --> F(OpenColorIO.cpython310.so python lib)
On Linux and macOS, it will work without any additional work. On WIndows, You'll need to use os.add_dll_directory.
2db9194 to
a92c7b6
Compare
|
Thanks for the review @JeanChristopheMorinPerso! I'll do some tests around the dynamic linking, I think I had some issues around the wheel repair steps in the past, will keep updating this PR to address the issues coming up. I think I'm now running into a familiar issue where Python module dynamically linked to the OCIO lib don't load in Windows, I don't think it is related to Python 3.8+ as the error in this CI run was triggered on Python 3.7 wheel build. I'm probably going to have to see if I can get access to a Windows workstation to make progress on this one. |
|
@remia make sure that the dependencies are beside the OCIO shared library (which is why I suggested to statically link the dependencies to the OCIO shared library, since it's easier to bundle that way). |
|
The issue was just that on Windows the DLL for OCIO get into the bin folder on the wheel layout, so I updated the I made a new test wheel on the test PyPI, I only checked it on mac OS but hopefully should work elsewhere if our tests are to be trusted. |
|
Perfect. I'll manually inspect the wheels today on all platforms. |
|
On Linux, the wheels contain:
The diff of The executables use And it looks like the osx wheels have the same problem. On Windows, everything seems fine I am able to import |
|
Thanks for checking @JeanChristopheMorinPerso! I had tested the wheel locally on mac OS but only using pip wheel, I will try to see why repairwheel (or mac OS equivalent) think it has to fix the wheel which seem like it was already working. Great catch though, I was wondering why Windows wheels were half the size. |
|
As a quick update, I think the reason why repairwheel / delocate is patching the generated wheel is because the disabling of the SONAME symlinks I added as a CMake build option when building the wheel is preventing CMake to generate the correct library install names (instead of @rpath/mylib.dylib I get a full path on mac OS for example). I did that because otherwise I get 3 copy of the OCIO lib in the wheel due to missing support for symlinks. On the flip side, I assume relying on a wheel repair process to get viable wheels is not really ideal, as it would presumably not work well for wheels build manually from source distribution. I'll continue my investigations. |
|
I uploaded a new test release, |
a481e2e to
83ead0e
Compare
JeanChristopheMorinPerso
left a comment
There was a problem hiding this comment.
Great work @remia!
|
Thanks @JeanChristopheMorinPerso, the PR is looking much nicer following your reviews! |
|
Last round of test wheels from the last update couple of days ago available: |
|
@remia , would you resolve the conflict here and we will go ahead and merge this? I notice that our Wheel GitHub Action has been failing somewhat randomly for the last two weeks, not sure if this will improve that? |
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
Signed-off-by: Rémi Achard <remiachard@gmail.com>
83ead0e to
10c3ecb
Compare
|
@doug-walker The latest fails I see on macOS are related to the recent SIMD updates: It seems the earlier issues were solved by @cedrik-fuoco-adsk in #1826 |
* Add command line apps and remove tests on Python wheel Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix warnings on Clang 14 Signed-off-by: Rémi Achard <remiachard@gmail.com> * Python wheel using dynamic linking Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix DLL directory Signed-off-by: Rémi Achard <remiachard@gmail.com> * Check path exists Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix DLL lookup on Windows Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix linux / mac wheels Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix symlinks and make doc optional Signed-off-by: Rémi Achard <remiachard@gmail.com> --------- Signed-off-by: Rémi Achard <remiachard@gmail.com> Signed-off-by: Brooke <beg9562@rit.edu>
* Add command line apps and remove tests on Python wheel Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix warnings on Clang 14 Signed-off-by: Rémi Achard <remiachard@gmail.com> * Python wheel using dynamic linking Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix DLL directory Signed-off-by: Rémi Achard <remiachard@gmail.com> * Check path exists Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix DLL lookup on Windows Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix linux / mac wheels Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix symlinks and make doc optional Signed-off-by: Rémi Achard <remiachard@gmail.com> --------- Signed-off-by: Rémi Achard <remiachard@gmail.com> Signed-off-by: Doug Walker <Doug.Walker@autodesk.com>
This PR adds command line applications (C++ and Python) to the Python wheels, some points to note:
-Osand/or LTO to compile the apps but the most efficient way would most likely be to compile OCIO dynamically and have Python package and apps link to it. I haven't tried that and expect it will bring complexity to the build process.Note that I have no prior experience adding scripts to Python packages, I'm open to any suggestion.
Wheels are available on the test PyPI, install with this command (at your own risks):
pip install -i https://test.pypi.org/simple/ opencolorio==2.3.0.dev4