[https://nvbugs/6482589][fix] Make CuError pickle-safe across process boundaries - #17289
[https://nvbugs/6482589][fix] Make CuError pickle-safe across process boundaries#17289lowsfer wants to merge 1 commit into
Conversation
WalkthroughThe change adds C++ ChangesCuError exception propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant kvCuError
participant NanobindModule
participant PythonCuError
kvCuError->>NanobindModule: throw error with errorCode
NanobindModule->>NanobindModule: convert errorCode to CUresult or integer
NanobindModule->>PythonCuError: set error_code and preserve message
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.cpp (1)
527-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBrace the
if (p)body.Use braces for this control-flow body.
Proposed fix
- if (p) + if (p) + { std::rethrow_exception(p); + }As per coding guidelines, “Use Allman brace style; always brace if/else, loop, and switch bodies.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.cpp` around lines 527 - 529, Update the if (p) statement in the exception-handling block to wrap std::rethrow_exception(p) in braces, following the project’s Allman brace style and always-braced control-flow guideline.Source: Coding guidelines
tensorrt_llm/runtime/kv_cache_manager_v2/_exceptions.py (1)
54-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a return annotation to
CuError.__reduce__.The repository requires annotations on every function. Annotate the reduction tuple type.
As per coding guidelines, “Annotate every function.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/runtime/kv_cache_manager_v2/_exceptions.py` around lines 54 - 55, Update CuError.__reduce__ to include an explicit return annotation describing the reduction tuple it returns, while preserving the existing self.__class__ and self.error_code behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.cpp`:
- Around line 527-529: Update the if (p) statement in the exception-handling
block to wrap std::rethrow_exception(p) in braces, following the project’s
Allman brace style and always-braced control-flow guideline.
In `@tensorrt_llm/runtime/kv_cache_manager_v2/_exceptions.py`:
- Around line 54-55: Update CuError.__reduce__ to include an explicit return
annotation describing the reduction tuple it returns, while preserving the
existing self.__class__ and self.error_code behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 98fd52b9-255d-4486-bb72-968f06aa938e
📒 Files selected for processing (3)
cpp/tensorrt_llm/nanobind/batch_manager/kvCacheManagerV2.cpptensorrt_llm/runtime/kv_cache_manager_v2/__init__.pytensorrt_llm/runtime/kv_cache_manager_v2/_exceptions.py
ab22595 to
8c1a327
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
CuError could not survive crossing a process boundary (e.g. disaggregated serving). The pure-Python CuError.__init__ stores a formatted message string in .args, so the default Exception pickling reconstructs it by calling the constructor with that string. The constructor then feeds the string to cuGetErrorString, which fails with "invalid literal for int() with base 10". Fixes: - Python backend: add CuError.__reduce__ so the exception is reconstructed from the numeric CUDA error code instead of the formatted message string. - C++ backend: bind CuError in nanobind (it was never bound and fell back to RuntimeError, losing type identity and error_code). Add a custom exception translator that carries the numeric error_code onto the Python instance. The bound type uses the plain Exception.__init__, so BaseException.__reduce__ round-trips both the message (.args) and error_code (__dict__) with no custom __reduce__ needed. - Dispatcher: promote CuError to a first-class C++ port (_cpp.CuError) instead of the getattr(..., RuntimeError) fallback. Signed-off-by: Yao Yao <lowsfer@users.noreply.github.com>
8c1a327 to
28f1c9b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
PR_Github #63974 [ run ] triggered by Bot. Commit: |
lowsfer
changed the title
|
PR_Github #63974 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64042 [ run ] triggered by Bot. Commit: |
|
PR_Github #64042 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64058 [ run ] triggered by Bot. Commit: |
|
PR_Github #64058 [ run ] completed with state
|
2 participants
Summary
CuErrorcrosses a process boundaryCuErrorso the Python backend retains its type anderror_codeCuErrorreconstruct from its numeric error code during picklingTesting
Dev Engineer Review
CuErrorbinding with anerror_codeattribute._cpp.CuErrordirectly from the Python backend.CuError.__reduce__for reconstruction from the numeric error code.CuErrorhandling across process boundaries.QA Engineer Review
No test changes.