Skip to content

Add more estimators to the compatibility test suite#7069

Merged
rapids-bot[bot] merged 6 commits intorapidsai:branch-25.10from
betatim:more-common-tests
Aug 12, 2025
Merged

Add more estimators to the compatibility test suite#7069
rapids-bot[bot] merged 6 commits intorapidsai:branch-25.10from
betatim:more-common-tests

Conversation

@betatim
Copy link
Copy Markdown
Member

@betatim betatim commented Jul 30, 2025

This adds more estimators and xfails for them to the compatibility test suite.

This is step 1 for #7061

Along the way I'm creating issues for issues that arise when running the checks that are more serious (aka we can't just mark the check as xfail). They should all refer to #7061 so you can see them there

@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Jul 30, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the Cython / Python Cython or Python issue label Jul 30, 2025
@betatim
Copy link
Copy Markdown
Member Author

betatim commented Jul 30, 2025

/ok to test

@betatim betatim added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 31, 2025
@betatim betatim marked this pull request as ready for review July 31, 2025 11:34
@betatim betatim requested a review from a team as a code owner July 31, 2025 11:34
@betatim betatim requested review from cjnolet and dantegd July 31, 2025 11:34
@betatim
Copy link
Copy Markdown
Member Author

betatim commented Jul 31, 2025

IMHO we can already merge this, even though there are more estimators that could be added. The CI gods are looking favourably on this PR right now, and the diff is already large.

Copy link
Copy Markdown
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

LGTM, just one question.

Comment thread python/cuml/cuml/tests/test_sklearn_compatibility.py Outdated
Copy link
Copy Markdown
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

@betatim
Copy link
Copy Markdown
Member Author

betatim commented Aug 4, 2025

After fixing the imports and adding more xfails and skipping more tests I am now at a point where estimators like RandomForestClassifier that were in a good state fail the most basic "is this estimator clone'able" check (which you can't xfail or skip) with a "bad alloc" failure. It seems like failures in one estimator effect other estimators. In particular some failed checks lead to an increased chance of future checks failing with MemoryErrors. Skipping those tests works to some extent, but eventually you get to the point that RandomForestClassifer is now at: even the most basic check fails, but if you run the test suite only on RandomForestClassifier everything passes or is xfailed.

Seems like a bit of a dead end to work on this without fixing what ever underlying problem is causing this, because there isn't much point is skipping all the checks.

@betatim
Copy link
Copy Markdown
Member Author

betatim commented Aug 4, 2025

The memory error that you see a lot is variations on this: MemoryError: std::bad_alloc: CUDA error (failed to allocate 1280 bytes) at: /home/thead/miniforge3/envs/cuml-20250729/include/rmm/mr/device/cuda_memory_resource.hpp

You can run the tests with pytest -sv --disable-warnings --tb=auto test_sklearn_compatibility.py which will eventually crash and produce a lot of MemoryErrors. You can also only run a subset, for example for TruncatedSVD with pytest -sv --disable-warnings --tb=auto test_sklearn_compatibility.py -k TruncatedSVD - if you run only the TruncatedSVD tests everything will either pass, be skipped or xfail. If you run all the tests new failures will appear within the TruncatedSVD tests.

@csadorf
Copy link
Copy Markdown
Contributor

csadorf commented Aug 4, 2025

I'd suggest to split off the RandomForestClassifier tests for now so that we can unblock this and then address those problems separately.

@betatim
Copy link
Copy Markdown
Member Author

betatim commented Aug 5, 2025

We can completely skip RandomForestClassifier and the estimators after them. However the problem isn't with those estimators per se. The problem is that at some point a check breaks some global state that leads to RMM being unable to allocate any amount of memory. As a result we end up marking many checks as xfail, even though they'd pass if you test only a specific estimator.

@betatim betatim force-pushed the more-common-tests branch from 364b8c6 to 040b9be Compare August 5, 2025 11:52
@betatim
Copy link
Copy Markdown
Member Author

betatim commented Aug 5, 2025

Ok, I like this solution better: we skip the naive bayes estimators as they are the ones that cause the problems. All the other estimators are tested.

Copy link
Copy Markdown
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Pre-approving since this generally LGTM, but I'd ask that we reference relevant issues in-code wherever applicable.

Comment thread python/cuml/tests/test_sklearn_compatibility.py Outdated
Comment thread python/cuml/tests/test_sklearn_compatibility.py Outdated
@betatim betatim force-pushed the more-common-tests branch from 607d2ea to 587e6d5 Compare August 11, 2025 15:41
@betatim
Copy link
Copy Markdown
Member Author

betatim commented Aug 12, 2025

/merge

@rapids-bot rapids-bot Bot merged commit e8b1405 into rapidsai:branch-25.10 Aug 12, 2025
130 of 132 checks passed
@betatim betatim deleted the more-common-tests branch August 12, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants