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 CMake build flag for generating Python bindings #1

Merged
merged 18 commits into from
May 14, 2021

Conversation

Mandrenkov
Copy link
Collaborator

@Mandrenkov Mandrenkov commented May 12, 2021

Context:
One of the major features scheduled for an upcoming Jet release is a set of Python bindings for the Jet C++ interface. The first step towards implementing this feature is creating a build flag for generating these bindings.

Description of the Change:

  • The required CMake version has been incremented to 3.14 in order to use FetchContent_MakeAvailable.
  • There is now a BUILD_PYTHON option in the CMake script which generates an .so file implementing the Python interface for Jet using pybind11.
  • Python bindings are now available for include/jet/Version.hpp.
  • The python/ directory is now included in the Format workflow.
  • There is now a Python workflow in GitHub Actions to generate and test the Python bindings.

Benefits:

  • The CMake script is cleaner and simpler with the help of FetchContent_MakeAvailable.
  • There is now a foundation for adding Python bindings for the other Jet C++ header files.

Possible Drawbacks:

  • Building the Jet unit tests or Python bindings requires a version of CMake which may not be available on some machines.

Related GitHub Issues:
None.

@Mandrenkov Mandrenkov added the enhancement ✨ New feature or request label May 12, 2021
@github-actions
Copy link

github-actions bot commented May 12, 2021

Test Report (Ubuntu)

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
215 tests ±0  215 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
483 runs  ±0  483 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f5debfb. ± Comparison against base commit f5debfb.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented May 12, 2021

Test Report (MacOS)

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
215 tests ±0  215 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
483 runs  ±0  483 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f5debfb. ± Comparison against base commit f5debfb.

♻️ This comment has been updated with latest results.

@Mandrenkov Mandrenkov marked this pull request as draft May 12, 2021 15:41
@Mandrenkov Mandrenkov marked this pull request as ready for review May 12, 2021 18:49
Copy link
Collaborator

@brownj85 brownj85 left a comment

Choose a reason for hiding this comment

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

Just some minor changes, and a bit of a question mark on using cmake 3.14. but I'll leave that up to your discretion

.github/CHANGELOG.md Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
python/Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
.github/workflows/format.yml Outdated Show resolved Hide resolved
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Great job on this. Just a few quick questions:

  • Would it make sense to leave object ownership in the C++ layer for the Tensor objects? Assuming so, ensuring Python never grabs them by setting the return policy to reference might be much faster (though this is speculative and would require some testing). https://pybind11.readthedocs.io/en/stable/advanced/functions.html#return-value-policies
  • Curious if we need a setup.py for the generated .so to be installable in the existing Python environment?
  • Lastly, just wondering if a separate CMake subproject rather than Makefile would be a good fit?

CMakeLists.txt Outdated Show resolved Hide resolved
@Mandrenkov
Copy link
Collaborator Author

Mandrenkov commented May 13, 2021

I'm not sure this applies to the Tensor class. As per the documentation, properties and members inherit the reference_internal policy by default; the other member functions of Tensor (which return a value) only return a scalar so the copy and move policies should be fine. For the free Tensor functions (such as Contract()), I'm assuming move is used which is what we would want.

In general, since we're not using raw pointers, pybind11 already seems to do the right thing. That said, I 100% agree that we should keep the return value policies in mind as we write more bindings and run benchmarks when we're not sure.

  • Curious if we need a setup.py for the generated .so to be installable in the existing Python environment?

All you need to do is add the Jet build directory to your PYTHONPATH and you're set! Alternatively, you can place the .so file in the same directory as your Python script. I think setup.py will be needed when we decide to build a wheel and host the Jet package on PyPI; however, we can do this at a later time.

  • Lastly, just wondering if a separate CMake subproject rather than Makefile would be a good fit?

Hmm, that sounds like an interesting idea although I've never used CMake to "build" a Python project before. How would this look like? Is there a way to automatically set up and use a virtual environment?

For the time being, I created a CMake script inside the python/ directory and put the relevant build commands in there.

@Mandrenkov Mandrenkov requested a review from mlxd May 13, 2021 18:11
@mlxd
Copy link
Member

mlxd commented May 14, 2021

Sounds good to me!

Hmm, that sounds like an interesting idea although I've never used CMake to "build" a Python project before. How would this look like? Is there a way to automatically set up and use a virtual environment?

For the time being, I created a CMake script inside the python/ directory and put the relevant build commands in there.

Sure, that's fine. The build, install, etc. can be set as a CMake custom command, and allow it all to be handled by the global CMake in the root directory. But I agree not important yet. This can always be iterated on.

CMakeLists.txt Show resolved Hide resolved
@Mandrenkov
Copy link
Collaborator Author

The build, install, etc. can be set as a CMake custom command, and allow it all to be handled by the global CMake in the root directory. But I agree not important yet. This can always be iterated on.

Oh, I see! In that case, I agree that we should do this in the long run; I'll create a story to track this task.

@Mandrenkov Mandrenkov merged commit f5debfb into main May 14, 2021
@Mandrenkov Mandrenkov deleted the python-bindings branch May 14, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants