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] Change build settings to set strict-config to false #6493

Conversation

Jorricks
Copy link
Contributor

@Jorricks Jorricks commented Jun 20, 2024

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.

pip install -e . --config-settings editable_mode=strict

(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 the editable_mode=strict due to the strict-config = true.

ERROR: Unrecognized options in config-settings:
editable_mode -> Did you mean: editable?

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.

Copy link
Collaborator

@borchero borchero left a 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?

Copy link
Collaborator

@jameslamb jameslamb left a 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:

variables:

and here:

and here:

(that approach is documented at https://scikit-build-core.readthedocs.io/en/latest/configuration.html#other-options)

@Jorricks
Copy link
Contributor Author

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.

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.

@Jorricks
Copy link
Contributor Author

@microsoft-github-policy-service agree

@jameslamb jameslamb self-requested a review June 21, 2024 01:52
Copy link
Collaborator

@jameslamb jameslamb left a 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.

@jameslamb jameslamb merged commit d88dc49 into microsoft:master Jun 21, 2024
41 checks passed
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

3 participants