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

Provide CMake-driver for building libcustom_calls #1345

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

mlxd
Copy link
Member

@mlxd mlxd commented Nov 29, 2024

Context: This PR migrates the custom calls library from setup.py driven compilation to CMake. Setuptools is still used to drive the build steps for the extension module, but with full support for a cmake-only build.
This PR also introduces scaffolding to allow the entire ecosystem to later be built from a single CMake driver, with subsequent work being undertaken in https://github.com/PennyLaneAI/catalyst/tree/cmakify_ecosystem

Description of the Change: As above. This PR also restricts the MacOS wheels to only their targeted host as a platform, removing the intermediate universal2 wheel on ARM platforms.

Benefits: Ensures all compiled modules can be produced with CMake.

Possible Drawbacks:

Related GitHub Issues:

@mlxd mlxd changed the title Provide single CMake-driver for building Catalyst [WIP] Provide single CMake-driver for building Catalyst Nov 29, 2024
CMakeLists.txt Show resolved Hide resolved
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md on your branch with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@mlxd mlxd changed the title [WIP] Provide single CMake-driver for building Catalyst [WIP] Provide CMake-driver for building libcustom_calls Nov 29, 2024
@mlxd mlxd requested a review from erick-xanadu December 6, 2024 18:47
@mlxd mlxd marked this pull request as ready for review December 6, 2024 18:47
@mlxd mlxd changed the title [WIP] Provide CMake-driver for building libcustom_calls Provide CMake-driver for building libcustom_calls Dec 6, 2024
setup.py Outdated Show resolved Hide resolved
@mlxd mlxd added author:build-wheels Run the wheel building workflows on this Pull Request and removed do-not-merge labels Dec 6, 2024
setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@erick-xanadu erick-xanadu left a comment

Choose a reason for hiding this comment

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

🤔 I deleted the build directory for our dialects and then pip install -e . --extra-index... but the directory didn't get rebuilt. Is this something that will be implemented in this PR?

@mlxd
Copy link
Member Author

mlxd commented Dec 9, 2024

🤔 I deleted the build directory for our dialects and then pip install -e . --extra-index... but the directory didn't get rebuilt. Is this something that will be implemented in this PR?

Ok, looks like something funky happened --- feel free to ignore this for now until I can see why that is going on.

@mlxd mlxd requested review from erick-xanadu and dime10 December 11, 2024 16:48
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Thanks @mlxd, these are nice additions!

.github/workflows/build-wheel-macos-arm64.yaml Outdated Show resolved Hide resolved
frontend/catalyst/utils/CMakeLists.txt Show resolved Hide resolved
frontend/catalyst/utils/CMakeLists.txt Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@dime10
Copy link
Contributor

dime10 commented Dec 11, 2024

Currently getting this error for make frontend:

distutils.errors.DistutilsFileError: can't copy '/var/folders/9m/_lrz_3x97pqd351m4bzgl_r80000gq/T/tmp6i4tut61.build-lib/catalyst/utils/libcustom_calls.so': doesn't exist or not a regular file

@mlxd
Copy link
Member Author

mlxd commented Dec 11, 2024

Currently getting this error for make frontend:

distutils.errors.DistutilsFileError: can't copy '/var/folders/9m/_lrz_3x97pqd351m4bzgl_r80000gq/T/tmp6i4tut61.build-lib/catalyst/utils/libcustom_calls.so': doesn't exist or not a regular file

I presume the Makefile is doing something very different then to the wheel builds? If so, we may want to unify these to a single build driver and just do that. I'll reproduce the above locally and take a look

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.17%. Comparing base (d740f88) to head (10e33d3).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1345   +/-   ##
=======================================
  Coverage   97.17%   97.17%           
=======================================
  Files          79       79           
  Lines        8447     8447           
  Branches      873      873           
=======================================
  Hits         8208     8208           
  Misses        186      186           
  Partials       53       53           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mlxd mlxd marked this pull request as draft December 11, 2024 19:57
@mlxd
Copy link
Member Author

mlxd commented Dec 11, 2024

Currently getting this error for make frontend:

distutils.errors.DistutilsFileError: can't copy '/var/folders/9m/_lrz_3x97pqd351m4bzgl_r80000gq/T/tmp6i4tut61.build-lib/catalyst/utils/libcustom_calls.so': doesn't exist or not a regular file

I cannot reproduce this locally:

$~ make frontend
...
Successfully installed PennyLane-Catalyst-0.10.0.dev22 appdirs-1.4.4 astunparse-1.6.3 autograd-1.7.0 autoray-0.7.0 cachetools-5.5.0 certifi-2024.8.30 charset-normalizer-3.4.0 diastatic-malt-2.15.2 gast-0.6.0 idna-3.10 jax-0.4.28 jaxlib-0.4.28 ml-dtypes-0.5.0 networkx-3.4.2 numpy-2.0.2 opt-einsum-3.4.0 packaging-24.2 pennylane-0.40.0.dev20 pennylane-lightning-0.40.0.dev30 pennylane-lightning-kokkos-0.40.0.dev30 requests-2.32.3 rustworkx-0.15.1 scipy-1.14.1 scipy-openblas32-0.3.28.0.2 six-1.17.0 termcolor-2.5.0 toml-0.10.2 typing_extensions-4.12.2 urllib3-2.2.3 wheel-0.45.1
rm -r frontend/PennyLane_Catalyst.egg-info

Can you try to purge your existing venv and build cache, and create a fresh one? Since the build driver is different than before, it may be resolved by this. If not, I'm a little confused.

@dime10
Copy link
Contributor

dime10 commented Dec 11, 2024

Currently getting this error for make frontend:

distutils.errors.DistutilsFileError: can't copy '/var/folders/9m/_lrz_3x97pqd351m4bzgl_r80000gq/T/tmp6i4tut61.build-lib/catalyst/utils/libcustom_calls.so': doesn't exist or not a regular file

I presume the Makefile is doing something very different then to the wheel builds? If so, we may want to unify these to a single build driver and just do that. I'll reproduce the above locally and take a look

I think the only difference really is that make frontend does an editable build 🤔

@mlxd
Copy link
Member Author

mlxd commented Dec 11, 2024

Yep, the editable build was the issue --- it apparently does not respect parts of the setup.py file that a wheel build will. I've reverted to using the .so for now as a temporary workaround.

I can look into (when time permits) injecting the right types with a restructuring of the CMakeExtension to instead have an additional (non ext module) builder wrapper, or to replace setuptools+distutils with a more modern build driver

@mlxd mlxd marked this pull request as ready for review December 11, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author:build-wheels Run the wheel building workflows on this Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants