Skip to content

Prevent CUDA issues when running UMAP with scikit-learn compatibility test suite #7107

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: branch-25.10
Choose a base branch
from
Open

Prevent CUDA issues when running UMAP with scikit-learn compatibility test suite #7107

wants to merge 9 commits into from

Conversation

viclafargue
Copy link
Contributor

Answers #7068

@viclafargue viclafargue requested a review from a team as a code owner August 12, 2025 10:17
@viclafargue viclafargue requested review from csadorf and jcrist August 12, 2025 10:17
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Aug 12, 2025
@viclafargue viclafargue mentioned this pull request Aug 12, 2025
Open
@viclafargue viclafargue added bug Something isn't working non-breaking Non-breaking change labels Aug 12, 2025
@betatim
Copy link
Member

betatim commented Aug 12, 2025

Nice! Cool to see that this was an easy thing to fix!

Could you remove check_estimators_empty_data_messages from

cuml/python/cuml/tests/test_sklearn_compatibility.py

Line 756 in e8b1405

"check_estimators_empty_data_messages",
so that the test runs? Maybe we can even remove some of the other tests as well from that list? Right now all of them are skipped because they cause memory errors.

betatim
betatim reviewed Aug 13, 2025
View reviewed changes
python/cuml/tests/test_sklearn_compatibility.py Outdated
@@ -547,6 +547,11 @@
# "check_transformers_unfitted": "UMAP does not raise error when transform called before fit",
"check_parameters_default_constructible": "UMAP parameters are mutated on init",
"check_fit_check_is_fitted": "UMAP passes check_is_fitted before being fit",
"check_estimators_empty_data_messages": "UMAP does not handle empty data",
Copy link
Member

Choose a reason for hiding this comment

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

How much work would it be to make it so that the test passes instead of xfailing it? We already raise the right type of exception, is it "just" a question of adjusting the text to match what the check expects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed a bench of them.

This was referenced Sep 2, 2025
Merged
Closed
@csadorf csadorf linked an issue Sep 4, 2025 that may be closed by this pull request
[BUG] UMAP creates many std::bad_alloc errors when being tested in scikit-learn compatibility test suite #7068
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betatim betatim betatim left review comments

@csadorf csadorf csadorf approved these changes

@jcrist jcrist Awaiting requested review from jcrist jcrist is a code owner automatically assigned from rapidsai/cuml-python-codeowners

Assignees

@viclafargue viclafargue

Labels
bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] UMAP creates many std::bad_alloc errors when being tested in scikit-learn compatibility test suite
3 participants
@viclafargue @betatim @csadorf