rapidsai / legate-boost

GBM implementation on Legate
https://rapidsai.github.io/legate-boost/
Apache License 2.0
8 stars 6 forks source link

ensure tests use installed library #144

Closed jameslamb closed 1 month ago

jameslamb commented 1 month ago

Contributes to #115

Proposes the following:

Notes for Reviewers

Trying to test conda packages on #129, I found that running something like legate --module pytest legateboost/test was failing with errors like this:

ImportError while importing test module '/raid/jlamb/repos/legate-boost/legateboost/test/utils.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../miniforge/envs/legate-boost-dev-delete-me/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
legateboost/__init__.py:1: in <module>
    from .legateboost import LBClassifier, LBRegressor
legateboost/legateboost.py:16: in <module>
    from .metrics import BaseMetric, metrics
legateboost/metrics.py:8: in <module>
    from .special import erf, loggamma
legateboost/special.py:22: in <module>
    from .library import user_context, user_lib
E   ModuleNotFoundError: No module named 'legateboost.library'

This was happening because of the following things:

As a result, the unit tests here have actually not been using the installed version of legateboost... they've been reading the Python source files from the repo!

I think we didn't notice this before because:

129 exposed this because there, I'm building a package in one environment and then downloading and installing it into another environment... where there isn't a library.py in the source tree.

Why a legateboost.testing submodule?

Several other RAPIDS projects use this pattern.

For example, cuml:

How I tested this

Checked CI logs, confirmed that all the tests in legateboost/test were run: https://github.com/rapidsai/legate-boost/actions/runs/10398566839/job/28796079869?pr=144

On a machine with 8 V100s and CUDA 12.2, ran the following:

conda env create \
    --name legate-boost-dev-delete-me \
    -f ./conda/environments/all_cuda-122.yaml

source activate legate-boost-dev

# build non-editable
./build.sh

# remove library.py
# (if source imports were still being used, this would caused a ModuleNotFoundError)
rm ./legateboost/library.py

# run the tests
ci/run_pytests_cpu.sh

Saw that fail on main with a ModuleNotFoundError like the one reported above. On this branch, it passes.

Also tested the instructions in contributing.md.

jameslamb commented 1 month ago

Ha no problem! This was a fun one.