Replace deprecated cuda.cudart with cuda.bindings.runtime.#8790
Open
ytl0623 wants to merge 2 commits intoProject-MONAI:devfrom
Open
Replace deprecated cuda.cudart with cuda.bindings.runtime.#8790ytl0623 wants to merge 2 commits intoProject-MONAI:devfrom
cuda.cudart with cuda.bindings.runtime.#8790ytl0623 wants to merge 2 commits intoProject-MONAI:devfrom
Conversation
Signed-off-by: ytl0623 <david89062388@gmail.com>
Contributor
📝 WalkthroughWalkthroughThe patch modifies monai/networks/trt_compiler.py to prefer importing Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
monai/networks/trt_compiler.py (1)
82-100: Docstring not updated for new behavior.The docstring says
cuda_ret: CUDA return codebut the function now accesseserr.value(an enum attribute). Consider updating to reflect thatcuda_retis a tuple containing a CUDA error enum. As per coding guidelines, docstrings should describe each variable.📝 Suggested docstring update
def cuassert(cuda_ret): """ Error reporting method for CUDA calls. + Args: - cuda_ret: CUDA return code. + cuda_ret: Tuple returned by CUDA runtime calls, where the first element + is a cudaError_t enum and subsequent elements are return values. + + Raises: + RuntimeError: If the CUDA call returned an error. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/networks/trt_compiler.py` around lines 82 - 100, Update the cuassert docstring to reflect the actual input and output: state that cuda_ret is a tuple where the first element is a CUDA error enum/object (accessed as err.value) and an optional second element is a return value; document the Args (cuda_ret: tuple of (cuda_error_enum, optional_result)) and the function's behavior/return (raises RuntimeError on non-zero err.value, returns the second tuple element if present, otherwise None). Mention the function name cuassert and the err.value access in the description so the docstring matches the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/networks/trt_compiler.py`:
- Around line 89-97: The bare "except Exception: pass" in the CUDA error path
silently hides failures when calling cudart.cudaGetErrorName and
cudart.cudaGetErrorString; update the except to capture the exception (e.g.,
except Exception as e) and record it (via logging.exception or logger.error with
the exception info) and/or append its text to err_msg before re-raising, so that
the RuntimeError raised from the block (around variables err, err_msg and calls
cudart.cudaGetErrorName / cudart.cudaGetErrorString) includes the original
lookup error details for debuggability.
- Line 42: The import removed the fallback and only attempts
optional_import("cuda.bindings.runtime"), breaking compatibility; restore the
fallback by attempting optional_import("cuda.bindings.runtime") first and if
cudart_imported is False (or cudart is None) then call
optional_import("cuda.cudart") to populate cudart and cudart_imported; update
the variables used elsewhere in this module (e.g., cudart, cudart_imported in
monai/networks/trt_compiler.py) so the rest of the code and tests that expect
the cuda.cudart fallback continue to work.
---
Nitpick comments:
In `@monai/networks/trt_compiler.py`:
- Around line 82-100: Update the cuassert docstring to reflect the actual input
and output: state that cuda_ret is a tuple where the first element is a CUDA
error enum/object (accessed as err.value) and an optional second element is a
return value; document the Args (cuda_ret: tuple of (cuda_error_enum,
optional_result)) and the function's behavior/return (raises RuntimeError on
non-zero err.value, returns the second tuple element if present, otherwise
None). Mention the function name cuassert and the err.value access in the
description so the docstring matches the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee2622a5-b0ee-452c-ac51-8f9b4ab7e6a3
📒 Files selected for processing (1)
monai/networks/trt_compiler.py
Comment on lines
+89
to
+97
| if err.value != 0: | ||
| err_msg = f"CUDA ERROR: {err.value}" | ||
| try: | ||
| _, err_name = cudart.cudaGetErrorName(err) | ||
| _, err_str = cudart.cudaGetErrorString(err) | ||
| err_msg = f"CUDA ERROR {err.value}: {err_name} — {err_str}" | ||
| except Exception: | ||
| pass | ||
| raise RuntimeError(err_msg) |
Contributor
There was a problem hiding this comment.
Bare except Exception: pass silently swallows errors.
Static analysis flags this pattern (Ruff S110, BLE001). If error name/string retrieval fails, consider logging the exception for debuggability rather than silently ignoring it.
🛠️ Proposed fix to log suppressed exceptions
try:
_, err_name = cudart.cudaGetErrorName(err)
_, err_str = cudart.cudaGetErrorString(err)
err_msg = f"CUDA ERROR {err.value}: {err_name} — {err_str}"
- except Exception:
- pass
+ except Exception as e:
+ get_logger("monai.networks.trt_compiler").debug(f"Failed to retrieve CUDA error details: {e}")
raise RuntimeError(err_msg)🧰 Tools
🪛 Ruff (0.15.6)
[error] 95-96: try-except-pass detected, consider logging the exception
(S110)
[warning] 95-95: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/networks/trt_compiler.py` around lines 89 - 97, The bare "except
Exception: pass" in the CUDA error path silently hides failures when calling
cudart.cudaGetErrorName and cudart.cudaGetErrorString; update the except to
capture the exception (e.g., except Exception as e) and record it (via
logging.exception or logger.error with the exception info) and/or append its
text to err_msg before re-raising, so that the RuntimeError raised from the
block (around variables err, err_msg and calls cudart.cudaGetErrorName /
cudart.cudaGetErrorString) includes the original lookup error details for
debuggability.
1. fix: Docstring not updated for new behavior. 2. fix: Missing fallback breaks backward compatibility. 3. fix: Bare except Exception: pass silently swallows errors. Signed-off-by: ytl0623 <david89062388@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/networks/trt_compiler.py (1)
42-44: Extend tests around the new import preference.The diff does not add targeted regression coverage for the preferred
cuda.bindings.runtimepath vs thecuda.cudartfallback. Please add a small mock-based test that forces both branches.As per coding guidelines, "Ensure new or modified definitions will be covered by existing or new unit tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monai/networks/trt_compiler.py` around lines 42 - 44, Add unit tests that mock optional_import to force both import branches in monai.networks.trt_compiler: one test should make optional_import("cuda.bindings.runtime") return (mock_module, True) so the code picks the preferred path, and assert behavior/state that relies on cudart being the returned module; a second test should make optional_import("cuda.bindings.runtime") return (None, False) and optional_import("cuda.cudart") return (mock_module, True) to exercise the fallback, asserting the same expected behavior. Use monkeypatch or unittest.mock.patch to stub optional_import, reference the function optional_import and the module monai.networks.trt_compiler (or the function that uses cudart there) to import the target under test after patching so the import branch is executed, and include assertions verifying that the chosen module was used in each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@monai/networks/trt_compiler.py`:
- Around line 85-92: The cuassert() docstring currently omits a Returns section
and the implementation only returns cuda_ret[1] or None, which contradicts the
comment that "subsequent elements are return values"; update both the behavior
and docstring for cuassert(): change the function to return all subsequent
elements from cuda_ret (return None if there are none, return the single element
if exactly one, or return a tuple of elements if multiple) and add a
Google-style "Returns:" section describing these cases, and keep the existing
"Raises:" text for RuntimeError when the first element indicates an error.
---
Nitpick comments:
In `@monai/networks/trt_compiler.py`:
- Around line 42-44: Add unit tests that mock optional_import to force both
import branches in monai.networks.trt_compiler: one test should make
optional_import("cuda.bindings.runtime") return (mock_module, True) so the code
picks the preferred path, and assert behavior/state that relies on cudart being
the returned module; a second test should make
optional_import("cuda.bindings.runtime") return (None, False) and
optional_import("cuda.cudart") return (mock_module, True) to exercise the
fallback, asserting the same expected behavior. Use monkeypatch or
unittest.mock.patch to stub optional_import, reference the function
optional_import and the module monai.networks.trt_compiler (or the function that
uses cudart there) to import the target under test after patching so the import
branch is executed, and include assertions verifying that the chosen module was
used in each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76d53fa1-999d-4a70-8d4e-3ce11b85e970
📒 Files selected for processing (1)
monai/networks/trt_compiler.py
Comment on lines
85
to
+92
| """ | ||
| Error reporting method for CUDA calls. | ||
| Args: | ||
| cuda_ret: CUDA return code. | ||
| cuda_ret: Tuple returned by CUDA runtime calls, where the first element | ||
| is a cudaError_t enum and subsequent elements are return values. | ||
|
|
||
| Raises: | ||
| RuntimeError: If the CUDA call returned an error. |
Contributor
There was a problem hiding this comment.
Document cuassert()'s return contract fully.
The updated docstring adds Raises, but it still omits Returns, and the implementation only returns cuda_ret[1] or None. That is narrower than “subsequent elements are return values.”
As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."
Also applies to: 104-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@monai/networks/trt_compiler.py` around lines 85 - 92, The cuassert()
docstring currently omits a Returns section and the implementation only returns
cuda_ret[1] or None, which contradicts the comment that "subsequent elements are
return values"; update both the behavior and docstring for cuassert(): change
the function to return all subsequent elements from cuda_ret (return None if
there are none, return the single element if exactly one, or return a tuple of
elements if multiple) and add a Google-style "Returns:" section describing these
cases, and keep the existing "Raises:" text for RuntimeError when the first
element indicates an error.
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
1 participant
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.
Fixes #8789
Description
Replaces the deprecated
cuda.cudartimport intrt_compiler.pywithcuda.bindings.runtime, which is the current API introduced incuda-bindings>= 12.6 (pulled in by PyTorch 2.10+).Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.