Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add .pyi generation and specify numpy dependency for Python wrapper build #1933

Closed
wants to merge 7 commits into from

Conversation

404Vector
Copy link
Contributor

Thanks to @cary-ilm for writing the new python wrapper. I'm sure this contribution will make OpenEXR much more user-friendly for python users in the future.

I tested generating pyi files according to @Parskatt suggestion. And I was able to generate pyi files easily (automatically) using pybind11-stubgen.

There are some issues when generating .pyi files with this tool targeting OpenEXR.so.
For example, the type hints for methods like OpenEXR.File.header() are generated as dicts, which can lead to situations where the programmer cannot specifically identify the internal objects.

However, I think it is worth updating because it still shows a lot of information about what objects are inside.


This pull request introduces enhancements to the Python wrapper build process with the following changes:
1. Automatic .pyi Generation:
• Updates CMakeLists.txt to automatically generate .pyi files during the build process using pybind11-stubgen.
• This ensures better IDE support and type hinting for the generated Python bindings.
2. numpy Dependency Specification:
• Adds numpy as a required dependency in pyproject.toml.
• Ensures that numpy is automatically installed in the Python environment during the build process if it is not already present.
3. Dependency Updates:
• Includes pybind11-stubgen in the build requirements to facilitate .pyi generation.
4. Additional Updates:
• Updates .gitignore to include Python-related ignores.

These changes aim to improve the developer experience, streamline the build process, and ensure the generated Python bindings provide proper type annotations for enhanced usability.

closed #1607 #1919

- Additional dependencies required for the build environment
Numpy must be installed in the python environment where the build is being performed in order to build.

- Automatic installation of dependencies like a normal python package
When installing this package, if numpy does not exist in the python environment, it will be installed automatically.

Signed-off-by: HyeongSeok Kim <[email protected]>
Copy link

linux-foundation-easycla bot commented Dec 4, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@cary-ilm
Copy link
Member

cary-ilm commented Dec 5, 2024

Thanks for this contribution!

The cmake build invoked by pypa/cibuildwheel is failing in the .github/workflows/python-wheels*.yml workflows, looks like they need to install pybind11-stubgen. I would have thought the "requires" statement in pyproject.toml would cover that, but apparently not.

… generating pyi file by

Signed-off-by: HyeongSeok Kim <[email protected]>
@404Vector
Copy link
Contributor Author

I'm guessing the issue is that the build environment's python isn't being used when generating the pyi file at build time.
I modified the command to explicitly use python from the build environment.

@404Vector
Copy link
Contributor Author

Even though I explicitly specified the Python interpreter, I still get dependency issues. Looking at the build log, it's clear that the required dependencies have already been installed.

It's clear that the Python that's installing the dependencies in pyproject.toml is different from the Python used for the build. (This seems like a problem that should be fixed in the future.)

To work around this problem, I changed the code to explicitly install dependencies for each python using the CIBW_BEFORE_BUILD option.

@404Vector
Copy link
Contributor Author

@cary-ilm
As a result of the experiment, problems occur when building on Mac and Windows.
Therefore, it seems difficult to generate pyi with the cmakelist command at build time in the current build environment.

It seems an easier approach to find a way to generate it separately in the workflow and then merge it at each build.

I'll do some more experiments in my personal repository.

@404Vector 404Vector closed this Dec 5, 2024
@Parskatt
Copy link

Parskatt commented Dec 5, 2024

@cary-ilm
As a result of the experiment, problems occur when building on Mac and Windows.
Therefore, it seems difficult to generate pyi with the cmakelist command at build time in the current build environment.

It seems an easier approach to find a way to generate it separately in the workflow and then merge it at each build.

I'll do some more experiments in my personal repository.

What about generating manually outside of CI and storing in the repo? This is the approach in e.g. pytorch. In some sense I think it's good to not have this too automatic.

@cary-ilm
Copy link
Member

cary-ilm commented Dec 5, 2024

Can you submit the numpy dependency as a separate PR? That resolves #1919 independently of the .pyi issue.

@JeanChristopheMorinPerso may have some insight into the pybind11-stub/CI issues, or .pyi generation in general.

@404Vector
Copy link
Contributor Author

OK!

@@ -32,6 +32,14 @@ endif()
install(TARGETS PyOpenEXR DESTINATION ${PYTHON_INSTALL_DIR} COMPONENT python)
install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/Imath.py DESTINATION ${PYTHON_INSTALL_DIR} COMPONENT python)

# Generate .pyi stub file using pybind11-stubgen. And install the generated .pyi file
add_custom_command(TARGET PyOpenEXR POST_BUILD
COMMAND PYTHONPATH=${CMAKE_CURRENT_BINARY_DIR} ${Python_EXECUTABLE} -m pybind11_stubgen -o ${CMAKE_BINARY_DIR}/OpenEXR --ignore-all-errors OpenEXR

Choose a reason for hiding this comment

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

You should take I to account that this cmake file can be use to build the python extension without all the python build system ecosystem.bthis mean that pybind11_stubgen could be missing and someone might also want to explicitly opt-out of it.

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.

OpenEXR - Python IntelliSense
4 participants