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

experiment with scikit-build-core as the build system #98

Closed
wants to merge 4 commits into from

Conversation

chillenb
Copy link
Contributor

@chillenb chillenb commented Jul 3, 2024

I think scikit-build-core is mature enough to rely on these days. There are some rough edges, but it's much nicer to use than distutils, and it takes care of many things automatically. Now that distutils is gone from python 3.12, it makes a lot of sense to migrate.

I've been able to build block2 for python 3.12 with scikit-build-core without that much trouble. Here's what I did. Give it a try and let me know how it goes!

@hczhai
Copy link
Contributor

hczhai commented Jul 3, 2024

Hi, thanks for the contribution. Personally I do not want to have the extra dependence on scikit-build-core. This might be useful at a later point if things totally break in Python 3.13.

The CMakeLists.txt in block2 has been carefully adjusted to deal with many different environments. It may look complicated but it is proved to be necessary. distutils can still be used in Python 3.12 (you can simply do pip install setuptools to get it).

@chillenb
Copy link
Contributor Author

chillenb commented Jul 3, 2024

No problem--I understand if you don't think switching is the right choice here. Thanks for replying so quickly -- and thanks for the tip about obtaining distutils via setuptools.

Out of curiosity, what kinds of environments required the most changes to CMakeLists.txt? (Maybe it is a cluster with an old CMake version ?)

@hczhai
Copy link
Contributor

hczhai commented Jul 3, 2024

Thanks for your understanding!

Out of curiosity, what kinds of environments required the most changes to CMakeLists.txt? (Maybe it is a cluster with an old CMake version ?)

For example, you can see the GitHub actions failed with your changes on CMakeLists.txt: https://github.com/block-hczhai/block2-preview/actions/runs/9771503523/job/26974844955?pr=98 And you can see it cannot find pybind11 because you changed the way of finding it.

