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

Automatically call CMake as part of PEP 517 build #1512

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

Conversation

mgorny
Copy link

@mgorny mgorny commented Feb 12, 2025

Call CMake and build the CPU extension when invoking the build via a PEP 517 backend, to ensure that at least some extension is built when users are building from source. This improves consistency with other Python packages, and reduces the risk of accidents.

We are using scikit-build-core setuptools plugin to take care of CMake dependencies and call into CMake. However, we need to modify the build_py command to ensure that CMake is called prior to the setuptools command, as otherwise the newly built shared library won't be picked up by build_py.

Since setuptools is still responsible for collecting the Python package, it also collects all other shared libraries that were built earlier, for example via manual CMake calls as done in the CI pipeline. Furthermore, if the user does not have scikit-build-core installed and calls setup.py directly, we output a warning but continue working as before.

The logic can be further extended in the future, for example to detect the best COMPUTE_BACKEND default.

Fixes #1511

Call CMake and build the CPU extension when invoking the build
via a PEP 517 backend, to ensure that at least some extension is built
when users are building from source.  This improves consistency with
other Python packages, and reduces the risk of accidents.

We are using `scikit-build-core` setuptools plugin to take care of CMake
dependencies and call into CMake.  However, we need to modify
the `build_py` command to ensure that CMake is called prior to
the setuptools command, as otherwise the newly built shared library
won't be picked up by `build_py`.

Since setuptools is still responsible for collecting the Python package,
it also collects all other shared libraries that were built earlier,
for example via manual CMake calls as done in the CI pipeline.
Furthermore, if the user does not have `scikit-build-core` installed
and calls `setup.py` directly, we output a warning but continue working
as before.

The logic can be further extended in the future, for example to detect
the best COMPUTE_BACKEND default.

Fixes bitsandbytes-foundation#1511

setup(version="0.45.3.dev0",
packages=find_packages(),
distclass=BinaryDistribution,
Copy link
Author

Choose a reason for hiding this comment

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

Technically, this is no longer necessary when CMake is called. However, since I preserved the support for using setup.py without scikit-build-core installed, I've left this hack in.

Copy link

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@matthewdouglas
Copy link
Member

Thanks! I do have a few questions, as I'm not familiar with scikit-build-core at all.

  • There's platforms that we don't quite support yet, such as macOS, but we do have some scaffolding on CMakeLists.txt to try to build for that platform anyway. We would want to avoid giving users the impression that everything should work if they happen to build an sdist. Is there a recommended way with scikit-build-core to provide a warning or fail the install?
  • Is this expected to work as an editable install?
  • Is there a way we can avoid invoking CMake again when building a wheel in the event that we've built the shared libraries externally?

@mgorny
Copy link
Author

mgorny commented Feb 19, 2025

  • Is there a recommended way with scikit-build-core to provide a warning or fail the install?

To be honest, I'm not very fluent with it either, and we aren't exactly using it in the most standard way here either (to allow for the current behavior of placing the shared library directly in source directory). In general, since the "base" here is setuptools, I think the simplest way would be to add an appropriate check to setup.py.

  • Is this expected to work as an editable install?

Yes.

  • Is there a way we can avoid invoking CMake again when building a wheel in the event that we've built the shared libraries externally?

Since it's all handled via setup.py, Python's the limit :-). I can either add some code to look for an existing library, or use an environment variable to control that — just let me know which approach you prefer.

@mgorny
Copy link
Author

mgorny commented Feb 19, 2025

Hmm, I have a slightly different idea. The current approach of writing straight into source directory is a bit hacky. I think a cleaner and more future-proof way would be to convert into scikit-build-core and use a more standard approach of installing all package files along with one compiled library into site-packages, for direct use by users. For packaging purposes, I could add a separate CMake code path that specifically collected libraries from the specified location and installed them, to be used in CI.

@matthewdouglas matthewdouglas self-assigned this Mar 3, 2025
@matthewdouglas matthewdouglas self-requested a review March 3, 2025 16:44
@Titus-von-Koeller
Copy link
Collaborator

Titus-von-Koeller commented Mar 7, 2025

First of all, thank you for your proactive approach with this PR. I agree that it makes sense to add this and it's very nice to have community members with a hands-on approach take the initiative.

As we're currently trying to streamline things a bit in our repo, just wondering what's the plan for this PR?

Are there any unresolved issues that need our input?

Is it clear which open steps we have? If we do, could those be added as a task list on the PR?

@mgorny
Copy link
Author

mgorny commented Mar 17, 2025

@Titus-von-Koeller, I'm sorry but I'm not sure if I understand your question.

Right now, I think we're at the point of discussing a solution. This PR provides a "minimal change" solution to the problem at hand — however, as I noted above, it's a bit hacky still.

Alternatively, I could explore more significant changes to the build system, that would streamline the build process into a typical PEP517 scikit-build-core package, that builds and installs a single variant — and then build an additional "CI support" workflow that would allow combining different variants on top of that. I think the end result would be the same, but the changes to code would be more significant. Notably, different libraries would not be picked implicitly by setuptools anymore, but would be pulled explicitly via CMake. That said, I'm not 100% sure if it'll work but that's my risk.

So I'm open to proceeding either way, just would like to know what you think. I've just pushed a fix to this approach, by the way, that should hopefully resolve the CI failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default to building the C extension automatically when installing from source
3 participants