Skip to content

Skip dispatching to GPU for unimplemented metrics in UMAP#6224

Merged
rapids-bot[bot] merged 9 commits intorapidsai:branch-25.02from
betatim:blacklist-unsupported-metrics
Feb 6, 2025
Merged

Skip dispatching to GPU for unimplemented metrics in UMAP#6224
rapids-bot[bot] merged 9 commits intorapidsai:branch-25.02from
betatim:blacklist-unsupported-metrics

Conversation

@betatim
Copy link
Copy Markdown
Member

@betatim betatim commented Jan 14, 2025

This lists more metrics that the CPU umap library supports but cuml doesn't yet support. By listing them as not implemented we don't dispatch to the GPU when a user selects them.

Also added them to a test to check that they do not raise an error. It would be nice to check that they ran on the CPU when the accelerator is enabled, but I couldn't find a nice way to do it :-/ Ideas welcome.

@betatim betatim requested a review from a team as a code owner January 14, 2025 16:16
@betatim betatim requested review from divyegala and teju85 January 14, 2025 16:16
@github-actions github-actions Bot added the Cython / Python Cython or Python issue label Jan 14, 2025
…ms/test_accel_umap.py

Co-authored-by: Dante Gama Dessavre <[email protected]>
@betatim betatim added the non-breaking Non-breaking change label Jan 15, 2025
@betatim
Copy link
Copy Markdown
Member Author

betatim commented Jan 15, 2025

I feel like I am missing something. The tests are still failing with FAILED estimators_hyperparams/test_accel_umap.py::test_umap_metric[russelrao] - ValueError: metric is neither callable nor a recognised string - but russelrao is how the metric is spelt??

@betatim
Copy link
Copy Markdown
Member Author

betatim commented Jan 15, 2025

The answer is that the docs call it russelrao but the code actually looks for russellrao.

@betatim betatim changed the title Skip dispatching to GPU for unimplemented metrics Skip dispatching to GPU for unimplemented metrics in UMAP Jan 15, 2025
@betatim betatim requested a review from a team as a code owner January 15, 2025 16:14
@betatim betatim requested a review from jameslamb January 15, 2025 16:14
@github-actions github-actions Bot added the ci label Jan 15, 2025
@betatim betatim force-pushed the blacklist-unsupported-metrics branch from df524b9 to 18dc640 Compare January 16, 2025 09:09
Copy link
Copy Markdown
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

I recommend reverting that rapids-mamba-retry --quiet. I've explained the tradeoffs in a comment.

Marking this "approve" so you're not blocked waiting on ci-codeowners, and so you can merge it without further review if you all want to make a different choice than I would in the face of those tradeoffs.

Comment thread ci/test_python_common.sh Outdated
@github-actions github-actions Bot removed the ci label Jan 30, 2025
@dantegd dantegd added the improvement Improvement / enhancement to an existing function label Feb 6, 2025
@dantegd
Copy link
Copy Markdown
Member

dantegd commented Feb 6, 2025

/merge

@rapids-bot rapids-bot Bot merged commit f67b426 into rapidsai:branch-25.02 Feb 6, 2025
@betatim betatim deleted the blacklist-unsupported-metrics branch February 7, 2025 08:13
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.

4 participants