I agree that the old way of finding pybind11 (https://github.com/block-hczhai/block2-preview/blob/master/CMakeLists.txt#L79-L80) is less elegant, but it works quite well and it does not require the user to set any environment variables. Another problem of find_package(pybind11 CONFIG REQUIRED) is that pybind11 is not a mandatory dependence of block2 (so REQUIRED should be removed). The same CMakeLists.txt can be used with -DBUILD_LIB=OFF where the libblock2.so will be built, which can then be linked with other C++ code (thus there will be no Python dependence).

We are trying hard to remove dependences (or at least making things optional): Python is optional, MKL is optional, MPI is optional, and even cmake is optional (block2 can be used as a header-only C++ library so in principle #include "block2.hpp" is enough and you do not need to compile even a libblock2.so, although certain things will be slower in this mode).

Loosely speaking, all complexities in CMakeLists.txt are external complexities (namely, the complexities from dependences rather than block2 itself). The user needs to deal with pybind11 or other Python complexities because the user wants to use block2 as a Python package. But block2 does not force the user to use it in a Python environment.

For the most common case, pip install block2==0.5.3rc14 --extra-index-url=https://block-hczhai.github.io/block2-preview/pypi/ should work with Python 3.12 which will download the binary and install it in seconds, then all complexities in CMakeLists.txt are irrelevant. You might be wondering why there is an extra option --extra-index-url=...: that is because the storage limit in PYPI is very tight and we cannot put too many versions or binary files there -- another external complexity. If you are okay with Python 3.11, then pip install block2 can simply work.

@chillenb
Copy link
Contributor Author

chillenb commented Jul 3, 2024

I see what you mean. If my previous changes broke your CI, that's quite bad. I tried working in a pure Python venv and I see how some of the mysterious-looking CMake configs were important for finding pybind11, as you pointed out. Thanks for explaining all of this.

I understand what you're saying about keeping dependencies optional in CMakeLists.txt so the user has a lot of flexibility in configuration. Sorry for interfering with that!

As a proof-of-concept, I restored the original CMakeLists.txt and added only the minimal changes necessary for scikit-build-core to work. I think now the github action should work (fingers crossed!). Actually, the same CMakeLists should work with your original setup.py --- from CMake's perspective, scikit-build-core is completely optional.

For my own use, I need to compile against my cluster's MKL and MPI installations, but python 3.11 is totally fine. I will follow the instructions online and refer to CMakeLists.txt to set the proper environment variables, etc. Hopefully there won't be problems and it'll go smoothly.

@chillenb chillenb changed the title change the build system to scikit-build-core experiment with scikit-build-core as the build system Jul 3, 2024
@hczhai
Copy link
Contributor

hczhai commented Jul 3, 2024

Actually, the same CMakeLists should work with your original setup.py

For this, setup.py should not be deleted. See, for example: https://github.com/block-hczhai/block2-preview/blob/master/.github/workflows/build.yml#L303-L309

@chillenb chillenb marked this pull request as draft July 3, 2024 23:59
@chillenb
Copy link
Contributor Author

chillenb commented Jul 4, 2024

Actually, the same CMakeLists should work with your original setup.py

For this, setup.py should not be deleted. See, for example: https://github.com/block-hczhai/block2-preview/blob/master/.github/workflows/build.yml#L303-L309

Good point about the workflow! Rather than retain the setup.py, which may confuse scikit-build-core, I provided a copy of the workflow file with all the sed commands changed appropriately. This was possible since pyproject.toml has nearly all configurations previously in setup.py, just in a different syntax.

@hczhai
Copy link
Contributor

hczhai commented Jul 4, 2024

This was possible since pyproject.toml has nearly all configurations previously in setup.py, just in a different syntax.

Thanks for the changes. Unfortunately, the setup.py did more than compiling a Python extension module. If you have a look at https://github.com/block-hczhai/block2-preview/blob/master/setup.py#L125-L140, the cmake command runs twice. The first time with -DBUILD_LIB=ON for building a Python extension module, and the second time with -DBUILD_LIB=OFF for building a normal binary executable (named block2). For the latter see https://block2.readthedocs.io/en/latest/user/interfaces.html#input-file-c-executable.

For why we need to additionally build this non-Python executable in the package: when you need to run very large scale DMRG (using more than 10k CPU cores), the overhead of Python/numpy will become significant (in certain computer architectures). The only way to get desired scalability is to get rid of Python. So this non-Python version is important.

For why it makes sense to include non-Python binaries in a Python package: this is common practice, see https://pypi.org/project/mkl/, https://pypi.org/project/cmake/, etc.

In general, as I explained before, if scikit-build-core is offered as an additional optional thing for building block2, I am ok to consider it as it might be useful for other users like you. In contrast, if you delete setup.py (just to make scikit-build-core happy), then scikit-build-core becomes a mandatory dependence for building block2, which does not seem reasonable to me, given the fact that the current building process works very well (for both Python 3.12 and older versions). Note that scikit-build-core itself has many other dependences (including setuptools for tests and docs).

@chillenb
Copy link
Contributor Author

chillenb commented Jul 4, 2024

Your reasoning certainly makes sense--there's no real reason to switch to scikit-build core since the current build system works fine. I agree with this. If, one day, setuptools+setup.py no longer provides a good solution, then it might be worthwhile.

Also, sorry for failing to notice the block2 executable being built and shipped in the wheels. Oops! 😖 Thanks for your patience.

Your suggestion about supporting scikit-build-core alongside the usual build system is very interesting. I only know one package that does this -- mpi4py -- so it should be possible! I will play around with this to see whether it can be done without making setup.py too ugly, and I'll get back to you with the results.

@hczhai
Copy link
Contributor

hczhai commented Jul 4, 2024

Thanks for your efforts on improving this!

@chillenb
Copy link
Contributor Author

chillenb commented Jul 5, 2024

Supporting multiple build backends like mpi4py seems like it will add complexity and add new sources of bugs. A stripped-down version of the method is to use a custom build backend like this one:

import os
from functools import cache
import importlib

@cache
def choose_build_backend():
    name = os.environ.get("BLOCK2_BUILD_BACKEND", "")
    name = name.lower().replace("_", "-")
    if name in ("skbuild", "scikit-build-core"):
        print("Using scikit-build-core backend")
        return "scikit_build_core.build"

    print("Using setuptools backend")
    return "setuptools.build_meta"

def import_build_backend():
    return importlib.import_module(choose_build_backend())

def build_wheel(*args, **kwargs):
    return import_build_backend().build_wheel(*args, **kwargs)

def build_sdist(*args, **kwargs):
    return import_build_backend().build_sdist(*args, **kwargs)

def get_requires_for_build_wheel(*args, **kwargs):
    return import_build_backend().get_requires_for_build_wheel(*args, **kwargs)

def get_requires_for_build_sdist(*args, **kwargs):
    return import_build_backend().get_requires_for_build_sdist(*args, **kwargs)

def prepare_metadata_for_build_wheel(*args, **kwargs):
    return import_build_backend().prepare_metadata_for_build_wheel(*args, **kwargs)

I'm not too enthusiastic about this solution. Maybe it's best to close this PR?

@hczhai
Copy link
Contributor

hczhai commented Jul 5, 2024

Supporting multiple build backends like mpi4py seems like it will add complexity and add new sources of bugs.

I am glad that we reached an agreement at least in the context of block2. Actually compiling the Python extension of block2 only requires just one line (or three lines for readability) of code:

g++ -O3 -D_USE_DMRG -D_USE_SU2SZ -fopenmp -fvisibility=hidden -shared -std=c++11 \
    -fPIC $(python3 -m pybind11 --includes) ../src/pybind.cpp -lblas -llapack \
    -o block2$(python3-config --extension-suffix)

which is surprisingly clear and easy for any adaptation, if compared with something like python3 setup.py calling choose_build_backend calling scikit_build_core calling cmake calling make calling g++.

@hczhai hczhai closed this Jul 5, 2024
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.

2 participants