jameslamb
commented
3 weeks ago I think that this is mostly working and ready for review!
Opening it up and tagging reviewers... I need some help.
All of the tests using the GPU variant, on a system with a GPU, are passing (build link) 🎉
However, several tests are failing with the CPU variant, on a CPU-only system.
24 failed, 163 passed, 7 skipped, 2 xfailed, 6 xpassed, 2738 warnings in 350.72s(build link).
Those tests are mostly failing like this:
E IndexError: Library legateboost does not have task 10There are more detailed stacktraces available at that link.
If it helps, here's one specific test case that's failing:
legate \
--sysmem 28000 \
--module pytest \
'legateboost/test/test_estimator.py::test_regressor[base_models3-squared_error-1]' \
-sv \
--durations=0full trace (click me)
```text ____________________________________________________________________________ test_regressor[base_models3-squared_error-1] ____________________________________________________________________________ num_outputs = 1, objective = 'squared_error', base_models = (And a smaller reproducible example:
import numpy as np
import legateboost as lb
np.random.seed(2)
X = np.random.random((100, 10))
y = np.random.random((X.shape[0], 1))
eval_result = {}
model = lb.LBRegressor(
n_estimators=20,
objective="squared_error",
random_state=2,
learning_rate=0.1,
base_models=(lb.models.KRR(),),
).fit(X, y, eval_result=eval_result)Here's where in legate-core that error message comes from:
jakirkham
trivialfis
manopapad
RAMitchell
Contributes to #115
Adds conda builds.
Notes for Reviewers
This is not even close to ready for review. Just putting it up here to show the direction I'm going. This diff will get smaller as other PRs are merged.
Design choices
using GitHub Artifact store instead of RAPIDS S3 bucket
details (click me)
RAPIDS is looking to eventually move away from its S3-based strategy for storing and hosting CI artifacts. I wanted to use this as a test case for that. GitHub Artifacts is also nice because, unlike https://downloads.rapids.ai/, it's on the public internet... so any outside-of-NVIDIA contributors can see their CI artifacts.building
*_gpuand*_cpu*variantsdetails (click me)
`legate-core` and `cunumeric` do something very similar. `legate-core` / `cunumeric` did not want to add a `*_gpu` portion to the build string when I proposed it, because they considered changing the build string format to be an unacceptable breaking change. Since these packages for `legate-boost` are brand new, I think we *should* do that here, to achieve the following semantics: ```shell # only installs the GPU variant (or fails if it's not installable) conda install legate-core=24.06=*_gpu # only installs the CPU variant (or fails if it's not installable) conda install legate-core=24.06=*_cpu # installs the GPU variant if CUDA is detected (look for '__cuda' in the output of 'conda info'), # otherwise installs the CPU variant conda install legate-core ```Building and testing in separate environments
details (click me)
On `main`, this project's CI is building and testing in the same environment. This can lead to issues like those described in #144, where packaging problems are silently missed. Building in one place and testing in another has a few nice benefits: * less GPU runner utilization - *builds happen on CPU-only runner* * CI is more likely to be able to detect packaging problems - *tests just install pre-built packages and all the dependencies they declare... could catch things like "this package needs `scipy` at runtime but it isn't in the package dependencies"* * greater confidence in CPU support - *CPU tests run on a machine that doesn't have CUDA or a GPU* - *installation of the CPU-only variant tested there too* * tests can run concurrently - *CPU tests and GPU tests run in physically separate instances, so can happen at the same time, which increases the amount of testing that can be done for any fixed amount of CI time*How to test this locally
See the notes added to
contributing.mdin the diff here (which also includes more explanation of these commands).In short: