[None][fix] Fix the issue of responsibility boundary between the assert and tllmException files #6723
Conversation
…rsion Signed-off-by: fanyunfan <[email protected]>
…xception files Signed-off-by: fanyunfan <[email protected]>
📝 WalkthroughWalkthroughException handling utilities and macros have been moved from Changes
Sequence Diagram(s)sequenceDiagram
participant User Code
participant tllmException.h
participant TllmException
User Code->>tllmException.h: Call TLLM_THROW(...) or throwRuntimeError(...)
tllmException.h->>TllmException: Construct TllmException with formatted message
tllmException.h-->>User Code: Exception thrown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Fan-Yunfan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp/include/tensorrt_llm/common/tllmException.h (1)
2-2: Update copyright year to include 2025.The copyright header should include the current year (2025) according to TensorRT-LLM coding guidelines.
Apply this diff to update the copyright year:
- * Copyright (c) 2022-2024, NVIDIA CORPORATION. All rights reserved. + * Copyright (c) 2022-2025, NVIDIA CORPORATION. All rights reserved.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp/include/tensorrt_llm/common/assert.h(0 hunks)cpp/include/tensorrt_llm/common/tllmException.h(2 hunks)
💤 Files with no reviewable changes (1)
- cpp/include/tensorrt_llm/common/assert.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp,cc,cxx}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
**/*.{cpp,h,hpp,cc,cxx}: Closing braces of namespaces should have a comment saying the namespace it closes (e.g., } // namespace foo).
Prefer const or constexpr variables over #defines whenever possible.
A variable that is not modified after its initialization should be declared as const.
Except 0 (used for checking signness/existence/emptiness), nullptr, true, false, all other literals should only be used for variable initialization.
Use the Allman indentation style for braces in C++ code.
Put the semicolon for an empty for or while loop in a new line.
The statement forming the body of a switch, while, do..while, or for statement shall be a compound statement (use brace-delimited statements).
If and else should always be followed by brace-delimited statements, even if empty or a single statement.
C++ filenames should use camel case with the first letter lowercase (e.g., thisIsAFilename.cpp), and all files involved in a compilation target must have case-insensitive unique filenames.
All types (including class names) should use camel case with uppercase first letter (e.g., FooBarClass).
Local variables, methods, and namespaces should use camel case with first letter lowercase (e.g., localFooBar).
Non-magic-number global variables that are non-static and not defined in anonymous namespace should use camel case prefixed by 'g' (e.g., gDontUseGlobalFoos).
Non-magic-number global variables that are static or defined in an anonymous namespace should use camel case prefixed by 's' (e.g., sMutableStaticGlobal).
Locally visible static variables should use camel case with lowercase prefix 's' as the first letter (e.g., static std::once_flag sFlag;).
Class member variables should use camel case prefixed with 'm' (e.g., mNbFooValues). Public member variables do not require the 'm' prefix but it is encouraged for clarity.
Enumerations, global constants, static constants at class-scope, and function-scope magic-number/literal constants should be uppercase snake case with prefix...
Files:
cpp/include/tensorrt_llm/common/tllmException.h
**/*.{h,hpp}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
Use a preprocessor guard in header files. The guard name must have prefix TRTLLM_ followed by the filename, all in caps, and no trailing underscore.
Files:
cpp/include/tensorrt_llm/common/tllmException.h
**/*.{cpp,h,hpp,cc,cxx,cu,py}
📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Files:
cpp/include/tensorrt_llm/common/tllmException.h
🧠 Learnings (7)
📓 Common learnings
Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : Exceptions must not be thrown across library boundaries.
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{h,hpp} : Use a preprocessor guard in header files. The guard name must have prefix TRTLLM_ followed by the filename, all in caps, and no trailing underscore.
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx,cu,py} : All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
📚 Learning: 2025-08-06T21:22:55.018Z
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : Exceptions must not be thrown across library boundaries.
Applied to files:
cpp/include/tensorrt_llm/common/tllmException.h
📚 Learning: 2025-08-06T21:22:55.018Z
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{h,hpp} : Use a preprocessor guard in header files. The guard name must have prefix TRTLLM_ followed by the filename, all in caps, and no trailing underscore.
Applied to files:
cpp/include/tensorrt_llm/common/tllmException.h
📚 Learning: 2025-08-06T21:22:55.018Z
Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-08-06T21:22:55.018Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx,cu,py} : All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.
Applied to files:
cpp/include/tensorrt_llm/common/tllmException.h
📚 Learning: 2025-08-06T13:58:07.506Z
Learnt from: galagam
PR: NVIDIA/TensorRT-LLM#6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
Applied to files:
cpp/include/tensorrt_llm/common/tllmException.h
📚 Learning: 2025-08-01T15:14:45.673Z
Learnt from: yibinl-nvidia
PR: NVIDIA/TensorRT-LLM#6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.
Applied to files:
cpp/include/tensorrt_llm/common/tllmException.h
📚 Learning: 2025-07-28T17:06:08.621Z
Learnt from: moraxu
PR: NVIDIA/TensorRT-LLM#6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.
Applied to files:
cpp/include/tensorrt_llm/common/tllmException.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (4)
cpp/include/tensorrt_llm/common/tllmException.h (4)
19-19: LGTM!The inclusion of
stringUtils.his necessary to support thefmtstrfunction used by the new exception handling utilities.
26-30: LGTM!The
TLLM_THROWmacro is well-implemented using the standard do-while(0) pattern and properly delegates to the existingNEW_TLLM_EXCEPTIONmacro.
32-33: LGTM!The
TLLM_WRAPmacro effectively combines exception type information with the original message using the existingdemangleutility.
59-67: LGTM!The
throwRuntimeErrorfunction overloads are well-implemented with proper[[noreturn]]attributes, consistent error message formatting, and appropriate parameter types for flexibility.
svc-trtllm-gh-bot
added
the
Community want to contribute
|
/bot run |
|
Dear @tongyuantongyu ，I noticed that you have made some related modifications to the assert.h file (a139eae), so I was wondering if you could take a look at this PR I submitted when you have some free time? 
By the way, I have another PR（https://github.com/NVIDIA/TensorRT-LLM/pull/6758） that needs your review. |
|
/bot run |
|
PR_Github #14731 [ run ] triggered by Bot |
|
PR_Github #14731 [ run ] completed with state |
|
/bot run |
|
PR_Github #14891 [ run ] triggered by Bot |
|
PR_Github #14891 [ run ] completed with state |
|
/bot run |
|
PR_Github #14961 [ run ] triggered by Bot |
|
PR_Github #14961 [ run ] completed with state |
|
/bot run |
|
PR_Github #15047 [ run ] triggered by Bot |
|
PR_Github #15047 [ run ] completed with state |
|
/bot run |
|
PR_Github #15210 [ run ] triggered by Bot |
|
PR_Github #15210 [ run ] completed with state |
|
/bot run |
|
PR_Github #15273 [ run ] triggered by Bot |
|
PR_Github #15273 [ run ] completed with state |
…ception files (NVIDIA#6723) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: Yuan Tong <[email protected]>
…ception files (NVIDIA#6723) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ception files (NVIDIA#6723) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ception files (NVIDIA#6723) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: Yuan Tong <[email protected]>
…ception files (NVIDIA#6723) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…ception files (NVIDIA#6723) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: Yuan Tong <[email protected]>
…ception files (NVIDIA#6723) Signed-off-by: fanyunfan <[email protected]> Co-authored-by: Yuan Tong <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
4 participants
Problem
In the cpp/include/tensorrt_llm/common directory, there is an unclear boundary of responsibilities between the assert.h file and the tllmException.h file.
From a framework design perspective, assert.h should focus on providing assertion mechanisms to verify assumptions during program runtime, while tllmException.h should specialize in exception handling. However, in the current implementation, assert.h includes exception-related macros such as TLLM_THROW, TLLM_WRAP, and related functions like throwRuntimeError.
Although this poses no syntactic issues, from a design and standardization standpoint, it creates ambiguity in the responsibilities of the files, which may confuse framework users. For example, a user might encounter the TLLM_THROW macro in their code but fail to find the corresponding exception-related header file included. Eventually, they might discover the macro in assert.h, leading to confusion: Why are exception-related macros placed in an assertion-related header file instead of the exception-related tllmException.h?
While this design consolidates commonly used assertion and exception-throwing macros in one place, reducing the number of header inclusions (from 2 to 1), it is not considered a good practice because it violates design principles and causes confusion. Moreover, by the same logic, placing LOG-related macros in assert.h could further reduce header inclusions, but this would clearly be undesirable.
Current Implementation
cpp/include/tensorrt_llm/common/assert.hSolution
Migrate the exception-related macro definitions and functions from the assert.h header file to the tllmException.h header file.
The content of the files after migration is as follows:
cpp/include/tensorrt_llm/common/assert.hcpp/include/tensorrt_llm/common/tllmException.hNote
The original assert.h header file included stringUtils.h (for fmtstr), which has now been moved to the tllmException.h header file. Additionally, since the header file included in assert.h is already present in tllmException.h, simply including tllmException.h is sufficient.
This change will not require large-scale code modifications, nor will it necessitate additional header file adjustments, because the exception-related functions and macros remain accessible in assert.h—only their source has shifted from being defined directly in the file to being imported from tllmException.h.
Summary by CodeRabbit
Bug Fixes
New Features