PR[1]: build: migrate from setuptools to scikit-build-core - #252
Conversation
msricher
left a comment
There was a problem hiding this comment.
Please also upload the CMakeLists.txt. Thanks
| # E is pycodestyle | ||
| # F is pyflakes | ||
| # UP is pyupgrade - automatically updates syntax | ||
| # B is flake8-bugbear | ||
| # I is sort imports alphabetically | ||
| # PGH is pygrep-hooks | ||
| # PL is pylint | ||
| # RUF is Ruff-specific rules |
There was a problem hiding this comment.
Why remove someone's comments? Keep them
msricher
left a comment
There was a problem hiding this comment.
- Thank you, this seems fine. Can you create a new branch,
san1357:dev, fromtheochem:master? We should merge these PRs into yourdevbranch, and then at the end, merge that intotheochem:master.
|
I'm going to run a copilot review. I usually do that automatically (we should add it to the GitHub actions workflow). It's not perfect but it often helps a bit. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the Python packaging/build backend from setuptools to scikit-build-core in order to integrate a CMake-based build that can fetch and compile libcint/qcint as part of building GBasis wheels.
Changes:
- Switch
pyproject.tomlbuild backend toscikit_build_core.build, add scikit-build configuration, and raise NumPy baseline to>=2.0. - Add a top-level
CMakeLists.txtthat usesFetchContentto pull/build libcint or qcint based on platform/AVX2 detection. - Add a placeholder C source for future Python/C bindings and update the pytest workflow’s NumPy pin.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
pyproject.toml |
Migrates build backend to scikit-build-core, adjusts dependencies, and adds scikit-build metadata/version config. |
CMakeLists.txt |
Introduces CMake build logic to fetch/build libcint/qcint and build/install a Python extension module. |
gbasis/integrals/src/libcint_wrap.c |
Adds a placeholder C file intended for future libcint Python/C bindings. |
.github/workflows/pytest.yaml |
Updates CI dependency pinning for NumPy in the Py3.9 “old deps” step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [build-system] | ||
| requires = ["setuptools>=64", "setuptools-scm>=8"] | ||
| build-backend = "setuptools.build_meta" | ||
| requires = ["numpy>=2.0", "scikit-build-core>=0.9"] | ||
| build-backend = "scikit_build_core.build" |
There was a problem hiding this comment.
Fixed — added setuptools-scm to [build-system].requires.
| dependencies = [ | ||
| # Ensure the minimal versions are kept consistent with those in .github/workflows/pytest.yaml | ||
| "numpy >=1.22, <2.0.0; platform_system=='Windows'", | ||
| "numpy >=1.22; platform_system=='Linux'", | ||
| # Updated to numpy>=2.0 required for scikit-build-core C extension support | ||
| # Note: Keep consistent with .github/workflows/pytest.yaml | ||
| "numpy>=2.0", | ||
| "scipy>=1.11.1", | ||
| "importlib_resources", | ||
| "sympy", |
There was a problem hiding this comment.
Raised scipy minimum to 1.13.0 which added NumPy 2.x support.
| [project.optional-dependencies] | ||
|
|
||
| # Install with: pip install gbasis[dev] | ||
|
|
There was a problem hiding this comment.
fixed - updated package name from gbasis to qc-gbasis in install comment
| # Install with: pip install gbasis[doc] | ||
|
|
| include(CheckCCompilerFlag) | ||
| check_c_compiler_flag("-mavx2" HAS_AVX2) | ||
|
|
There was a problem hiding this comment.
Added Windows-specific AVX2 detection using /arch:AVX2 for MSVC compilers.
| if(HAS_AVX2 AND NOT CMAKE_SYSTEM_PROCESSOR MATCHES "arm") | ||
| set(USE_QCINT ON) | ||
| message(STATUS "Architecture: x86 + AVX2 detected → use qcint") | ||
| else() | ||
| set(USE_QCINT OFF) | ||
| message(STATUS "Architecture: ARM/non-AVX detected → use libcint") | ||
| endif() |
There was a problem hiding this comment.
Added user override support — if USE_QCINT is explicitly set by the user, auto-detection is skipped.
| # Link C extension with libcint | ||
| target_link_libraries(libcint_bindings PRIVATE cint) | ||
|
|
||
| # Prevent libcint from installing to system directories | ||
| set(CINT_INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/cint_install CACHE PATH "" FORCE) | ||
|
|
||
| # Install only the Python extension into the wheel | ||
| install(TARGETS libcint_bindings | ||
| LIBRARY DESTINATION ${SKBUILD_PLATLIB_DIR}/gbasis/integrals/lib | ||
| ) No newline at end of file |
There was a problem hiding this comment.
This will be addressed in the next PR which adds the platform-aware libcint shared library copying alongside the extension.
|
|
||
| /* libcint Python/C API bindings | ||
| * PR 3 mein implement hoga | ||
| */ No newline at end of file |
There was a problem hiding this comment.
This is just a placeholder file in PR 1. The full implementation including PyInit_libcint_bindings will be added in the follow-up PR.
| @@ -33,7 +33,7 @@ jobs: | |||
| if: ${{ matrix.py == '3.9'}} | |||
| # Ensure these versions are consistent with the minimal version requirements | |||
| # in pyproject.toml | |||
| run: pip install numpy==1.22 scipy==1.11.1 | |||
| run: pip install numpy==2.0 scipy==1.11.1 | |||
There was a problem hiding this comment.
Fixed — bumped scipy to 1.13.0 in pytest.yaml to match pyproject.toml minimum.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@msricher Created san1357:dev branch from theochem:master. |
|
OK, fixes look good. Please merge this into your dev branch and we'll continue from there with the next PR. |
PaulWAyers
left a comment
There was a problem hiding this comment.
I'd like @msricher or @marco-2023 to do a detailed review and final approval but it is OK for me.
I'm fine with it, it's just boilerplate he's going to merge into his dev branch,. |
PaulWAyers
left a comment
There was a problem hiding this comment.
@San1357 you can merge it now, then.
|
@msricher, @PaulWAyers, @San1357 if it is going to be merged in dev, it would need to be another PR. This one is trying to merge on main. |
|
As suggested by @marco-2023, I've created a new PR targeting san1357:dev: San1357#1 The changes have been successfully merged into the dev branch. |
|
@msricher @marco-2023 @PaulWAyers For the next PR, I'll create them targeting san1357:dev on my fork. I can add you as collaborators so you can review them there. Would that work, or do you prefer a different approach? |
|
Sure, that works for me. |
Summary
I migrated the build system from setuptools to scikit-build-core.
In the existing pyproject.toml, setuptools was being used — but setuptools cannot compile C code. So I replaced it with
scikit-build-core — because scikit-build-core can call CMake, and CMake will compile libcint.
I referred to the repo matrix-permanent project for this — it already had the same
scikit-build-core setup. so I followed the same structure.
Added CMakeLists.txt with FetchContent for automatic libcint/qcint download
Platform detection: x86+AVX2 → qcint (optimized), ARM/non-AVX → libcint (generic)
Referenced matrix-permanent repo and Libcint 6 paper (Section IV) for structure and compile flags
Part of Issue GSoC 2026: Better linking between
LibcintandGBasis#229 (PR-1)[Week-1]Description:
How To Test
Tested locally on Mac M5 (ARM):
Prerequisites:
Build locally:
pip install -e . --no-build-isolation -vExpected output during build:
Verify installation:
python -c "import gbasis; print('gbasis import successful')"Expected output:
IMPORTANT NOTES: