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

[python-package] upgrade to scikit-build-core 0.9.3 #6263

Merged
merged 12 commits into from
May 8, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Jan 10, 2024

Proposes upgrading scikit-build-core, 0.4.4 -> 0.9.3 (the latest version).

That new version preserves support for python >= 3.7 and lots of great changes have been made since v0.4.4, including:

For full release notes, see https://github.com/scikit-build/scikit-build-core/releases

@jameslamb jameslamb changed the title WIP: [python-package] upgrade to scikit-build-core 0.7.1 [python-package] upgrade to scikit-build-core 0.7.1 Jan 10, 2024
@jameslamb jameslamb marked this pull request as ready for review January 10, 2024 05:07
@jameslamb
Copy link
Collaborator Author

Moving this back to draft and undoing your approval @guolinke . I see some builds failed and this might need to be reworked.

Will re-open it and request reviews when it's ready. Sorry!

@jameslamb jameslamb requested a review from guolinke April 3, 2024 18:02
@jameslamb jameslamb marked this pull request as draft April 3, 2024 18:02
@jameslamb jameslamb changed the title [python-package] upgrade to scikit-build-core 0.7.1 WIP: [python-package] upgrade to scikit-build-core 0.7.1 Apr 3, 2024
@jameslamb jameslamb changed the title WIP: [python-package] upgrade to scikit-build-core 0.7.1 WIP: [python-package] upgrade to scikit-build-core 0.9.3 May 8, 2024
ninja.make-fallback = true
cmake.args = [
"-D__BUILD_FOR_PYTHON:BOOL=ON"
]
cmake.verbose = false
cmake.build-type = "Release"
cmake.targets = ["_lightgbm"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ensures that only the shared library is built, and not other targets like the CLI or headers.

scikit-build-core didn't support that back when we did #5759, which is why this exists:

LightGBM/CMakeLists.txt

Lines 54 to 60 in 88cec47

if(__BUILD_FOR_PYTHON OR __BUILD_FOR_R)
# the Python and R package don't require the CLI
set(BUILD_CLI OFF)
# installing the R and Python package shouldn't place LightGBM's headers
# outside of where the package is installed
set(INSTALL_HEADERS OFF)
endif()

I'd like to leave that in CMakeLists.txt as extra protection for now. It's only a tiny bit of complexity in exchange for protection against some potentially-impactful side effects (like pip install of an sdist placing LightGBM's headers somewhere outside of where pip manages files).

cmake.targets = ["_lightgbm"]
# stripping binaries should be turned back on once this is fixed:
# https://github.com/jameslamb/pydistcheck/issues/235
install.strip = false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See jameslamb/pydistcheck#235 for a full description of this.

Short description: it's great that scikit-build-core strips binaries by default now, but we don't need it in LightGBM. And pydistcheck's check on debug symbols (which runs in CI) gives us confidence that no debug symbols sneak into compiled objects in wheels.

@jameslamb jameslamb changed the title WIP: [python-package] upgrade to scikit-build-core 0.9.3 [python-package] upgrade to scikit-build-core 0.9.3 May 8, 2024
@jameslamb
Copy link
Collaborator Author

Ok I think is working now and is ready for another review whenever you have time, @guolinke . Sorry it took so long to get back to it.

@jameslamb jameslamb marked this pull request as ready for review May 8, 2024 21:08
@jameslamb
Copy link
Collaborator Author

Thank you!

@jameslamb jameslamb merged commit 20f2092 into master May 8, 2024
38 checks passed
@jameslamb jameslamb deleted the newer-scikit-build-core branch May 8, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants