[None][chore] No-op changes to support context parallelism in disaggregated serving later #7063
Conversation
📝 WalkthroughWalkthroughAdds a context-parallelism (cp) dimension to CacheState and its ParallelConfig, threads cp through constructors, equality, serialization, stringification, and tests, and adds runtime guards in cache formatters to reject non-1 context-parallelism. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as CacheFormatter::inquireSupport
participant Self as selfConfig
participant Dest as destConfig
Note over Caller,Self: Validate configs
Caller->>Self: read mContextParallelism (cp_self)
Caller->>Dest: read mContextParallelism (cp_dest)
alt cp_self != 1 or cp_dest != 1
Caller->>Caller: log "context parallelism is not currently supported"
Caller-->>Caller: return false
else cp_self == 1 and cp_dest == 1
Caller->>Caller: continue other compatibility checks (TP/PP/DP)
Caller-->>Caller: return result
end
sequenceDiagram
participant Serializer as serializeCacheState
participant Stream as OutputStream
participant Deserializer as deserializeCacheState
participant Input as InputStream
Serializer->>Stream: write ParallelConfig fields (tp, pp)
Serializer->>Stream: write ParallelConfig.mContextParallelism (cp)
Serializer->>Stream: write other CacheState fields
Deserializer->>Input: read ParallelConfig fields (tp, pp)
Deserializer->>Input: read ParallelConfig.mContextParallelism (cp)
Deserializer->>Deserializer: construct CacheState(..., pipelineParallelism, contextParallelism, DataType, ...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
|
brb-nv
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 (3)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (2)
160-179: toString includes cp; add missing header for stringstream
- Including
cpin string output keeps diagnostics aligned with the new dimension — good.- Minor nit: this file uses
std::stringstreamin multipletoString()implementations but doesn’t include<sstream>. Please add it to the includes to avoid relying on transitive headers.Add near the other standard headers:
#include <sstream>Also applies to: 172-172
51-59: Header guards vs pragma onceProject guidelines require header include guards named like
TRTLLM_<FILENAME>_H. This header uses#pragma once. Consider switching to a guard to align with the codebase conventions.cpp/tests/unit_tests/executor/serializeUtilsTest.cpp (1)
1336-1353: Broaden test parametrization to exercise cp>1 configurationsMost instantiations fix cp to 1. To cover the new dimension meaningfully, consider adding cases with
contextCpand/orgenCp> 1 (e.g., {1, 2}) where feasible so rank-domain partitioning and target mapping logic are exercised under non-trivial cp.Also applies to: 1355-1358, 1361-1371, 1372-1377, 1379-1388, 1390-1411, 1413-1426
📜 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 (4)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h(5 hunks)cpp/tensorrt_llm/executor/serialization.cpp(3 hunks)cpp/tests/batch_manager/cacheTransceiverTest.cpp(21 hunks)cpp/tests/unit_tests/executor/serializeUtilsTest.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/tests/unit_tests/executor/serializeUtilsTest.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.hcpp/tensorrt_llm/executor/serialization.cppcpp/tests/batch_manager/cacheTransceiverTest.cpp
**/*.{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/tests/unit_tests/executor/serializeUtilsTest.cppcpp/tensorrt_llm/executor/serialization.cppcpp/tests/batch_manager/cacheTransceiverTest.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/tests/unit_tests/executor/serializeUtilsTest.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.hcpp/tensorrt_llm/executor/serialization.cppcpp/tests/batch_manager/cacheTransceiverTest.cpp
**/*.{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/tests/unit_tests/executor/serializeUtilsTest.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.hcpp/tensorrt_llm/executor/serialization.cppcpp/tests/batch_manager/cacheTransceiverTest.cpp
**/*.{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/executor/dataTransceiverState.h
🧠 Learnings (2)
📚 Learning: 2025-08-14T21:04:50.208Z
Learnt from: thorjohnsen
PR: NVIDIA/TensorRT-LLM#6910
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:0-0
Timestamp: 2025-08-14T21:04:50.208Z
Learning: In KV cache onboarding logic during prefill in cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp, when calculating which blocks fall within the attention window, use getTokensPerBlock() to advance token indices rather than block->getUniqueTokens().size(), because the calculation needs to consider the post-prefill state where blocks will be filled to capacity, not their current token count.
Applied to files:
cpp/tests/batch_manager/cacheTransceiverTest.cpp
📚 Learning: 2025-08-15T06:46:54.853Z
Learnt from: eopXD
PR: NVIDIA/TensorRT-LLM#6767
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:0-0
Timestamp: 2025-08-15T06:46:54.853Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp addToken function, newly allocated blocks are unshared by design. The beam search path in addToken (when sequence.getNumTokens() > windowSize) is currently broken/non-functional with SWA, so the block allocation doesn't follow a shared-then-unshared pattern.
Applied to files:
cpp/tests/batch_manager/cacheTransceiverTest.cpp
🧬 Code Graph Analysis (3)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (2)
cpp/tests/unit_tests/kernels/decodingKernelTest.cpp (1)
tensorParallelism(346-372)cpp/include/tensorrt_llm/batch_manager/cacheTransceiver.h (1)
nodiscard(67-90)
cpp/tensorrt_llm/executor/serialization.cpp (1)
cpp/tests/unit_tests/kernels/decodingKernelTest.cpp (1)
tensorParallelism(346-372)
cpp/tests/batch_manager/cacheTransceiverTest.cpp (2)
cpp/tests/unit_tests/kernels/decodingKernelTest.cpp (1)
tensorParallelism(346-372)cpp/tests/unit_tests/batch_manager/cacheTransBufferTest.cpp (4)
numLayers(32-69)numLayers(32-33)numLayers(77-88)numLayers(77-77)
⏰ 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 (19)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (6)
55-56: CP threaded into ParallelConfig via WorldConfig — verify DP rank/size semanticsGood to see
cpadded here. One sanity check:mParallelConfig{ tp, pp, cp, enableAttentionDP, DPrank=worldConfig.getTensorParallelRank(), DPsize=worldConfig.getTensorParallelism() }assumes Attention DP uses the TP domain for rank/size. If the DP domain can diverge from TP in some configurations, we should thread a dedicated DP-size getter fromWorldConfiginstead of reusing TP.
62-71: Ctor overload updated to include contextParallelism — looks correctThe parameter ordering and the initialization of
mParallelConfigwith the newcontextParallelismare consistent.
73-82: Second ctor overload with contextParallelism — looks correctThe braced initialization maintains argument order and matches the struct layout.
84-88: Equality now includes AttentionConfig — good catchIncluding
mAttentionConfiginCacheState::operator==avoids false positives when attention settings differ.
103-118: ParallelConfig extended with mContextParallelism and equality updated — OKField addition and
operator==update are consistent and non-ambiguous.
130-134: Add AttentionConfig::operator== — OKThis enables clean structural equality across cache states.
cpp/tests/unit_tests/executor/serializeUtilsTest.cpp (6)
726-733: ContextPhaseParams test updated to pass cp via CacheState — OKThe constructor call now includes the cp argument before
DataType. This aligns with the new API.
136-153: WorldConfig/CacheState equality test extended with cp — OK
- Introducing
contextParallelismand threading it toWorldConfigvalidates the 3D parallel config path.- The
state0 == state1check remains coherent with the updated equality semantics.
510-511: Param tuple and setup signature expanded for cp — double-check tuple-index mappingThe
AsymmetricTestParamnow has cp for both context and generation. ThesetUpCommunicatorsignature matches. Please verify all downstream uses ofstd::get<...>(param)reflect the new indices throughout the test body and instantiation blocks.Also applies to: 520-522
704-709: CacheState construction propagates cp in both context and generation paths — OK
mCacheStateandmContextCacheStateinclude cp in their parallel config.- The DP-size remains tied to TP-size (
DPsize = mContextTpSize), consistent with the existing DP domain assumptions.Also applies to: 867-871
1138-1156: Parameter extraction includes cp for context and generation — OKThe expanded tuple deconstruction is consistent with the updated
AsymmetricTestParam.
1444-1447: Target tests updated to include cp — OKThe helper
verifyContextand subsequent scenarios include cp in the constructedCacheState. Looks consistent with the new API and expected behavior.Also applies to: 1519-1523, 1555-1563
cpp/tensorrt_llm/executor/serialization.cpp (2)
534-535: Wire format change: inserted mContextParallelism — confirm compatibility expectations
CacheStateserialization/deserialization now writes/readsmContextParallelismbetweenmPipelineParallelismandmEnableAttentionDP. This alters the wire format.
- If you guarantee single-version comms (both ends upgraded together) and no persisted artifacts depend on the previous layout, this is fine.
- If cross-version communication or long-lived persisted buffers are possible, consider versioning the
CacheStateencoding (or guardingcpwith a feature/version tag) to keep backward compatibility. Without a version, older readers will misinterpret fields.I can help sketch a minimal version-tag pattern if needed.
Also applies to: 552-553, 569-570
541-543: Constructor call updated to pass contextParallelism — OKThe deserializer now reconstructs
CacheStatewith the new cp parameter in the correct position.cpp/tests/batch_manager/cacheTransceiverTest.cpp (5)
102-103: RequestInfoTest: CacheState now includes cp — OKThe basic roundtrip includes cp in
CacheState, matching the new ctor.
136-153: CacheConfigTest: cp included in worldConfig and equality path — OKUsing
contextParallelisminWorldConfigand verifyingCacheStateequality across constructors looks sound.
169-169: Mocks updated to construct CacheState with cp — OKThe mocked
DataTransceiverStateincludes cp inCacheStateconsistently across sender/requester tests.Also applies to: 221-222
704-709: AsymmetricalCacheTest: propagate cp in both gen and context CacheState — OK
mCacheStateandmContextCacheStateboth includemCpSize/mContextCpSize.- DP rank/size parameters remain bound to TP, consistent with the rest of the suite.
1138-1147: Param deconstruction: add cp indices (contextCp, genCp) — OKMapping of tuple indices to variables is coherent with the updated
AsymmetricTestParamdefinition.
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/mlaCacheFormatter.cpp (1)
555-560: Bug: duplicated self check likely intended to validate dest as well.Both sides of the condition reference selfConfig, so destConfig is never validated here. This can incorrectly accept unsupported configurations where dest heads-per-layer != 1.
- if ((selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1) - || (selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1)) + if ((selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1) + || (destConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1)) { - TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: only support MLA"); + TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: only support mNbKvHeadsPerLayer == 1 for MLA"); return false; }
🧹 Nitpick comments (4)
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (2)
573-577: Context-parallelism guard added — good; consider logging actual values.Guarding CP>1 here is aligned with the PR goals of a no-op API introduction. To make triage easier when this trips, include the self/dest CP values in the warning.
- if (selfConfig.getParallelConfig().mContextParallelism != 1 || destConfig.getParallelConfig().mContextParallelism != 1) + if (selfConfig.getParallelConfig().mContextParallelism != 1 || destConfig.getParallelConfig().mContextParallelism != 1) { - TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: context parallelism is not currently supported"); + TLLM_LOG_WARNING( + "MLACacheFormatter::inquireSupport: context parallelism is not currently supported (self=%d, dest=%d)", + selfConfig.getParallelConfig().mContextParallelism, + destConfig.getParallelConfig().mContextParallelism); return false; }
562-566: Redundant kvFactor check; remove duplication.mKvFactor equality is already validated earlier (Lines 522-526). Keeping both adds noise and maintenance overhead with no benefit.
- if (selfConfig.getAttentionConfig().mKvFactor != destConfig.getAttentionConfig().mKvFactor) - { - TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: only support same kv factor"); - return false; - }cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp (2)
825-829: Context-parallelism guard added — good; include CP values for better diagnostics.The no-op gating for CP>1 is correct. Emitting the actual CP values simplifies debugging and log triage.
- if (selfConfig.getParallelConfig().mContextParallelism != 1 || destConfig.getParallelConfig().mContextParallelism != 1) + if (selfConfig.getParallelConfig().mContextParallelism != 1 || destConfig.getParallelConfig().mContextParallelism != 1) { - TLLM_LOG_WARNING("CacheFormatter::inquireSupport: context parallelism is not currently supported"); + TLLM_LOG_WARNING( + "CacheFormatter::inquireSupport: context parallelism is not currently supported (self=%d, dest=%d)", + selfConfig.getParallelConfig().mContextParallelism, + destConfig.getParallelConfig().mContextParallelism); return false; }
825-829: DRY: consider extracting a shared helper for the CP gate used in both formatters.Both CacheFormatter and MLACacheFormatter now perform the same CP==1 check and warning. A small inline utility (e.g., in a common header) would reduce duplication and keep messages consistent.
📜 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/tensorrt_llm/batch_manager/cacheFormatter.cpp(1 hunks)cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/mlaCacheFormatter.cppcpp/tensorrt_llm/batch_manager/cacheFormatter.cpp
**/*.{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/mlaCacheFormatter.cppcpp/tensorrt_llm/batch_manager/cacheFormatter.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/mlaCacheFormatter.cppcpp/tensorrt_llm/batch_manager/cacheFormatter.cpp
**/*.{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/mlaCacheFormatter.cppcpp/tensorrt_llm/batch_manager/cacheFormatter.cpp
🧠 Learnings (2)
📚 Learning: 2025-08-06T08:18:28.669Z
Learnt from: zhengd-nv
PR: NVIDIA/TensorRT-LLM#6633
File: cpp/tensorrt_llm/batch_manager/dataTransceiverImpl.cpp:145-155
Timestamp: 2025-08-06T08:18:28.669Z
Learning: In cpp/tensorrt_llm/batch_manager/dataTransceiverImpl.cpp, the existing `mMtxForMap` mutex in DataSenderImpl is sufficient to synchronize measurement file operations in the `release` method, as all file operations occur within the same critical section that protects the `mRequestToSession` map access.
Applied to files:
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
📚 Learning: 2025-08-15T06:46:54.853Z
Learnt from: eopXD
PR: NVIDIA/TensorRT-LLM#6767
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:0-0
Timestamp: 2025-08-15T06:46:54.853Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp addToken function, newly allocated blocks are unshared by design. The beam search path in addToken (when sequence.getNumTokens() > windowSize) is currently broken/non-functional with SWA, so the block allocation doesn't follow a shared-then-unshared pattern.
Applied to files:
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
⏰ 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 (1)
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (1)
573-577: Summary mismatch: change is in inquireSupport, not unformat.The AI summary mentions adding the CP guard in unformat(); the actual guard here is in inquireSupport().
brb-nv
force-pushed
the
user/brb/helix-in-disagg-initial
branch
2 times, most recently
from
42694f6 to
0297181
Compare
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 (2)
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (1)
555-557: Bug: second operand checks self twice; dest never validated.The intent is to enforce single KV head for both self and dest, but the second condition repeats self instead of checking dest.
Apply this fix:
- if ((selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1) - || (selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1)) + if ((selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1) + || (destConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1)) { TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: only support MLA"); return false; }cpp/tests/batch_manager/cacheTransceiverTest.cpp (1)
659-662: Fix cacheType shadowing to enable K-only modeIn
cpp/tests/batch_manager/cacheTransceiverTest.cpp(around lines 659–662), the innerauto cacheTypeshadows the outer variable, sokSELFKONLYis never applied. Replace the inner declaration with an assignment:@@ -659,7 +659,7 @@ CacheType cacheType = CacheType::kSELF; if (kvFactor == 1) { - auto cacheType = CacheType::kSELFKONLY; + cacheType = CacheType::kSELFKONLY; }After applying this change, please re-run the tests covering
kvFactor == 1(MLA paths) to confirm that K-only behavior is now exercised.
🧹 Nitpick comments (6)
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (4)
562-566: Duplicate kvFactor check; remove redundancy.This duplicates the earlier kvFactor equality check (Lines 522–526). Keep one to avoid confusion and drift.
- if (selfConfig.getAttentionConfig().mKvFactor != destConfig.getAttentionConfig().mKvFactor) - { - TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: only support same kv factor"); - return false; - }
33-36: Missing include for unordered_set.This file uses std::unordered_set (Lines 528, 536) but does not include <unordered_set>. Relying on transitive includes is brittle.
#include <cstddef> #include <cstdint> #include <future> +#include <unordered_set>
602-611: Log prefix consistency nit: use MLACacheFormatter in warnings.These messages mention “CacheFormatter::inquireSupport” inside MLACacheFormatter. Aligning the prefix eases debugging and grep-ability.
- TLLM_LOG_WARNING("CacheFormatter::inquireSupport: layers %d must be divisible by pipeline parallelism :%d", + TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: layers %d must be divisible by pipeline parallelism :%d", selfNumLayers, selfPPSize); ... - TLLM_LOG_WARNING("CacheFormatter::inquireSupport: layers %d must be divisible by pipeline parallelism :%d ", + TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: layers %d must be divisible by pipeline parallelism :%d ", destNumLayers, destPPSize);
567-572: Optional: guard division by zero when DP is enabled.If mEnableAttentionDP is true while mDPsize is accidentally zero, modulo will UB. A pre-check makes this resilient.
- if (selfConfig.getParallelConfig().mEnableAttentionDP - && (selfConfig.getParallelConfig().mTensorParallelism % selfConfig.getParallelConfig().mDPsize != 0)) + if (selfConfig.getParallelConfig().mEnableAttentionDP) { - TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: TP size must be divisible by DP size"); - return false; + if (selfConfig.getParallelConfig().mDPsize == 0) + { + TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: DP size must be > 0 when attention DP is enabled"); + return false; + } + if (selfConfig.getParallelConfig().mTensorParallelism % selfConfig.getParallelConfig().mDPsize != 0) + { + TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: TP size must be divisible by DP size"); + return false; + } }Make the analogous change for destConfig as well.
Also applies to: 579-583
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (2)
62-71: Constructor overloads with contextParallelism are consistent.Signatures are orderly (tp, pp, cp, dtype, attentionType, kvFactor, DP...). This keeps call sites explicit and avoids ambiguity.
Consider a convenience overload that defaults contextParallelism=1 to reduce churn in simple call sites.
Also applies to: 73-82
17-17: Prefer include guards over pragma once per project guideline.Headers must use include guards named TRTLLM__H. Replace or augment #pragma once.
You can wrap the header like this (file-wide change):
#ifndef TRTLLM_DATA_TRANSCEIVER_STATE_H #define TRTLLM_DATA_TRANSCEIVER_STATE_H // existing contents, keep #pragma once if you prefer, but guards are required by guideline #pragma once // ... file content ... #endif // TRTLLM_DATA_TRANSCEIVER_STATE_H
📜 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 (6)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h(5 hunks)cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp(1 hunks)cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp(1 hunks)cpp/tensorrt_llm/executor/serialization.cpp(3 hunks)cpp/tests/batch_manager/cacheTransceiverTest.cpp(21 hunks)cpp/tests/unit_tests/executor/serializeUtilsTest.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp
- cpp/tests/unit_tests/executor/serializeUtilsTest.cpp
- cpp/tensorrt_llm/executor/serialization.cpp
🧰 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/tests/batch_manager/cacheTransceiverTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.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/tests/batch_manager/cacheTransceiverTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.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/tests/batch_manager/cacheTransceiverTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.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/tests/batch_manager/cacheTransceiverTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.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/executor/dataTransceiverState.h
🧠 Learnings (2)
📚 Learning: 2025-08-14T21:04:50.208Z
Learnt from: thorjohnsen
PR: NVIDIA/TensorRT-LLM#6910
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:0-0
Timestamp: 2025-08-14T21:04:50.208Z
Learning: In KV cache onboarding logic during prefill in cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp, when calculating which blocks fall within the attention window, use getTokensPerBlock() to advance token indices rather than block->getUniqueTokens().size(), because the calculation needs to consider the post-prefill state where blocks will be filled to capacity, not their current token count.
Applied to files:
cpp/tests/batch_manager/cacheTransceiverTest.cpp
📚 Learning: 2025-08-15T06:46:54.853Z
Learnt from: eopXD
PR: NVIDIA/TensorRT-LLM#6767
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:0-0
Timestamp: 2025-08-15T06:46:54.853Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp addToken function, newly allocated blocks are unshared by design. The beam search path in addToken (when sequence.getNumTokens() > windowSize) is currently broken/non-functional with SWA, so the block allocation doesn't follow a shared-then-unshared pattern.
Applied to files:
cpp/tests/batch_manager/cacheTransceiverTest.cpp
⏰ 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 (17)
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (1)
573-577: Guarding on context parallelism (cp) is correct for now; message is clear.Rejecting cp != 1 keeps runtime behavior unchanged while introducing the cp concept. This matches the PR’s “no-op” goal.
Do all callers route through inquireSupport() before invoking format/unformat? If not, we should add equivalent defensive guards (or an early no-op) in those paths too to prevent accidental misuse. I can help audit the call graph if needed.
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (5)
55-57: Threading cp and DP defaults via WorldConfig looks good.mContextParallelism is sourced from worldConfig; DP rank/size default to TP rank/size which matches how tests initialize DP. No behavior change until cp != 1 is used.
Confirm Serialization handles mContextParallelism consistently (write, read, and serializedSize). The PR description says it’s done; if you want, I can cross-check.
84-88: Include AttentionConfig in equality — good catch.Adding attention configuration to CacheState equality prevents subtle mismatches from going unnoticed.
103-118: ParallelConfig gains cp and reflects it in equality — aligned with the new dimension.
130-134: AttentionConfig operator== is appropriate.Explicit equality helps the higher-level CacheState::operator== stay clean.
172-172: toString includes cp — good for observability.cpp/tests/batch_manager/cacheTransceiverTest.cpp (11)
102-102: CacheState ctor updated with cp — serialization/equality Basic test remains valid.Passing cp=8 here is fine since this test only round-trips RequestInfo; it won’t hit runtime guards.
136-152: WorldConfig and CacheState equality test now include cp — good coverage.Adding contextParallelism to WorldConfig and asserting equality of two CacheStates across ctor paths ensures the new field is wired correctly.
168-172: Mock paths pass cp=1 — aligned with runtime guard.Keeps tests green while cp is a no-op.
Also applies to: 221-223
322-323: Symmetric test CacheState uses cp=1 — OK.
510-522: Param pack extended for cp; setup signature updated accordingly.The tuple order and setup parameters are consistent (contextTp, contextPp, contextCp, genTp, genPp, genCp, ...).
Ensure all INSTANTIATE_TEST_CASE_P tuples align with the new order; from the diff they do. If failures pop up, it’ll most likely be due to misordered params.
572-592: cp routed into per-instance state — good.mCpSize/mContextCpSize recorded; ranks and sizes computed unchanged otherwise.
704-709: cp propagated into CacheState for both generation and context paths.This matches how the cache formatter’s support checks will read cp=1 and proceed.
867-872: DP test helper reconstructs CacheState including cp — correct.
1103-1105: Members extended with cp sizes — consistent with setup paths.
1336-1371: Test matrices set cp=1 — compatible with current runtime restriction.All INSTANTIATE_* macros fix cp to 1 for both context and generation, which avoids tripping the new guard.
Also applies to: 1372-1427
1456-1460: Target rank computation tests incorporate cp — no-op as cp=1.These constructions keep the existing expectations intact while threading cp forward.
Also applies to: 1536-1541, 1643-1648
brb-nv
force-pushed
the
user/brb/helix-in-disagg-initial
branch
from
0297181 to
07ae648
Compare
|
/bot run |
brb-nv
changed the title
|
PR_Github #15832 [ run ] triggered by Bot |
|
PR_Github #15832 [ run ] completed with state |
…gregated serving Signed-off-by: Balaram Buddharaju <[email protected]>
brb-nv
force-pushed
the
user/brb/helix-in-disagg-initial
branch
from
07ae648 to
cc239f9
Compare
|
/bot run |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (1)
24-28: Missing include (used by toString methods)This header uses std::stringstream in multiple toString() implementations but doesn’t include . Relying on transitive includes is brittle and can cause downstream compile failures.
Apply:
#include <cstdint> #include <string> +#include <sstream> #include <variant> #include <vector>Also applies to: 160-180, 197-207, 219-223, 243-246, 324-354, 400-412
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (1)
555-574: Bug: duplicate self check; dest heads-per-layer is never validatedThis condition checks self twice and never validates dest, so it always rejects unless self has exactly one head. Likely intended to check both self and dest.
Apply:
- if ((selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1) - || (selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1)) + if (selfConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1 + || destConfig.getModelConfig().mNbKvHeadsPerLayer.at(0) != 1) { TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: only support MLA"); return false; }
♻️ Duplicate comments (1)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (1)
84-88: Equality now includes AttentionConfig — good callIncluding AttentionConfig in CacheState::operator== and adding AttentionConfig::operator== closes a real gap (past reviewers questioned omitting attention config from equality). This aligns hashing/equality with the full semantics of the cache layout.
Also applies to: 130-134
🧹 Nitpick comments (4)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (1)
62-71: Constructor API: consider a compatibility overload (cp defaults to 1)Threading cp through the ctors is fine. To ease downstream churn (and align with “no-op” runtime behavior), consider keeping a backwards-compatible overload that defaults contextParallelism=1 and forwards to the new ctor. This avoids breaking external consumers that haven’t been migrated yet.
Apply for both vector-based and scalar nbKvHeads overloads:
+ // Back-compat overload: defaults contextParallelism to 1 + CacheState(std::vector<SizeType32> nbKvHeadPerLayer, SizeType32 sizePerHead, SizeType32 tokensPerBlock, + SizeType32 tensorParallelism, SizeType32 pipelineParallelism, nvinfer1::DataType dataType, + AttentionType attentionType = AttentionType::kDEFAULT, int kvFactor = 2, + bool enableAttentionDP = false, int DPrank = 0, int DPsize = 0) + : CacheState(std::move(nbKvHeadPerLayer), sizePerHead, tokensPerBlock, tensorParallelism, pipelineParallelism, + /*contextParallelism=*/1, dataType, attentionType, kvFactor, enableAttentionDP, DPrank, DPsize) + {} + + // Back-compat overload: defaults contextParallelism to 1 + CacheState(SizeType32 nbAttentionLayers, SizeType32 nbKvHeads, SizeType32 sizePerHead, SizeType32 tokensPerBlock, + SizeType32 tensorParallelism, SizeType32 pipelineParallelism, nvinfer1::DataType dataType, + AttentionType attentionType = AttentionType::kDEFAULT, int kvFactor = 2, + bool enableAttentionDP = false, int DPrank = 0, int DPsize = 0) + : CacheState(nbAttentionLayers, nbKvHeads, sizePerHead, tokensPerBlock, tensorParallelism, pipelineParallelism, + /*contextParallelism=*/1, dataType, attentionType, kvFactor, enableAttentionDP, DPrank, DPsize) + {}Also applies to: 73-82
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (3)
599-607: Nit: inconsistent logger prefix (“CacheFormatter” vs “MLACacheFormatter”)For clarity and grep-ability, keep the same component prefix.
- TLLM_LOG_WARNING("CacheFormatter::inquireSupport: layers %d must be divisible by pipeline parallelism :%d", + TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: layers %d must be divisible by pipeline parallelism :%d", selfNumLayers, selfPPSize); ... - TLLM_LOG_WARNING("CacheFormatter::inquireSupport: layers %d must be divisible by pipeline parallelism :%d ", + TLLM_LOG_WARNING("MLACacheFormatter::inquireSupport: layers %d must be divisible by pipeline parallelism :%d ", destNumLayers, destPPSize);
536-538: Type consistency: use SizeType32 for dest set as wellsetVecDest currently uses int, while setVecSelf uses SizeType32. Align types to avoid narrowing and maintain consistency.
- std::unordered_set<int> setVecDest{ + std::unordered_set<SizeType32> setVecDest{ destConfig.getModelConfig().mNbKvHeadsPerLayer.begin(), destConfig.getModelConfig().mNbKvHeadsPerLayer.end()};
165-171: Spelling: agentConnnecion → agentConnectionMinor naming fix improves readability.
- auto* agentConnnecion = dynamic_cast<executor::kv_cache::AgentConnection const*>(connections[0]); - if (agentConnnecion != nullptr) + auto* agentConnection = dynamic_cast<executor::kv_cache::AgentConnection const*>(connections[0]); + if (agentConnection != nullptr) { TLLM_CHECK_WITH_INFO(bufferCoverTargetNum == pPDomainSize, "Agent need all buffer pre-allocated"); TLLM_CHECK(onlyUseDynamicBuffer == false); }- auto* agentConnnecion = dynamic_cast<executor::kv_cache::AgentConnection const*>(connections[0]); - if (agentConnnecion != nullptr) + auto* agentConnection = dynamic_cast<executor::kv_cache::AgentConnection const*>(connections[0]); + if (agentConnection != nullptr) { - cacheBufferId = agentConnnecion->getCacheBufferId(); + cacheBufferId = agentConnection->getCacheBufferId(); TLLM_CHECK(cacheBufferId.has_value()); }Also applies to: 349-375
📜 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 (7)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h(5 hunks)cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp(1 hunks)cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp(1 hunks)cpp/tensorrt_llm/executor/serialization.cpp(3 hunks)cpp/tests/batch_manager/cacheTransceiverTest.cpp(21 hunks)cpp/tests/unit_tests/executor/agentCommTest.cpp(1 hunks)cpp/tests/unit_tests/executor/serializeUtilsTest.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp
- cpp/tests/unit_tests/executor/serializeUtilsTest.cpp
- cpp/tensorrt_llm/executor/serialization.cpp
- cpp/tests/batch_manager/cacheTransceiverTest.cpp
🧰 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/tests/unit_tests/executor/agentCommTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.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/tests/unit_tests/executor/agentCommTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.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/tests/unit_tests/executor/agentCommTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.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/tests/unit_tests/executor/agentCommTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cppcpp/include/tensorrt_llm/executor/dataTransceiverState.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/executor/dataTransceiverState.h
🧠 Learnings (3)
📚 Learning: 2025-08-15T06:46:54.897Z
Learnt from: eopXD
PR: NVIDIA/TensorRT-LLM#6767
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:0-0
Timestamp: 2025-08-15T06:46:54.897Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp addToken function, newly allocated blocks are unshared by design. The beam search path in addToken (when sequence.getNumTokens() > windowSize) is currently broken/non-functional with SWA, so the block allocation doesn't follow a shared-then-unshared pattern.
Applied to files:
cpp/tests/unit_tests/executor/agentCommTest.cpp
📚 Learning: 2025-08-20T06:56:02.889Z
Learnt from: eopXD
PR: NVIDIA/TensorRT-LLM#6768
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:577-579
Timestamp: 2025-08-20T06:56:02.889Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp, maxSequenceLength is now enforced as a non-optional argument in the BlockManager constructor, so concerns about std::nullopt defaulting to 0 are not applicable. When windowSize > maxSequenceLength, a warning should be added instead of handling optional parameter cases.
Applied to files:
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
📚 Learning: 2025-08-14T21:04:50.248Z
Learnt from: thorjohnsen
PR: NVIDIA/TensorRT-LLM#6910
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:0-0
Timestamp: 2025-08-14T21:04:50.248Z
Learning: In KV cache onboarding logic during prefill in cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp, when calculating which blocks fall within the attention window, use getTokensPerBlock() to advance token indices rather than block->getUniqueTokens().size(), because the calculation needs to consider the post-prefill state where blocks will be filled to capacity, not their current token count.
Applied to files:
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
⏰ 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 (3)
cpp/include/tensorrt_llm/executor/dataTransceiverState.h (1)
107-117: cp threaded through ParallelConfig and toString — LGTM
- Adding mContextParallelism to ParallelConfig and its operator== is consistent.
- Including cp in toString improves diagnostics.
Also applies to: 172-173
cpp/tests/unit_tests/executor/agentCommTest.cpp (1)
93-94: Test updated to pass cp — LGTMThe CacheState construction reflects the new cp argument (set to 1). Matches the “cp is wired but must be 1” runtime guards.
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (1)
567-574: Guarding cp!=1 in inquireSupport is correct for the current scopeReturning early with a clear warning keeps runtime behavior unchanged while cp is not supported. Message is explicit and actionable.
|
PR_Github #15946 [ run ] triggered by Bot |
|
PR_Github #15946 [ run ] completed with state |
Description
We don't support context parallelism in disaggregated serving today. This MR makes initial no-op changes API introducing the concept of context parallelism in disaggregated serving. CP remains unsupported.
Test Coverage
Existing unit tests for disaggregated serving.
GitHub Bot Help
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.
Summary by CodeRabbit