Remove guarded imports of scipy#6596
Merged
rapids-bot[bot] merged 2 commits intorapidsai:branch-25.06from Apr 29, 2025
Merged
Conversation
SciPy is currently a required dependency of cuml (and has been for a while). With the intent to make sklearn a required dependency (which also requires scipy), it seems unlikely we'll ever step back from this requirement. This PR removes all the remaining guarded imports of scipy, along with the `has_scipy` function. There were plenty of places where we didn't guard the import, so doing this improves consistency and deletes some unneeded branching. In the 2 places where we were using `has_scipy` to also check the version I've inlined the checks which is also more readable IMO.
jcrist
added
improvement
gforsyth
approved these changes
Apr 28, 2025
betatim
reviewed
Apr 29, 2025
Comment on lines
-38
to
+34
| is_scipy_sparse = has_scipy() and scipy.sparse.issparse(X) | ||
| return cupyx.scipy.sparse.issparse(X) or is_scipy_sparse | ||
| return scipy.sparse.issparse(X) or cupyx.scipy.sparse.issparse(X) |
Member
There was a problem hiding this comment.
Do we have to worry about the order in which the parts of the or statement are evaluated? I think Python short circuits logical statements like this, aka it stops once the value is known. I can't work out in my head if the old version relied on this behaviour to avoid errors? Maybe also far too obscure a thing to worry about?
Member
There was a problem hiding this comment.
Maybe https://github.com/rapidsai/cuml/pull/6596/files#diff-c3c4164cf77550054ea884f2df9f240020f66dcdb29e69e3eabedb308f5fd246R224 is the answer: too obscure to worry about
betatim
approved these changes
Apr 29, 2025
Member
betatim
left a comment
betatim
left a comment
There was a problem hiding this comment.
LGTM
One question for my education but I think I answered it myself from the rest of the diff.
Member
|
/merge |
rapids-bot Bot
pushed a commit
that referenced
this pull request
Apr 29, 2025
This PR started because I noticed most of the functions in `import_utils.py` were effectively dead code. It spiraled out a bit from there to remove _almost all_ of `import_utils.py` in favor of: - Using `pytest.importorskip` to handle conditional imports in tests. This is the proper `pytest` pattern to do this, and is both easier to get correct and uses fewer lines of code. - Localized import checks (see `hdbscan.pyx`). Keeping the import check local to the module is easier to read IMO, and also helps avoid dead code accumulating in a global utils file. - No import checks. Things like `cupy` are required dependencies and don't need to be gated at all. The remaining functions will be removed in other PRs: - `has_dask` will go away once `dask` is made optional (see #5934) - `has_scipy` is removed in #6596 - `has_sklearn` will go away once `sklearn` is made a required dependency Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Simon Adorf (https://github.com/csadorf) URL: #6599
Ofek-Haim
pushed a commit
to Ofek-Haim/cuml
that referenced
this pull request
May 13, 2025
SciPy is currently a required dependency of cuml (and has been for a while). With the intent to make sklearn a required dependency (which also requires scipy), it seems unlikely we'll ever step back from this requirement. This PR removes all the remaining guarded imports of scipy, along with the `has_scipy` function. There were plenty of places where we didn't guard the import, so doing this improves consistency and deletes some unneeded branching. In the 2 places where we were using `has_scipy` to also check the version I've inlined the checks which is also more readable IMO. Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Gil Forsyth (https://github.com/gforsyth) - Tim Head (https://github.com/betatim) URL: rapidsai#6596
Ofek-Haim
pushed a commit
to Ofek-Haim/cuml
that referenced
this pull request
May 13, 2025
This PR started because I noticed most of the functions in `import_utils.py` were effectively dead code. It spiraled out a bit from there to remove _almost all_ of `import_utils.py` in favor of: - Using `pytest.importorskip` to handle conditional imports in tests. This is the proper `pytest` pattern to do this, and is both easier to get correct and uses fewer lines of code. - Localized import checks (see `hdbscan.pyx`). Keeping the import check local to the module is easier to read IMO, and also helps avoid dead code accumulating in a global utils file. - No import checks. Things like `cupy` are required dependencies and don't need to be gated at all. The remaining functions will be removed in other PRs: - `has_dask` will go away once `dask` is made optional (see rapidsai#5934) - `has_scipy` is removed in rapidsai#6596 - `has_sklearn` will go away once `sklearn` is made a required dependency Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Simon Adorf (https://github.com/csadorf) URL: rapidsai#6599
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SciPy is currently a required dependency of cuml (and has been for a while). With the intent to make sklearn a required dependency (which also requires scipy), it seems unlikely we'll ever step back from this requirement.
This PR removes all the remaining guarded imports of scipy, along with the
has_scipyfunction. There were plenty of places where we didn't guard the import, so doing this improves consistency and deletes some unneeded branching. In the 2 places where we were usinghas_scipyto also check the version I've inlined the checks which is also more readable IMO.