[https://nvbugs/5467062][fix] pass logitsPostProcessorBatched by reference #7110
Conversation
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 193 files out of 300 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the 📝 WalkthroughWalkthroughUpdated LogitsPostProcessor::operator() to accept the optional LogitsPostProcessorBatched parameter by const reference instead of by value in both declaration and definition. No logic or control flow changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ 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/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
/bot run |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/tensorrt_llm/batch_manager/logitsPostProcessor.cpp (1)
73-81: Guard against nullopt before dereferencing logitsPostProcessorBatchedIf any request sets
mApplyLogitsPostProcessorBatchedbut the caller provides no batched functor (defaulted/nullopt), this will dereference an empty optional. Add a guard.Minimal fix:
if (!reqIdsVec.empty()) { logitsPostProcessorIsApplied = true; if (replicateLogitsPostProcessor || worldConfig.isFirstTensorParallelRank()) { if (!logitsPostProcessorBatched) { // Choose one: log-and-return, assert, or throw (consistent with project policy). TLLM_LOG_ERROR("Batched logits post processor is required but not provided."); return false; } (*logitsPostProcessorBatched)(reqIdsVec, logitsVec, beamTokensVec, stream, clientIdsVec); } }
🧹 Nitpick comments (4)
cpp/include/tensorrt_llm/batch_manager/logitsPostProcessor.h (3)
48-51: ABI heads-up: signature change alters the mangled symbolChanging from by-value to by-const-ref is an ABI-breaking change for external consumers compiled against older headers. Please confirm this is internal-only or plan a coordinated rebuild/release note.
18-18: Add include guard per project guidelinesGuidelines require TRTLLM__H include guards. Replace or complement
#pragma oncewith a guard.Example:
// At top (replace #pragma once) #ifndef TRTLLM_LOGITSPOSTPROCESSOR_H #define TRTLLM_LOGITSPOSTPROCESSOR_H // ... file contents ... #endif // TRTLLM_LOGITSPOSTPROCESSOR_H
2-2: Update copyright year to include 2025The .cpp already uses 2025. Please align the header.
- * SPDX-FileCopyrightText: Copyright (c) 2022-2024 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-FileCopyrightText: Copyright (c) 2022-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.cpp/tensorrt_llm/batch_manager/logitsPostProcessor.cpp (1)
42-47: Micro-opt: pre-reserve vectors when collecting batched argumentsTo avoid repeated reallocations when
decoderRequests.size()is known, reserve capacity before the loop.reqIdsVec.reserve(inputBuffers.decoderRequests.size()); logitsVec.reserve(inputBuffers.decoderRequests.size()); beamTokensVec.reserve(inputBuffers.decoderRequests.size()); clientIdsVec.reserve(inputBuffers.decoderRequests.size());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cpp/include/tensorrt_llm/batch_manager/logitsPostProcessor.h(1 hunks)cpp/tensorrt_llm/batch_manager/logitsPostProcessor.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh}: In C++, close namespaces with a comment naming the namespace (e.g., } // namespace foo)
Prefer const/constexpr variables over #define for constants
Declare variables const if not modified after initialization
Use Allman brace style in C++
C++ filenames use lowerCamelCase and must be case-insensitively unique within a build target
C++ type names use UpperCamelCase
Local variables, methods, and namespaces use lowerCamelCase
Global non-static variables not in anonymous namespace use gPrefix lowerCamelCase (e.g., gExample)
Static globals or globals in anonymous namespaces use sPrefix lowerCamelCase
Locally visible static variables start with 's' (e.g., static std::once_flag sFlag;)
Member variables use mPrefix lowerCamelCase; public members may omit but are encouraged to use 'm'
Constants (enums, global/static/function-scope magic numbers) use kPREFIXED_UPPER_SNAKE (e.g., kDIGIT_NUM)
If macros are unavoidable, use UPPER_SNAKE_CASE (prefer constants over #define)
Constructor parameter that conflicts with a public member name gets trailing underscore (foo_)
Literal suffixes should be uppercase (e.g., 1234L not 1234l)
C++: use spaces only; indent 4 spaces
Run clang-format (LLVM style) before submitting; wrap lines at 120 characters
If formatting must be bypassed, use // clang-format off/on around the section
Prefer smart pointers; use unique_ptr for sole ownership, shared_ptr for shared; weak_ptr only in exceptional cases
Do not use deprecated pre-C++11 smart pointers
Use C++ style comments; avoid C comments except special inline cases; prefer // single-line
Capitalize and punctuate full-sentence comments
Follow Doxygen rules: use //! for comments and //!< for members in C++
Disable code with #if/#endif and mnemonic conditions; avoid commented-out code; avoid dead code
Do not throw exceptions across library boundaries
Use least-forceful casts; avoid removing const/volatile; avoid C-style and functional casts (except constructors); p...
Files:
cpp/tensorrt_llm/batch_manager/logitsPostProcessor.cppcpp/include/tensorrt_llm/batch_manager/logitsPostProcessor.h
**/*.{cpp,cxx,cc,cu}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.{cpp,cxx,cc,cu}: Avoid literal values except for 0, nullptr, true, false; use named constexpr for other literals
Place semicolon of empty for/while loop on a new line
Always use brace-delimited bodies for switch/while/do-for/if/else
Use inline C comments in argument lists when parameter meaning is unclear (e.g., /* checkForErrors = */ false)
Do not use assignment in subexpressions (e.g., if (x = y) ... is forbidden)
Switch on enums should enumerate all values and omit default to catch new values at compile time
Structure switch statements; prohibit fallthrough except between empty cases; each case ends with break or throw; return at end of case not allowed; put break inside braces for compound case
Prefer anonymous namespaces over static for internal linkage of functions
Every defined function must be called at least once (no unused methods)
Files:
cpp/tensorrt_llm/batch_manager/logitsPostProcessor.cpp
**/*.{h,hpp,hxx,hh,cuh,cpp,cxx,cc,cu}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Parameter names must be consistent between declarations and definitions
Files:
cpp/tensorrt_llm/batch_manager/logitsPostProcessor.cppcpp/include/tensorrt_llm/batch_manager/logitsPostProcessor.h
**/*.{cpp,cxx,cc,cu,h,hpp,hxx,hh,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Prepend NVIDIA copyright header (current year) to all source files
Files:
cpp/tensorrt_llm/batch_manager/logitsPostProcessor.cppcpp/include/tensorrt_llm/batch_manager/logitsPostProcessor.h
**/*.{h,hpp,hxx,hh,cuh}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
Header files must use include guards named TRTLLM__H without underscores prefix/suffix (e.g., TRTLLM_FOO_BAR_HELLO_H)
Files:
cpp/include/tensorrt_llm/batch_manager/logitsPostProcessor.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 (2)
cpp/include/tensorrt_llm/batch_manager/logitsPostProcessor.h (1)
48-51: Good change: avoid copying std::function by passing std::optional<...> as const&This eliminates potentially expensive copies of std::function without changing semantics. Declaration matches the definition.
cpp/tensorrt_llm/batch_manager/logitsPostProcessor.cpp (1)
35-38: Definition updated to take std::optional<...> by const& — consistent with headerNo logic change; eliminates unnecessary copies of the callable.
|
PR_Github #16010 [ ] completed with state |
To avoid triggering the GIL on pass-by-value copy (?) Signed-off-by: Alexandre Milesi <[email protected]>
milesial
force-pushed
the
logits-processor-fix
branch
from
827213e to
d422b58
Compare
Actually this would already be covered by the existing C++ executor tests, since the default is not having any processor registered / null. |
brb-nv
removed request for
Shixiaowei02,
byshiue,
hlu1,
hypdeb,
laikhtewari,
liji-nv,
lucaslie,
poweiw,
ruodil,
schetlur-nv,
symphonylyh and
syuoni
|
/bot run |
|
PR_Github #16128 [ run ] triggered by Bot |
|
PR_Github #16128 [ run ] completed with state |
…rence (#7110) Signed-off-by: Alexandre Milesi <[email protected]>
…rence (NVIDIA#7110) Signed-off-by: Alexandre Milesi <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…rence (NVIDIA#7110) Signed-off-by: Alexandre Milesi <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…rence (NVIDIA#7110) Signed-off-by: Alexandre Milesi <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…rence (NVIDIA#7110) Signed-off-by: Alexandre Milesi <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
…rence (NVIDIA#7110) Signed-off-by: Alexandre Milesi <[email protected]> Signed-off-by: Wangshanshan <[email protected]>
9 participants
Avoids a GIL grab from the scheduling C++ thread when unnecessary (blocking kernel scheduling of the sampler). Up to 20% throughput increase at high concurrencies when a batched processor is registered.