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

move most project metadata into pyproject.toml #126

Merged
merged 17 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/github-actions.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

@jameslamb jameslamb Jul 26, 2024

Choose a reason for hiding this comment

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

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:

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.

- name: Run cpu tests
run: |
legate --sysmem 28000 --module pytest legateboost/test/[!_]**.py -sv --durations=0
Expand Down
12 changes: 9 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,13 @@ model = lb.LBRegressor(base_models=(lb.models.KRR(sigma=0.5), lb.models.Tree(max

## Installation

From the project directory
```
pip install .
If you already have `cunumeric` and `legate-core` installed, run the following:

```shell
pip install \
--no-build-isolation \
--no-deps \
.
```

For more details on customizing the build and setting up a development environment, see [`contributing.md`](./contributing.md).
38 changes: 36 additions & 2 deletions build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,33 @@

set -e -u -o pipefail

NUMARGS=$#
ARGS=$*

HELP="build liblegateboost.so and a 'legateboost' Python wheel, and install that wheel

$0 [<flag> ...]

where <flag> is any of:

--editable install Python wheel in editable mode
"

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

Choose a reason for hiding this comment

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

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.


if hasArg -h || hasArg --help; then
echo "${HELP}"
exit 0
fi

# Set defaults for vars modified by flags to this script
PIP_INSTALL_ARGS=(
--no-build-isolation
--no-deps
)

# ensure 'native' is used if CUDAARCHS isn't set
# (instead of the CMake default which is a specific architecture)
# ref: https://cmake.org/cmake/help/latest/variable/CMAKE_CUDA_ARCHITECTURES.html
Expand All @@ -10,7 +37,14 @@ declare -r CMAKE_CUDA_ARCHITECTURES="${CUDAARCHS:-native}"
legate_root=$(
python -c 'import legate.install_info as i; from pathlib import Path; print(Path(i.libpath).parent.resolve())'
)
echo "Using Legate at $legate_root"
echo "Using Legate at '${legate_root}'"

cmake -S . -B build -Dlegate_core_ROOT="${legate_root}" -DCMAKE_BUILD_TYPE=Release -DCMAKE_CUDA_ARCHITECTURES="${CMAKE_CUDA_ARCHITECTURES}"
cmake --build build -j
python -m pip install -e .

if hasArg --editable; then
PIP_INSTALL_ARGS+=("--editable")
fi

echo "building legatboost Python package..."
jameslamb marked this conversation as resolved.
Show resolved Hide resolved
python -m pip install "${PIP_INSTALL_ARGS[@]}" .
2 changes: 1 addition & 1 deletion ci/build_wheel.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

set -e -u -o pipefail

./build.sh
python -m build \
--no-isolation \
--skip-dependency-check \
--wheel \
--outdir dist
6 changes: 3 additions & 3 deletions conda/environments/all_cuda-118.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ channels:
- conda-forge
dependencies:
- cmake>=3.24.0,!=3.30.0
- cunumeric=24.06.*
- cunumeric==24.06.*
- hypothesis
- legate-core=24.06.*
- legate-core==24.06.*
- matplotlib
- mypy
- myst-parser
Expand All @@ -17,7 +17,7 @@ dependencies:
- openblas
- pydata-sphinx-theme
- pytest<8
- python-build
- python-build>=1.2.0
- scikit-build>=0.18.0
- scikit-learn
- seaborn
Expand Down
45 changes: 37 additions & 8 deletions contributing.md
Original file line number Diff line number Diff line change
@@ -1,17 +1,46 @@
# Contributing to legateboost

For editable installation
```
pip install -e .
```
To include test dependencies
`legateboost` depends on some libraries that are not easily installable with `pip`.

Use `conda` to create a development environment that includes them.

```shell
# CUDA 11.8
conda env create \
--name legate-boost-dev \
-f ./conda/environments/all_cuda-118.yaml

source activate legate-boost-dev
```
pip install -e .[test]

The easiest way to develop is to compile the shared library separately, then build
and install an editable wheel that uses it.

```shell
./build.sh --editable
```

## Running tests

CPU:

```shell
legate \
--sysmem 28000 \
--module pytest \
legateboost/test
```
legate --module pytest legateboost/test

GPU:

```shell
legate \
--gpus 1 \
--fbmem 28000 \
--sysmem 28000 \
--module pytest \
legateboost/test/test_estimator.py \
-k 'not sklearn'
```

## Change default CUDA architectures
Expand All @@ -22,7 +51,7 @@ If installing with `pip`, set the `CUDAARCHS` environment variable, as described

```shell
CUDAARCHS="70;80" \
pip install .
pip install --no-build-isolation --no-deps .
```

For CMake-based builds, pass `CMAKE_CUDA_ARCHITECTURES`.
Expand Down
43 changes: 36 additions & 7 deletions dependencies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,32 @@ files:
cuda: ["11.8"]
includes:
- build
- build_tools
- docs
- run
- test
py_build:
output: pyproject
pyproject_dir: .
extras:
table: build-system
includes:
- build
py_run:
output: pyproject
pyproject_dir: .
extras:
table: project
includes:
- run
py_test:
output: pyproject
pyproject_dir: .
extras:
table: project.optional-dependencies
key: test
includes:
- test
channels:
- legate
- conda-forge
Expand All @@ -17,31 +40,37 @@ dependencies:
common:
- output_types: [conda]
packages:
- cmake>=3.24.0,!=3.30.0
- &legate_core legate-core=24.06.*
- openblas
- python-build
- output_types: [conda, pyproject, requirements]
packages:
- cmake>=3.24.0,!=3.30.0
- &legate_core legate-core==24.06.*
- scikit-build>=0.18.0
- setuptools>=70.0
docs:
build_tools:
common:
- output_types: [conda]
packages:
- python-build>=1.2.0
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

docs:
common:
- output_types: [conda, pyproject, requirements]
packages:
- sphinx
- pydata-sphinx-theme
- myst-parser
run:
common:
- output_types: [conda]
- output_types: [conda, pyproject, requirements]
packages:
- cunumeric=24.06.*
- cunumeric==24.06.*
Copy link
Member Author

Choose a reason for hiding this comment

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

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

- *legate_core
- numpy
- scikit-learn
- typing-extensions>=4.0
test:
common:
- output_types: [conda]
- output_types: [conda, pyproject, requirements]
packages:
- hypothesis
- matplotlib
Expand Down
47 changes: 47 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
[build-system]
requires = [
"cmake>=3.24.0,!=3.30.0",
"legate-core==24.06.*",
"scikit-build>=0.18.0",
"setuptools>=70.0",
] # This list was generated by `rapids-dependency-file-generator`. To make changes, edit dependencies.yaml and run `rapids-dependency-file-generator`.
build-backend = "setuptools.build_meta"

[project]
name = "legateboost"
version = "0.1"
authors = [
{name = "NVIDIA Corporation"},
]
classifiers = [
"Intended Audience :: Developers",
"Topic :: Database",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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",
],

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
"Topic :: Database",

Let's remove it.

"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",
]
dependencies = [
"cunumeric==24.06.*",
"legate-core==24.06.*",
"numpy",
"scikit-learn",
"typing-extensions>=4.0",
] # This list was generated by `rapids-dependency-file-generator`. To make changes, edit dependencies.yaml and run `rapids-dependency-file-generator`.
description = "GBM libary on Legate"
license = {text = "Apache 2.0"}
requires-python = ">=3.8"

[project.optional-dependencies]
test = [
"hypothesis",
"matplotlib",
"mypy",
"nbconvert",
"notebook",
"pytest<8",
"seaborn",
"xgboost",
] # This list was generated by `rapids-dependency-file-generator`. To make changes, edit dependencies.yaml and run `rapids-dependency-file-generator`.
38 changes: 0 additions & 38 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,46 +39,8 @@
cmake_flags.append(env_cmake_args)
os.environ["CMAKE_ARGS"] = " ".join(cmake_flags)

requires = [
"cunumeric",
"legate-core",
"scikit-learn",
"scipy",
"numpy",
"typing_extensions", # Required by legate.core as well.
]

extras_require = {
"test": [
"hypothesis",
"pytest<8",
"xgboost",
"notebook",
"nbconvert",
"seaborn",
"matplotlib",
"mypy",
]
}

setup(
name="legateboost",
version="0.1",
description="GBM libary on Legate",
author="NVIDIA Corporation",
license="Apache 2.0",
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",
],
install_requires=requires,
extras_require=extras_require,
packages=find_packages(
where=".",
include=["legateboost", "legateboost.*"],
Expand Down
Loading