Remove safe_imports.py#6588
Merged
rapids-bot[bot] merged 45 commits intorapidsai:branch-25.06from Apr 25, 2025
Merged
Conversation
jcrist
added
improvement
bdice
approved these changes
Apr 25, 2025
Contributor
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
A couple of comments. Overall it looks great. This makes the cuML import system and library design make so much more sense to me.
8 tasks
Member
Author
|
/merge |
Ofek-Haim
pushed a commit
to Ofek-Haim/cuml
that referenced
this pull request
May 13, 2025
As part of removing `cuml-cpu` support, we no longer need to guard imports for CPU or GPU builds. I originally set out to remove only the `gpu_only_import*` calls, but as I was going I found lots of crud and ended up removing _all_ the guarded imports in `safe_imports.py` (along with that entire file). This was mostly mechanical work, aided by a helper script, patience, and a background podcast. Usage of these functions fell into a few camps: - Guarding GPU imports for non-gpu builds (`gpu_only_import`/`gpu_only_import_from`). These can be fully removed - all our builds are now GPU builds. - Guarding CPU-only imports for non-cpu builds (`cpu_only_import`/`cpu_only_import_from`). Afaict these never made much sense, all cpu dependencies are also dependencies of the GPU-only builds. - Guarding some optional dependencies (`safe_import`/`safe_import_from`). Usage of this was pretty sparse, in most cases the logic was clearer when I inlined the try/except call in the calling code. - Guarding `scipy` imports as if `scipy` was an optional dependency. `scipy` is not an optional dependency for `cuml`, and hasn't been for at least a while. There's still some guarded imports from `has_scipy` checks in a few places, but I removed all those that were handled by the functions removed in this PR. Since sklearn requires scipy (and we're moving towards requiring sklearn), there's no chance we'll ever want to make it optional, and there were plenty of unguarded imports of scipy already. Might as well lean all in. Same goes for guarded `pandas` imports - we require pandas, no need to guard them. - Guarding `nvtx` imports. I've made a new `cuml.internals.nvtx` module to handle that more simply and uniformly. There are a few places where there were code changes made as well. These are mostly nits dealing with switching to using namespaced versions of the imports (`cudf.DataFrame` instead of `CudfDataFrame`), resulting in simpler and more uniform imports. If in the future we decide we want to make some dependencies optional, I'd encourage us to do so in a non-magical way. Take what was done in `cuml.internals.nvtx` as an example of a decent pattern (isolating the existence check to one location, and providing clear consistent alternatives in that same spot). Littering the code with optional imports makes it easy to miss one, and also harder to reason about. Part of rapidsai#6523. Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Bradley Dice (https://github.com/bdice) URL: rapidsai#6588
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
2 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.
As part of removing
cuml-cpusupport, we no longer need to guard imports for CPU or GPU builds. I originally set out to remove only thegpu_only_import*calls, but as I was going I found lots of crud and ended up removing all the guarded imports insafe_imports.py(along with that entire file). This was mostly mechanical work, aided by a helper script, patience, and a background podcast.Usage of these functions fell into a few camps:
gpu_only_import/gpu_only_import_from). These can be fully removed - all our builds are now GPU builds.cpu_only_import/cpu_only_import_from). Afaict these never made much sense, all cpu dependencies are also dependencies of the GPU-only builds.safe_import/safe_import_from). Usage of this was pretty sparse, in most cases the logic was clearer when I inlined the try/except call in the calling code.scipyimports as ifscipywas an optional dependency.scipyis not an optional dependency forcuml, and hasn't been for at least a while. There's still some guarded imports fromhas_scipychecks in a few places, but I removed all those that were handled by the functions removed in this PR. Since sklearn requires scipy (and we're moving towards requiring sklearn), there's no chance we'll ever want to make it optional, and there were plenty of unguarded imports of scipy already. Might as well lean all in. Same goes for guardedpandasimports - we require pandas, no need to guard them.nvtximports. I've made a newcuml.internals.nvtxmodule to handle that more simply and uniformly.There are a few places where there were code changes made as well. These are mostly nits dealing with switching to using namespaced versions of the imports (
cudf.DataFrameinstead ofCudfDataFrame), resulting in simpler and more uniform imports.If in the future we decide we want to make some dependencies optional, I'd encourage us to do so in a non-magical way. Take what was done in
cuml.internals.nvtxas an example of a decent pattern (isolating the existence check to one location, and providing clear consistent alternatives in that same spot). Littering the code with optional imports makes it easy to miss one, and also harder to reason about.Part of #6523.