Skip to content

use scikit-build-core as build backend #1019

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

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

Conversation

TonyXiang8787
Copy link
Member

@TonyXiang8787 TonyXiang8787 commented Jun 15, 2025

This is an attempt to use scikit-build-core to replace setuptools as build backend for Python. It uses native cmake to build the shared object.

check list

  • Modify build backend
  • Configure Python
  • Modify CI
  • Adjust project URL
  • Adjust build guide
  • Test conda build in conda-forge
  • Test local build for all local dev environment (two persons per environment)

Conda forge test in:

conda-forge/power-grid-model-feedstock#389

local build test guide

prepare

  1. Check-out this branch.
  2. Remove the existing virtual environment, create a new one.
  3. Remove build folder if it exists.
  4. If you are in Windows, you have to conduct the remaining test in MSVC developer console.

test build

Test build to sdist and wheel using build.

pip install build
python -m build .
pip install dist/*.whl pytest pytest-cov msgpack
pytest

pip uninstall power-grid-model
pip install dist/*.tar.gz
pytest

test editable install

Test build in-place for editable install.

pip uninstall power-grid-model
pip install -e .[dev]
pytest

Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
@TonyXiang8787 TonyXiang8787 self-assigned this Jun 15, 2025
@TonyXiang8787 TonyXiang8787 added improvement Improvement on internal implementation do-not-merge This should not be merged labels Jun 15, 2025
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
@TonyXiang8787 TonyXiang8787 marked this pull request as ready for review June 16, 2025 12:51
@TonyXiang8787 TonyXiang8787 requested a review from mgovers June 16, 2025 12:52
@TonyXiang8787
Copy link
Member Author

@mgovers this is in principle ready to review. But wait until many developers test in their local build environment to merge.

Copy link
Contributor

@figueroa1395 figueroa1395 left a comment

Choose a reason for hiding this comment

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

I tested in MacOS with a conda (mini-forge) env and everything works well. I also reviewed conda-forge/power-grid-model-feedstock#389 and everything seems fine.

As a side note for others testing locally, I would recommend using python -m build . --verbose to build, as it takes some time and otherwise you'd be left in blank.

Hereby I give my approval to the new workflow (waiting for others to review before marking approved on the PR itself).

@figueroa1395
Copy link
Contributor

One additional thing, is it just my impression or the build time seems to be considerably longer with the new workflow?

Signed-off-by: Tony Xiang <[email protected]>
@TonyXiang8787
Copy link
Member Author

One additional thing, is it just my impression or the build time seems to be considerably longer with the new workflow?

@figueroa1395

In latest main, the MacOS Python build took 83.9s:
https://github.com/PowerGridModel/power-grid-model/actions/runs/15701285970/job/44236484805

In this PR, the MacOS Python build took 92.9s:
https://github.com/PowerGridModel/power-grid-model/actions/runs/15714477992/job/44280706230?pr=1019

So it is not considerably longer. We can also enable parallel build (in cmake/ninja) for local develop environment. This should speed up a lot.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
74.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@TonyXiang8787 TonyXiang8787 requested a review from Copilot June 26, 2025 08:46
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces setuptools with scikit-build-core as the Python build backend, centralizes DLL lookup into a new helper, and updates CI, docs, and packaging accordingly.

  • Switched build backend to scikit-build-core and removed setup.py
  • Added get_pgm_dll_path for consistent dynamic library loading
  • Updated GitHub Actions workflows, conda env, and documentation for the new build system

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/power_grid_model/power_grid_model_c/get_pgm_dll_path.py New helper to locate and prioritize PGM dynamic libraries
src/power_grid_model/_core/power_grid_core.py Replaced manual DLL search with get_pgm_dll_path()
pyproject.toml Updated build-system requirements/backend and scikit-build settings
.github/workflows/build-test-release.yml Adapted version file handling and pip install flags
docs/advanced_documentation/build-guide.md Updated dependency table to include scikit-build-core
MANIFEST.in Removed in favor of scikit-build-core defaults
cmake/pgm_version.cmake Enhanced regex-based version extraction
.github/conda_pgm_env.yml Updated conda env to install scikit-build-core
.github/actions/enable-msvc/action.yml New composite action to set up MSVC on runners
Comments suppressed due to low confidence (4)

.github/workflows/build-test-release.yml:281

  • The -C flag is not a valid pip install option and will be interpreted as a constraints file. Consider using the --config-settings option to pass build backend settings (e.g., --config-settings "wheel.cmake=false").
        run: python -m pip install . --no-build-isolation --no-deps -C wheel.cmake=false

src/power_grid_model/power_grid_model_c/get_pgm_dll_path.py:10

  • Consider adding unit tests for get_pgm_dll_path to verify platform-specific paths, the editable-build fallback, and ordering of lookup candidates.
def get_pgm_dll_path() -> Path:

docs/advanced_documentation/build-guide.md:95

  • It would be helpful to add a section or note explaining how to invoke scikit-build-core for building the project (e.g., required CLI commands or entry-point usage).
| [scikit-build-core](https://github.com/scikit-build/scikit-build-core)                                                  | build dependency         | [Apache](https://github.com/scikit-build/scikit-build-core/blob/main/LICENSE)                                 |

Comment on lines +16 to +20
lib_dir_1 = package_dir / "bin"
lib_dir_2 = package_dir / "bin"
else:
lib_dir_1 = package_dir / "lib"
lib_dir_2 = package_dir / "lib64"
Copy link
Preview

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

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

On Windows both lib_dir_1 and lib_dir_2 are set to the same path. You could simplify this by iterating over a single list of candidate directories instead of duplicating assignments.

Suggested change
lib_dir_1 = package_dir / "bin"
lib_dir_2 = package_dir / "bin"
else:
lib_dir_1 = package_dir / "lib"
lib_dir_2 = package_dir / "lib64"
lib_dirs = [package_dir / "bin"]
else:
lib_dirs = [package_dir / "lib", package_dir / "lib64"]

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge This should not be merged improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants