Use cuda-python bindings for getting device properties. #4830
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
| return f"{major}.{minor}" | ||
|
|
||
|
|
||
| def _is_ampere_or_newer(): |
There was a problem hiding this comment.
I implemented what was here before, but I would double-check this logic: are our notebooks still failing on Ampere and newer? Does this check need to be removed?
There was a problem hiding this comment.
LGTM. I'm in favor of committing what's here now then checking the notebooks that use the "# Does not run on Ampere" comment on Ampere to see if it's still needed. If not needed, we can have a followup PR to remove it and/or the comments.
There was a problem hiding this comment.
@rlratzel Sounds good. I will follow up with a PR that removes this, and our CI will cover the check. Our ARM runners use Ampere.
| @@ -19,7 +19,6 @@ | |||
| from cugraph.utilities.path_retrieval cimport get_traversed_cost as c_get_traversed_cost | |||
| from cugraph.structure.graph_primtypes cimport * | |||
| from libc.stdint cimport uintptr_t | |||
| from numba import cuda | |||
There was a problem hiding this comment.
This was an unused import.
| @@ -210,45 +207,42 @@ def get_traversed_path_list(df, id): | |||
| return answer | |||
|
|
|||
|
|
|||
| def is_cuda_version_less_than(min_version=(10, 2)): | |||
| def is_cuda_version_less_than(min_version): | |||
There was a problem hiding this comment.
Based on the function name, this should not have a default value. Its default was also outdated.
This function also appears to be unused. Do we want to keep it?
There was a problem hiding this comment.
Removing it seems like a good idea to me.
|
|
||
|
|
||
| def is_device_version_less_than(min_version=(7, 0)): | ||
| def is_device_version_less_than(min_version): |
There was a problem hiding this comment.
Based on the function name, this should not have a default value.
It appears this is only used once, to guard against use on Pascal. However, we dropped Pascal a year ago. Can we remove this guard on the test? Then, can we delete this function since it is unused?
Co-authored-by: jakirkham <[email protected]>
bdice
added
improvement
|
It seems like the same or similar version functions are in multiple places. Could we pick one and use that in the other places? |
I think some of these are needed in odd places -- things like listing compatible notebooks shouldn't require a cugraph utility function. There may be some opportunities for removal, though. See my comments above. |
|
Agreed. Maybe @dantegd can provide us some guidance 😉 |
| @@ -210,45 +207,42 @@ def get_traversed_path_list(df, id): | |||
| return answer | |||
|
|
|||
|
|
|||
| def is_cuda_version_less_than(min_version=(10, 2)): | |||
| def is_cuda_version_less_than(min_version): | |||
There was a problem hiding this comment.
Removing it seems like a good idea to me.
| return f"{major}.{minor}" | ||
|
|
||
|
|
||
| def _is_ampere_or_newer(): |
There was a problem hiding this comment.
LGTM. I'm in favor of committing what's here now then checking the notebooks that use the "# Does not run on Ampere" comment on Ampere to see if it's still needed. If not needed, we can have a followup PR to remove it and/or the comments.
|
/merge |
This PR removes some utilities that were updated in #4830 but are no longer needed. xref: rapidsai/build-planning#117 Authors: - Bradley Dice (https://github.com/bdice) Approvers: - James Lamb (https://github.com/jameslamb) - Rick Ratzel (https://github.com/rlratzel) URL: #4855
This PR uses
cuda-pythonfor getting device properties. These APIs are more stable than getting this information vianumba.cuda.Companion to #4829 (this is not dependent on that PR, though).