-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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] Change build settings to set strict-config to false #6493
[python-package] Change build settings to set strict-config to false #6493
Conversation
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'd be fine with it, wdyt @jameslamb?
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.
There is no reason to throw errors on flags it does not understand, as it does not impact the compilation.
This is not correct.
The purpose of setting this to true
is to catch exactly cases like this, where you're passing through a flag to the build backend that doesn't actually affect the build. With that set to false
, the behavior becomes just silently ignoring it... in your case, maybe thinking you got an editable installation of lightgbm
but not actually getting one, because you're passing a flag meant for setuptools
through to `scikit-build-core.
That's been useful here, especially during active development changing how the Python library is packaged, to catch issues like typos in config settings.
At this stage, I'm ok with changing the value that for this in pyproject.toml
as you've proposed, but let's please preserve that strictness in LightGBM's CI.
Please set environment variable SKBUILD_STRICT_CONFIG=true
here:
Line 10 in 5cd95a5
variables: |
and here:
env: |
and here:
LightGBM/.github/workflows/cuda.yml
Line 66 in 5cd95a5
env: |
(that approach is documented at https://scikit-build-core.readthedocs.io/en/latest/configuration.html#other-options)
…- Set only true for CI/CD
Good point. I was purely looking from a 'user' perspective but of course during development it helps tremendously if you are directly notified about inconsistencies or misspellings. Thank you for pointing out the locations I needed to add the environment variable. Saved me a lot of time :). Also thanks for being flexible to directly consider the change. |
@microsoft-github-policy-service agree |
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.
thanks very much.
It will likely be a while (on the order of months) until the next release of lightgbm
. So for your builds where you're unconditionally passing setuptools
config settings through to library installs, you'll still need to pass -C skbuild.strict-config=false
.
Context
Setuptools recently introduced a new way for editable installations. Unfortunately, that does not work well with many tools at this moment in time, nor does it work well with a mono-repo. For that reason we choose to stick with the old way of doing editable installations which is shown here on setuptools docs.
(You will most likely see this appear on more places as people upgrade
pip
versions and the new way of editable installations becomes the default.)Our code-base, of which we have 20+ different teams with different ML tooling, works as expected with this approach. Unfortunately,
lightgbm
is the exception.In this MR
lightgbm
throws an error on receiving theeditable_mode=strict
due to the strict-config = true.There is no reason to throw errors on flags it does not understand, as it does not impact the compilation. Therefore I propose to set this value to
False
.