Apply new validation to cuml.manifold - #8042
Conversation
jinsolp
added
Cython / Python
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThree manifold estimators (SpectralEmbedding, TSNE, UMAP) migrate bespoke input conversion to centralized validation ( ChangesManifold Input Validation Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/manifold/spectral_embedding.pyx`:
- Around line 423-429: The call to check_inputs(...) in fit currently ignores
its return value so subsequent code still uses the original X; update fit to
capture the validated/coerced array (e.g., X = check_inputs(self, X, ...)) and
then use that returned X for any further operations (reading X.shape and passing
into spectral_embedding(...)). Make sure the call-sites referencing X.shape and
the spectral_embedding(...) invocation use the validated X (and preserve
accept_sparse/self.affinity behavior).
In `@python/cuml/cuml/manifold/umap/umap.pyx`:
- Around line 1210-1240: check_inputs() can return y as a host (NumPy) array so
directly using y.data.ptr will fail; modify the y handling so when y is not None
you detect whether y has a .data.ptr (CuPy) and otherwise wrap it with CumlArray
to obtain a pointer. Concretely, in the block that sets y_ptr (currently using
y.data.ptr), replace that single access with logic like: if hasattr(y, "data")
and hasattr(y.data, "ptr") then set y_ptr = <uintptr_t>y.data.ptr else create
y_m = CumlArray(data=y, index=index_if_needed) and set y_ptr = y_m.ptr (and
ensure y_m is kept in scope similar to X_m). This uses the existing CumlArray
symbol and aligns with how X is handled so supervised fit works for both host
and device y.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4648bec4-721d-4ec3-a8cb-7756417c55f3
📒 Files selected for processing (3)
python/cuml/cuml/manifold/spectral_embedding.pyxpython/cuml/cuml/manifold/t_sne.pyxpython/cuml/cuml/manifold/umap/umap.pyx
ff9bad1 to
b332651
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
python/cuml/cuml/manifold/umap/umap.pyx (1)
1239-1243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
ydevice-backed before passing its pointer to native fit.Line 1242 avoids the NumPy attribute error, but it still leaves supervised
fit()passing a host pointer whenevercheck_inputs()returns host-backedy(the"host"/Nonemem-type paths above).Xis normalized throughCumlArrayfirst;yshould be treated the same way or the native call can read the wrong memory.Proposed fix
cdef uintptr_t y_ptr = 0 + y_m = None if y is not None: - y_ptr = <uintptr_t>( - y.data.ptr if isinstance(y, cp.ndarray) else y.ctypes.data - ) + y_m = CumlArray(data=y) + y_ptr = y_m.ptr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/manifold/umap/umap.pyx` around lines 1239 - 1243, The supervised fit path currently takes a host pointer for y (setting y_ptr from y.ctypes.data) which can mismatch X's device memory; ensure y is converted to a device-backed CumlArray before extracting its pointer (mirror how X is normalized). Concretely, in the fit flow around check_inputs() replace direct use of y with a device-normalized object (e.g., create a CumlArray or call the same input-normalization helper used for X), then set y_ptr from that device-backed object's data.ptr (and still handle cp.ndarray vs CumlArray appropriately) so the native fit always receives a device pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/cuml/cuml/manifold/umap/umap.pyx`:
- Around line 1239-1243: The supervised fit path currently takes a host pointer
for y (setting y_ptr from y.ctypes.data) which can mismatch X's device memory;
ensure y is converted to a device-backed CumlArray before extracting its pointer
(mirror how X is normalized). Concretely, in the fit flow around check_inputs()
replace direct use of y with a device-normalized object (e.g., create a
CumlArray or call the same input-normalization helper used for X), then set
y_ptr from that device-backed object's data.ptr (and still handle cp.ndarray vs
CumlArray appropriately) so the native fit always receives a device pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 36bec911-7a0f-4ece-b7e4-2df68d22e310
📒 Files selected for processing (2)
python/cuml/cuml/manifold/spectral_embedding.pyxpython/cuml/cuml/manifold/umap/umap.pyx
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml/manifold/spectral_embedding.pyx
| if self.hash_input: | ||
| if X_input_sparse: | ||
| X_for_hash = X.toarray().get() | ||
| else: | ||
| X_for_hash = cp.asnumpy(X) | ||
| if _joblib_hash(X_for_hash) == self._input_hash: | ||
| return self.embedding_ |
There was a problem hiding this comment.
Preserve the current input index on the hash fast path.
Line 1456 returns self.embedding_ directly, so when hash_input=True and the same values are transformed with a different dataframe index, the output keeps the training index instead of index from this call.
Proposed fix
if self.hash_input:
if X_input_sparse:
X_for_hash = X.toarray().get()
else:
X_for_hash = cp.asnumpy(X)
if _joblib_hash(X_for_hash) == self._input_hash:
- return self.embedding_
+ return CumlArray(
+ data=self.embedding_.to_output("cupy"),
+ index=index,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.hash_input: | |
| if X_input_sparse: | |
| X_for_hash = X.toarray().get() | |
| else: | |
| X_for_hash = cp.asnumpy(X) | |
| if _joblib_hash(X_for_hash) == self._input_hash: | |
| return self.embedding_ | |
| if self.hash_input: | |
| if X_input_sparse: | |
| X_for_hash = X.toarray().get() | |
| else: | |
| X_for_hash = cp.asnumpy(X) | |
| if _joblib_hash(X_for_hash) == self._input_hash: | |
| return CumlArray( | |
| data=self.embedding_.to_output("cupy"), | |
| index=index, | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuml/cuml/manifold/umap/umap.pyx`:
- Around line 1198-1209: The current branching can set mem_type="host" for dense
training data (when params.build_algo is NN_DESCENT or clustered
BRUTE_FORCE_KNN), which leaves CumlArray as NumPy-backed and causes
self._raw_data.ptr to be an invalid GPU pointer in lib.transform(...); change
the condition so mem_type="host" is only chosen for sparse inputs—i.e., require
X_is_sparse in the branch that sets mem_type="host" (update the block using
X_is_sparse, params.build_algo, lib.graph_build_algo.NN_DESCENT,
lib.graph_build_algo.BRUTE_FORCE_KNN, and params.build_params.n_clusters) so
dense data uses base_mem_type (keeps a device copy) for later transform() calls.
- Around line 1211-1243: check_inputs can leave y as integer dtype, but
lib.fit[_sparse] expects float*; before computing y_ptr in the umap.fit path
coerce y to float32 and C-contiguous: if y is a cupy ndarray use y =
cp.ascontiguousarray(y, dtype=cp.float32) else use y = np.ascontiguousarray(y,
dtype=np.float32) (or .astype(np.float32, copy=False) with order='C'), then take
the pointer as done now (y.data.ptr for cupy or y.ctypes.data for numpy); update
the code around the y_ptr assignment (referencing check_inputs, the local
variable y and the y_ptr calculation) so the buffer passed to lib.fit is always
float32 and contiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4205292e-400b-4732-bae5-9ec547e4982e
📒 Files selected for processing (1)
python/cuml/cuml/manifold/umap/umap.pyx
viclafargue
left a comment
viclafargue
left a comment
There was a problem hiding this comment.
Thanks for working on this!
Pre-approving to speed up things as most comments are minor things. However, importantly the y array should systematically be located on device memory.
| cdef bool X_is_sparse = is_sparse(X) | ||
|
|
||
| cdef lib.UMAPParams params | ||
| init_params(self, params, n_rows=n_rows, is_sparse=X_is_sparse) |
There was a problem hiding this comment.
More of a detail, but in principle we would like to leave the estimator in an clean state before input validation. Here init_params sets self._a, self._b, self._n_neighbors before input validation had the opportunity to fail with an exception.
There was a problem hiding this comment.
Should we reorder check_inputs before init_params? The issue when we do this is that we need to duplicate some code, becausemem_type for check_inputs depends on build_algo and knn_n_clusters, which are resolved inside init_params.
Do you have any other suggestions?
There was a problem hiding this comment.
It is probably not worth changing then.
|
/merge |
5 participants
Closes #7997