Skip to content

UMAP multi-gpu knn graph build docs#7019

Merged
rapids-bot[bot] merged 12 commits intorapidsai:branch-25.08from
jinsolp:umap-mg-docs
Jul 28, 2025
Merged

UMAP multi-gpu knn graph build docs#7019
rapids-bot[bot] merged 12 commits intorapidsai:branch-25.08from
jinsolp:umap-mg-docs

Conversation

@jinsolp
Copy link
Copy Markdown
Contributor

@jinsolp jinsolp commented Jul 18, 2025

Closes #6856

@jinsolp jinsolp self-assigned this Jul 18, 2025
@jinsolp jinsolp requested a review from a team as a code owner July 18, 2025 00:39
@jinsolp jinsolp requested review from teju85 and vyasr July 18, 2025 00:39
@jinsolp jinsolp added doc Documentation Cython / Python Cython or Python issue algo: umap labels Jul 18, 2025
@jinsolp jinsolp removed request for teju85 and vyasr July 18, 2025 00:39
@jinsolp jinsolp added the non-breaking Non-breaking change label Jul 18, 2025
Comment thread python/cuml/cuml/manifold/umap.pyx
Comment thread python/cuml/cuml/manifold/umap.pyx Outdated
Copy link
Copy Markdown
Contributor

@viclafargue viclafargue 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 a small edit would be necessary.

Comment thread python/cuml/cuml/manifold/umap.pyx Outdated
Comment on lines +141 to +148
if not (
found_doc.type.startswith("cuml.Handle")
and klass == cuml.manifold.umap.UMAP
):
# Ensure the docstring is identical
assert (
found_doc.type == base_item_doc.type
), "Docstring mismatch for {}".format(name)
Copy link
Copy Markdown
Contributor

@csadorf csadorf Jul 28, 2025

Choose a reason for hiding this comment

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

The need for the exceptional handling here hints at an abstraction issue. This test ensures that the base class parameters and its children have the same signature for parameters of the same name. We need the exception because this is no longer the case, the UMAP class has a different signature.

A "correct" fix could be to either actually change the signature of the Base class such that it generally accepts instances of either cuml.Handle or pylibraft.common.DeviceResourcesSNMG type or ensure that cuml.Handle is either dervied from DeviceResourcesSNMG or at least shares a common base class.

We should not make any attempt at fixing or addressing this in this PR since it was originally introduced in #6654.

I suggest that we maintain this state for now and evaluate independently whether a correction is needed.

@csadorf
Copy link
Copy Markdown
Contributor

csadorf commented Jul 28, 2025

/merge

@rapids-bot rapids-bot Bot merged commit 84494e8 into rapidsai:branch-25.08 Jul 28, 2025
128 of 132 checks passed
@jinsolp jinsolp deleted the umap-mg-docs branch August 8, 2025 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

algo: umap Cython / Python Cython or Python issue doc Documentation non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DOC] docs for multi-gpu knn build in UMAP

5 participants