move most project metadata into pyproject.toml #126

jameslamb · 2024-07-25T19:42:57Z

Contributes to #115

Proposes moving most project metadata into pyproject.toml.

Notes for Reviewers

I found this helpful while working on this: https://scikit-build.readthedocs.io/en/latest/usage.html

Benefits of these changes

allows dependency lists to automatically be managed via rapids-dependency-file-generator (and single-sourced from dependencies.yaml)
will help reduce duplication between metadata for wheels and for conda packages
- e.g. like this: https://github.com/rapidsai/rapids-build-backend/blob/cb2dedbb8532739822378811e4adbab06514f582/conda/recipes/rapids-build-backend/meta.yaml#L23-L25

Why can't we completely eliminate `setup.py`?

I think it's possible and I'm planning to try it, just trying to do this in small-enough-to-thoughtfully-review chunks.

How I tested this

On a machine with CUDA 12.2 (R535 driver) and 8 V100s, I tried all the different installation and testing paths described in README.md and contributing.md.

I also built a wheel like this

ci/build_wheel.sh

And manually inspected it like this:

# list all the contents
unzip -l dist/*.whl

# check the metadata looked right
pkginfo --simple dist/*.whl

# check for portability problems
pydistcheck --inspect dist/*.whl

(pydistcheck (link) is my little side-project for testing the contents of Python package distributions)

jameslamb · 2024-07-26T16:23:38Z

.github/workflows/github-actions.yml

@@ -62,6 +62,9 @@ jobs:
        with:
          name: legateboost-wheel
          path: dist/legateboost*.whl
+      - name: Install legateboost
+        run: |
+          python -m pip install --no-deps dist/*.whl


Removing build.sh from ci/build_wheel.sh ensures that here in CI, we're driving the entire CMake build via Python packaging tools (python -m build, in this case). That ensures that the shared library is actually copied into the wheel, and that therefore the wheels could be installed on their own.

That's not the case today. Try downloading a recent wheel artifact, e.g. from https://github.com/rapidsai/legate-boost/actions/runs/10111717786

unzip legateboost-wheel.zip unzip -l

liblegateboost.so has not been getting packaged.

contents (click me)

Archive: legateboost-0.1-cp312-cp312-linux_x86_64.whl Length Date Time Name --------- ---------- ----- ---- 604 07-26-2024 13:38 legateboost/__init__.py 5162 07-26-2024 13:38 legateboost/callbacks.py 2682 07-26-2024 13:38 legateboost/input_validation.py 1148 07-26-2024 13:38 legateboost/install_info.py 34517 07-26-2024 13:38 legateboost/legateboost.py 3156 07-26-2024 13:38 legateboost/library.py 10209 07-26-2024 13:38 legateboost/metrics.py 19391 07-26-2024 13:38 legateboost/objectives.py 4635 07-26-2024 13:38 legateboost/shapley.py 2943 07-26-2024 13:38 legateboost/special.py 12535 07-26-2024 13:38 legateboost/utils.py 124 07-26-2024 13:38 legateboost/models/__init__.py 2464 07-26-2024 13:38 legateboost/models/base_model.py 7397 07-26-2024 13:38 legateboost/models/krr.py 4122 07-26-2024 13:38 legateboost/models/linear.py 4882 07-26-2024 13:38 legateboost/models/nn.py 8768 07-26-2024 13:38 legateboost/models/tree.py 0 07-26-2024 13:38 legateboost/test/__init__.py 1848 07-26-2024 13:38 legateboost/test/test_callbacks.py 6744 07-26-2024 13:38 legateboost/test/test_estimator.py 1703 07-26-2024 13:38 legateboost/test/test_examples.py 7510 07-26-2024 13:38 legateboost/test/test_metric.py 3787 07-26-2024 13:38 legateboost/test/test_objective.py 3100 07-26-2024 13:38 legateboost/test/test_shapley.py 3895 07-26-2024 13:38 legateboost/test/test_special.py 1845 07-26-2024 13:38 legateboost/test/test_utils.py 7832 07-26-2024 13:38 legateboost/test/test_with_hypothesis.py 1335 07-26-2024 13:38 legateboost/test/utils.py 0 07-26-2024 13:38 legateboost/test/models/__init__.py 2720 07-26-2024 13:38 legateboost/test/models/test_krr.py 4028 07-26-2024 13:38 legateboost/test/models/test_linear.py 2925 07-26-2024 13:38 legateboost/test/models/test_nn.py 1420 07-26-2024 13:38 legateboost/test/models/test_tree.py 790 07-26-2024 13:38 legateboost/test/models/utils.py 3669 07-26-2024 13:38 legateboost-0.1.data/data/lib/cmake/legateboost_python/legateboost_python-config-version.cmake 5276 07-26-2024 13:38 legateboost-0.1.data/data/lib/cmake/legateboost_python/legateboost_python-config.cmake 1312 07-26-2024 13:38 legateboost-0.1.data/data/lib/cmake/legateboost_python/legateboost_python-dependencies.cmake 4182 07-26-2024 13:38 legateboost-0.1.data/data/lib/cmake/legateboost_python/legateboost_python-targets.cmake 10174 07-26-2024 13:38 legateboost-0.1.dist-info/LICENSE 1063 07-26-2024 13:38 legateboost-0.1.dist-info/METADATA 99 07-26-2024 13:38 legateboost-0.1.dist-info/WHEEL 12 07-26-2024 13:38 legateboost-0.1.dist-info/top_level.txt 3909 07-26-2024 13:38 legateboost-0.1.dist-info/RECORD --------- ------- 205917 43 files

Because of the use of build.sh, this line was always hit during the build:

legate-boost/CMakeLists.txt

Line 17 in 157b427

find_package(legateboost QUIET)

Which I think led to it not being recompiled in the _skbuild directory, which I think is why it's not getitng included in wheels.

jameslamb · 2024-07-26T16:26:31Z

build.sh

+
+function hasArg {
+    (( NUMARGS != 0 )) && (echo " ${ARGS} " | grep -q " $1 ")
+}


I know some of this looks a little complicated, but it's all standard across most RAPIDS repos. You'll see something similar in raft, cuml, cudf, etc.

example from raft: https://github.com/rapidsai/raft/blob/branch-24.10/build.sh

I think consistency with all those projects is useful, as it makes it easier for anyone developing on them to also come help out here, and makes it easier to integrate the various RAPIDS build tools into this repo.

This build.sh file will eventually be the one that drives the build for conda packages.

jameslamb · 2024-07-26T16:40:14Z

dependencies.yaml

    common:
      - output_types: [conda]
+        packages:
+          - python-build>=1.2.0


Moving this to its own conda-specific section, so it doesn't end up in the [build-system] list of requirements. Something like pip install / pip wheel looks in that list to determine what needs to be installed to build a wheel... if you're using pip install / pip wheel to do that, then you don't need build.

jameslamb · 2024-07-26T16:40:52Z

dependencies.yaml

        packages:
-          - cunumeric=24.06.*
+          - cunumeric==24.06.*


pip doesn't recognized =, just ==. This change is necessary now that this entry is going to end up in pyproject.toml too.

pyproject.toml

build.sh

trivialfis · 2024-07-27T18:54:57Z

pyproject.toml

+]
+classifiers = [
+    "Intended Audience :: Developers",
+    "Topic :: Database",


Haven't looked into other items yet, but database seems weird.

I was surprised to see it here too, but wasn't confident enough in my understanding of this project to say that it was wrong. This list of classifiers was exactly copied from what's already in setup.py:

legate-boost/setup.py

Lines 70 to 79 in d4edf2d

classifiers=[

"Intended Audience :: Developers",

"Topic :: Database",

"Topic :: Scientific/Engineering",

"License :: OSI Approved :: Apache Software License",

"Programming Language :: Python",

"Programming Language :: Python :: 3.8",

"Programming Language :: Python :: 3.9",

"Programming Language :: Python :: 3.10",

],

Suggested change

"Topic :: Database",

Let's remove it.

jameslamb added 5 commits July 22, 2024 14:54

move most project metadata into pyproject.toml

0133485

move some configuration to pyproject.toml

176b10a

more changes

bf9bf3d

add a build.sh

ea3d6a3

merge main

5ca1cd0

jameslamb added breaking Introduces a breaking change improvement Improves an existing functionality labels Jul 25, 2024

jameslamb added 3 commits July 25, 2024 13:15

add a wheel-building script

b3e216c

put floors on build dependencies

d2d07ed

add some packaging artifacts to .gitignore

d0de49e

jameslamb mentioned this pull request Jul 25, 2024

isolate wheel-building in a shell script, run shellcheck #127

Merged

jameslamb added 4 commits July 25, 2024 14:26

clean up .gitignore

34ac8b5

update pip install docs

8e524d7

merge main

a79dc7a

linting

1c2e610

jameslamb commented Jul 26, 2024

View reviewed changes

remove docs link

4523435

jameslamb changed the title ~~WIP: move most project metadata into pyproject.toml~~ move most project metadata into pyproject.toml Jul 26, 2024

jameslamb marked this pull request as ready for review July 26, 2024 17:04

jameslamb requested review from RAMitchell and trivialfis July 26, 2024 17:04

jameslamb mentioned this pull request Jul 26, 2024

[DO NOT MERGE] WIP: use scikit-build-core, PEP 517 builds #119

Closed

jameslamb commented Jul 26, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

Update pyproject.toml

7f80bf0

jameslamb commented Jul 26, 2024

View reviewed changes

build.sh Outdated Show resolved Hide resolved

Update build.sh

de6d77a

trivialfis reviewed Jul 27, 2024

View reviewed changes

RAMitchell approved these changes Jul 29, 2024

View reviewed changes

trivialfis approved these changes Jul 29, 2024

View reviewed changes

jameslamb and others added 2 commits July 29, 2024 08:27

Update pyproject.toml

f927f49

Merge branch 'main' into pyproject-toml

c6bdc6e

jameslamb merged commit 726b3ae into rapidsai:main Jul 29, 2024
3 checks passed

jameslamb deleted the pyproject-toml branch July 29, 2024 14:33

jameslamb mentioned this pull request Jul 29, 2024

remove setup.cfg, move more configuration into pyproject.toml #131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move most project metadata into pyproject.toml #126

move most project metadata into pyproject.toml #126

jameslamb commented Jul 25, 2024 •

edited

Loading

jameslamb Jul 26, 2024 •

edited

Loading

jameslamb Jul 26, 2024

jameslamb Jul 26, 2024

jameslamb Jul 26, 2024

trivialfis Jul 27, 2024

jameslamb Jul 29, 2024

jameslamb Jul 29, 2024

	classifiers=[
	"Intended Audience :: Developers",
	"Topic :: Database",
	"Topic :: Scientific/Engineering",
	"License :: OSI Approved :: Apache Software License",
	"Programming Language :: Python",
	"Programming Language :: Python :: 3.8",
	"Programming Language :: Python :: 3.9",
	"Programming Language :: Python :: 3.10",
	],

move most project metadata into pyproject.toml #126

move most project metadata into pyproject.toml #126

Conversation

jameslamb commented Jul 25, 2024 • edited Loading

Notes for Reviewers

Benefits of these changes

Why can't we completely eliminate setup.py?

How I tested this

jameslamb Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

jameslamb Jul 26, 2024

Choose a reason for hiding this comment

jameslamb Jul 26, 2024

Choose a reason for hiding this comment

jameslamb Jul 26, 2024

Choose a reason for hiding this comment

trivialfis Jul 27, 2024

Choose a reason for hiding this comment

jameslamb Jul 29, 2024

Choose a reason for hiding this comment

jameslamb Jul 29, 2024

Choose a reason for hiding this comment

jameslamb commented Jul 25, 2024 •

edited

Loading

Why can't we completely eliminate `setup.py`?

jameslamb Jul 26, 2024 •

edited

Loading