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

WIP: [ci] [python-package] support setuptools<61 in build-python.sh (fixes #6665) #6681

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Oct 15, 2024

Fixes #6665

As described in #6665 (comment), build-python.sh install --precompile is broken on Ubuntu 22.04 because the setuptools that gets installed with the python3-* packages for that distribution is older than v61.0.0 (the first version that supported pyproject.toml).

To fix that, this proposes having build-python.sh write a setup.cfg file when option --precompile is passed to it.

Notes for Reviewers

This is temporary... but should be supported for at least a few years

...or until we can find a better way to support such installs, maybe by using build + more CMake to find the already-built lib_lightgbm.so.

Per https://ubuntu.com/about/release-cycle, Ubuntu 22.04 will receive "standard support" for 2.5+ more years.

This should be totally forward-compatible

Later setuptools versions prefer pyproject.toml to setup.cfg, and pyproject.toml takes precedent when both are found. From the release notes for setuptools==61.0.0 (link)

The values specified in pyproject.toml will take precedence over those specified in setup.cfg or setup.py.

How I tested this

Ran the following from the root of the repo, on an M2 Mac (arm64).

testing code (click me)
docker run \
    --rm \
    -v $(pwd):/opt/work \
    -w /opt/work \
    -it ubuntu:22.04 \
    bash

apt-get update -y
apt-get install -y --no-install-recommends \
    git \
    ca-certificates \
    curl \
    build-essential \
    libboost-dev \
    libboost-system-dev \
    libboost-filesystem-dev \
    ocl-icd-opencl-dev \
    python3-dev \
    python3-pip \
    python3-venv

CMAKE_VERSION="3.30.0"
curl -O -L \
    "https://github.com/Kitware/CMake/releases/download/v${CMAKE_VERSION}/cmake-${CMAKE_VERSION}-linux-$(arch).sh"
mkdir /opt/cmake
sh "cmake-${CMAKE_VERSION}-linux-$(arch).sh" --skip-license --prefix=/opt/cmake
ln -sf /opt/cmake/bin/cmake /usr/local/bin/cmake
cmake --version
# cmake version 3.30.0

cmake -B build -S . -DUSE_GPU=1
cmake --build build -j2

ln -sf /usr/bin/python3 /usr/bin/python

python -m pip install \
    -U \
    numpy \
    pandas \
    scipy \
    scikit-learn \
    setuptools

sh ./build-python.sh install --precompile

python -c "import lightgbm"
python ./examples/python-guide/simple_example.py

On master, that reproduces the error reported in #6665. On this branch, it succeeds.

@jameslamb jameslamb added the fix label Oct 15, 2024
@jameslamb jameslamb changed the title WIP: [ci] [python-package] support setuptools<61 in build-python.sh (fixes #6665) [ci] [python-package] support setuptools<61 in build-python.sh (fixes #6665) Oct 15, 2024
@jameslamb jameslamb changed the title [ci] [python-package] support setuptools<61 in build-python.sh (fixes #6665) WIP: [ci] [python-package] support setuptools<61 in build-python.sh (fixes #6665) Oct 15, 2024
echo "" >> ./setup.cfg
echo '[options]' >> ./setup.cfg
echo 'packages = lightgbm' >> ./setup.cfg
echo 'include_package_data = True' >> ./setup.cfg
Copy link
Collaborator

Choose a reason for hiding this comment

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

WDYT about having a "template" file (say, template_setup.cfg) with these lines which will be renamed (or its' content will be inserted somewhere) when needed? TBH, these echos are really hard to read 😬

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with you, I'm really unhappy with these.

I'd love to avoid checking any more files into source control for this purpose. What about using heredocs?

It would look a little weird in the script because we'd have to left-justify them (I think), but overall they'd be easier to read.

like this:

        # 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
cat > ./MANIFEST.in <<EOF
recursive-include lightgbm \*.dll \*.dylib \*.so
EOF
cat > ./setup.cfg <<EOF
[options]
packages = lightgbm
include_package_data = True
EOF
        mkdir -p ./lightgbm/lib
        if test -f ../lib_lightgbm.so; then
            echo "found pre-compiled lib_lightgbm.so"
            cp ../lib_lightgbm.so ./lightgbm/lib/lib_lightgbm.so
        elif test -f ../lib_lightgbm.dylib; then
            echo "found pre-compiled lib_lightgbm.dylib"
            cp ../lib_lightgbm.dylib ./lightgbm/lib/lib_lightgbm.dylib
        elif test -f ../Release/lib_lightgbm.dll; then
            echo "found pre-compiled Release/lib_lightgbm.dll"
            cp ../Release/lib_lightgbm.dll ./lightgbm/lib/lib_lightgbm.dll
        elif test -f ../windows/x64/DLL/lib_lightgbm.dll; then
            echo "found pre-compiled windows/x64/DLL/lib_lightgbm.dll"
            cp ../windows/x64/DLL/lib_lightgbm.dll ./lightgbm/lib/lib_lightgbm.dll
            cp ../windows/x64/DLL/lib_lightgbm.lib ./lightgbm/lib/lib_lightgbm.lib
        fi
        rm -f ./*.bak

That'd be a short-term fix... longer-term, I think we could use CMake to get all this stuff out of this script, like having something of this form:

find_library(lightgbm)
if(lightgbm_FOUND)
   return()
endif()

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using heredocs?

Looks much better than echos 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[python-package] UNKNOWN python package
2 participants