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] remove setup.cfg #6624

Merged
merged 1 commit into from
Aug 27, 2024
Merged

[python-package] remove setup.cfg #6624

merged 1 commit into from
Aug 27, 2024

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Aug 27, 2024

I don't think python-package/setup.cfg serves any purpose in this project any more. This proposes removing it it.

Notes for Reviewers

How I tested this

The configuration in setup.cfg doesn't affect scikit-build-core based builds (see docs here), so I just tested the --precompile option where a setuptools build backend is substituted in:

LightGBM/build-python.sh

Lines 308 to 316 in a9df7f1

# use regular-old setuptools for these builds, to avoid
# trying to recompile the shared library
sed -i.bak -e '/start:build-system/,/end:build-system/d' pyproject.toml
echo '[build-system]' >> ./pyproject.toml
echo 'requires = ["setuptools"]' >> ./pyproject.toml
echo 'build-backend = "setuptools.build_meta"' >> ./pyproject.toml
echo "" >> ./pyproject.toml
echo "recursive-include lightgbm *.dll *.dylib *.so" > ./MANIFEST.in
echo "" >> ./MANIFEST.in

cmake -B build -S .
cmake --build build --target _lightgbm -j4
sh build-python.sh install --precompile

Saw the unit tests pass

pytest tests/python_package_test

And the package contained the expected files:

tar -tf ./dist/*.tar.gz
list of files (click me)
lightgbm-4.5.0.99/
lightgbm-4.5.0.99/LICENSE
lightgbm-4.5.0.99/MANIFEST.in
lightgbm-4.5.0.99/PKG-INFO
lightgbm-4.5.0.99/README.rst
lightgbm-4.5.0.99/lightgbm/
lightgbm-4.5.0.99/lightgbm/__init__.py
lightgbm-4.5.0.99/lightgbm/basic.py
lightgbm-4.5.0.99/lightgbm/callback.py
lightgbm-4.5.0.99/lightgbm/compat.py
lightgbm-4.5.0.99/lightgbm/dask.py
lightgbm-4.5.0.99/lightgbm/engine.py
lightgbm-4.5.0.99/lightgbm/lib/
lightgbm-4.5.0.99/lightgbm/lib/lib_lightgbm.dylib
lightgbm-4.5.0.99/lightgbm/libpath.py
lightgbm-4.5.0.99/lightgbm/plotting.py
lightgbm-4.5.0.99/lightgbm/py.typed
lightgbm-4.5.0.99/lightgbm/sklearn.py
lightgbm-4.5.0.99/lightgbm.egg-info/
lightgbm-4.5.0.99/lightgbm.egg-info/PKG-INFO
lightgbm-4.5.0.99/lightgbm.egg-info/SOURCES.txt
lightgbm-4.5.0.99/lightgbm.egg-info/dependency_links.txt
lightgbm-4.5.0.99/lightgbm.egg-info/requires.txt
lightgbm-4.5.0.99/lightgbm.egg-info/top_level.txt
lightgbm-4.5.0.99/pyproject.toml
lightgbm-4.5.0.99/setup.cfg

@jameslamb jameslamb marked this pull request as ready for review August 27, 2024 06:04
@jameslamb jameslamb changed the title WIP: [python-package] remove setup.cfg [python-package] remove setup.cfg Aug 27, 2024
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM! Love such types of PRs! 😄

Just out of curiosity, did you checked the content of generated setup.cfg?

list of files (click me)
...
lightgbm-4.5.0.99/setup.cfg

@StrikerRUS StrikerRUS merged commit fde0157 into master Aug 27, 2024
44 checks passed
@StrikerRUS StrikerRUS deleted the install-prefix branch August 27, 2024 08:54
@jameslamb
Copy link
Collaborator Author

Just out of curiosity, did you checked the content of generated setup.cfg?

Wow I hadn't even noticed that! Ha ok I was curious, so I checked.

cmake -B build -S .
cmake --build build --target _lightgbm -j4
sh build-python.sh install --precompile

cd ./dist
tar -xvzf *.tar.gz
lightgbm-4.5.0.99/setup.cfg

It just contains this:

[egg_info]
tag_build = 
tag_date = 0

Looks like that comes from somewhere around here in setuptools: https://github.com/pypa/setuptools/blob/738dd480ed123161b34a9ee648a61841f7e9e0ad/setuptools/command/egg_info.py#L208

@StrikerRUS
Copy link
Collaborator

Thanks for checking this! Now we know the normal standard behaviour for setup.cfg.

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.

2 participants