-
Notifications
You must be signed in to change notification settings - Fork 38
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
base: main
Are you sure you want to change the base?
Conversation
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]>
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]>
@mgovers this is in principle ready to review. But wait until many developers test in their local build environment to merge. |
There was a problem hiding this 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).
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]>
In latest In this PR, the MacOS Python build took 92.9s: 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. |
Signed-off-by: Tony Xiang <[email protected]>
Signed-off-by: Tony Xiang <[email protected]>
|
There was a problem hiding this 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 removedsetup.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) |
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" |
There was a problem hiding this comment.
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.
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.
This is an attempt to use
scikit-build-core
to replacesetuptools
as build backend for Python. It uses nativecmake
to build the shared object.check list
Conda forge test in:
conda-forge/power-grid-model-feedstock#389
local build test guide
prepare
build
folder if it exists.test build
Test build to sdist and wheel using
build
.test editable install
Test build in-place for editable install.