Remove deprecated handle argument - #7751
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRemoved the deprecated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
python/cuml/cuml/tsa/stationarity.pyx (1)
35-54:⚠️ Potential issue | 🟡 MinorDocument
convert_dtypein the public docstring.Line 35 introduces
convert_dtype, but the Parameters section stops atpval_threshold, which makes the signature/docstring inconsistent for users. Please add a short description forconvert_dtype.✏️ Suggested docstring update
@@ pval_threshold : float The p-value threshold above which a series is considered stationary. + convert_dtype : bool, default=True + Whether to convert inputs to float32 when possible.python/cuml/cuml/metrics/trustworthiness.pyx (1)
80-84:⚠️ Potential issue | 🟡 MinorDuplicate validation check.
The same validation for
n_neighbors > X.shape[0]is performed twice with identical error messages. One of these checks should be removed.Proposed fix
if n_neighbors > X.shape[0]: raise ValueError("n_neighbors must be <= the number of rows.") - if n_neighbors > X.shape[0]: - raise ValueError("n_neighbors must be <= the number of rows.") - cdef uintptr_t d_X_ptrpython/cuml/cuml/svm/linear.pyx (2)
105-127:⚠️ Potential issue | 🟡 MinorMinor: Docstring mentions removed
handleparameter.The docstring at line 109-110 still references a
handle : pylibraft.common.Handleparameter that no longer exists in the function signature.📝 Proposed fix to remove stale docstring
"""Perform a Linear SVR or SVC fit. Parameters ---------- - handle : pylibraft.common.Handle - The handle to use. X : CumlArray, shape = (n_samples, n_features) Training vectors
85-127:⚠️ Potential issue | 🟡 MinorUpdate docstring to remove stale
handleparameter documentation.The API change is correctly implemented and all callers have been properly updated. However, the docstring still documents a
handleparameter (line 109) that no longer exists in the function signature. Thehandleis now created internally viaget_handle(n_streams=n_streams)at line 224. Remove the stale parameter documentation from the docstring:handle : pylibraft.common.Handle The handle to use.Both
LinearSVC.fit()andLinearSVR.fit()correctly call the underlyingcuml.svm.linear.fit()function without passing a handle, so no downstream changes are required.python/cuml/cuml/tsa/holtwinters.pyx (1)
87-91:⚠️ Potential issue | 🟡 MinorMinor: Stale
handlereference in docstring.The Known Differences section at lines 87-91 still mentions
handleas a parameter that differs from statsmodels:"this version can take additional parameters
eps,start_periods,ts_num, andhandle"Since
handleis no longer a parameter, this documentation should be updated.📝 Proposed fix to update stale docstring
* this version can take additional parameters `eps`, - `start_periods`, `ts_num`, and `handle` + `start_periods`, and `ts_num`python/cuml/cuml/explainer/base.pyx (1)
344-344:⚠️ Potential issue | 🟠 MajorRemove
.orderfrom the attribute access on line 344.Line 344 incorrectly accesses
self.masker.order.dtype. Line 338 shows thatself.masker.orderis a string ("C"in this case), not an array. Attempting to access.dtypeon a string will raise anAttributeErrorat runtime.The check should access
self.masker.dtypeinstead, which is the dtype of the background data array (as evidenced by line 355 where the masker is cast with.astype(cp.float32)).Fix
- if self.masker.order.dtype == cp.float32: + if self.masker.dtype == cp.float32:
🤖 Fix all issues with AI agents
In `@python/cuml/cuml/cluster/kmeans.pyx`:
- Line 658: _update both _predict_labels_inertia and transform so they use the
instance MG handle when appropriate: replace direct calls to get_handle() with
the conditional pattern used in fit — use self.handle if self._multi_gpu else
get_handle() — ensuring _predict_labels_inertia, transform, and any local
variable named handle follow this pattern; this keeps KMeansMG (which sets
_multi_gpu = True) using its multi-GPU handle rather than a new default handle.
In `@python/cuml/cuml/datasets/arima.pyx`:
- Around line 53-56: The make_arima function signature currently defaults
dtype='double' while its docstring claims the default is 'single'; update either
the function signature or the docstring so they match. Locate the make_arima
definition and either change the signature dtype default to 'single' or edit the
docstring text to state 'double' (whichever is correct for expected behavior),
and ensure any tests or callers expecting the previous default are adjusted
accordingly.
In `@python/cuml/cuml/metrics/pairwise_distances.pyx`:
- Line 329: The call to sparse_pairwise_distances is missing the metric_arg
captured by the wrapper, so when X is sparse the user-specified metric_arg is
dropped; update the return call in the pairwise_distances wrapper to forward
metric_arg (e.g., call sparse_pairwise_distances(X, Y, metric, convert_dtype,
metric_arg=metric_arg, **kwds)) so the provided metric_arg reaches
sparse_pairwise_distances and controls metrics like Minkowski.
In `@python/cuml/cuml/testing/utils.py`:
- Around line 268-270: The docstring for the parameter custom_constructors
currently shows a mismatched example key and constructor; update the example so
the dictionary key and instantiated class match (e.g., use {'LinearSVC': lambda:
cuml.LinearSVC(n_streams=2)} or {'LogisticRegression': lambda:
cuml.LogisticRegression(...)}), editing the docstring near the
custom_constructors description in utils.py to keep names consistent and clear.
🧹 Nitpick comments (2)
python/cuml/cuml/cluster/dbscan_mg.py (1)
20-22: Optional: fail fast on a missing handle.Line 20 makes
handlerequired, but aNonevalue will fail later. Consider a small guard for a clearer error.💡 Suggested guard
def __init__(self, *, handle, **kwargs): + if handle is None: + raise ValueError("DBSCANMG requires a valid handle for MG comms") self.handle = handle super().__init__(**kwargs)python/cuml/cuml/manifold/umap/umap.pyx (1)
1238-1242: LGTM!The handle acquisition via
get_handle(device_ids=self.device_ids)correctly preserves multi-device capability. Note: Line 1242 appears redundant ashandle_is already assigned on line 1239.🔧 Minor: Remove redundant handle assignment
handle = get_handle(device_ids=self.device_ids) cdef handle_t * handle_ = <handle_t*> <size_t> handle.getHandle() cdef unique_ptr[device_buffer] embeddings_buffer cdef lib.HostCOO fss_graph = lib.HostCOO() - handle_ = <handle_t*> <size_t> handle.getHandle()
This removes almost all the remaining deprecations (outside of #7751). The only one I didn't handle was deprecations in `TargetEncoder` since I couldn't figure out what the intended behavior was. Will leave that for others. Summary: - Removes deprecated `cuml.internals.memory_utils` module - Removes deprecated `TotalIters` for `cuml.svm` - Removes deprecated `y` parameter in `train_test_split` - Removes deprecated parameters to `UMAP` (fixes #7709) Authors: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) Approvers: - Simon Adorf (https://github.com/csadorf) URL: #7761
|
/merge |
I think what happened is that #7751 fixed something that used to lead to the `check_do_not_raise_errors_in_init_or_set_params` check failing. The reason we ended up seeing it in #7632 is that #7751 was merged before #7753 (and we didnt rerun the CI for that PR). Authors: - Tim Head (https://github.com/betatim) Approvers: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) URL: #7768
This removes almost all the remaining deprecations (outside of NVIDIA#7751). The only one I didn't handle was deprecations in `TargetEncoder` since I couldn't figure out what the intended behavior was. Will leave that for others. Summary: - Removes deprecated `cuml.internals.memory_utils` module - Removes deprecated `TotalIters` for `cuml.svm` - Removes deprecated `y` parameter in `train_test_split` - Removes deprecated parameters to `UMAP` (fixes NVIDIA#7709) Authors: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) Approvers: - Simon Adorf (https://github.com/csadorf) URL: NVIDIA#7761
This removes the deprecated `handle` argument/attributes on all relevant models and functions. Note that for now the (_mostly_ private) `*MG` classes retain their `handle` argument and attribute since the multi-gpu comms are currently attached to the handles. Some care has been taken to ensure the proper handle is utilized for multi-gpu APIs. All other APIs now make use of the `get_handle` function exclusively. This is a follow-up to NVIDIA#7628. Fixes NVIDIA#7722. Authors: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) Approvers: - Victor Lafargue (https://github.com/viclafargue) - Dante Gama Dessavre (https://github.com/dantegd) URL: NVIDIA#7751
I think what happened is that NVIDIA#7751 fixed something that used to lead to the `check_do_not_raise_errors_in_init_or_set_params` check failing. The reason we ended up seeing it in NVIDIA#7632 is that NVIDIA#7751 was merged before NVIDIA#7753 (and we didnt rerun the CI for that PR). Authors: - Tim Head (https://github.com/betatim) Approvers: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) URL: NVIDIA#7768
This removes the deprecated
handleargument/attributes on all relevant models and functions.Note that for now the (mostly private)
*MGclasses retain theirhandleargument and attribute since the multi-gpu comms are currently attached to the handles. Some care has been taken to ensure the proper handle is utilized for multi-gpu APIs. All other APIs now make use of theget_handlefunction exclusively.This is a follow-up to #7628. Fixes #7722.