[None][fix]revert kvcache transfer #6709
Conversation
📝 WalkthroughWalkthroughThe changes revise the logic in cache formatting and transfer decision functions for batch management. Specifically, they simplify the conditions for sending and receiving cache by removing dependencies on data parallel ranks and instead use fixed modulo checks against duplication or parallelism factors. Associated unit tests are updated to reflect these logic changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CacheFormatter
Caller->>CacheFormatter: needSendCache(selfTpRankInDpGroup, targetInfo)
CacheFormatter-->>Caller: Returns (selfTpRankInDpGroup % mDupHeadFactor == 0)
Caller->>CacheFormatter: pickRecvConnections(targetInfo)
CacheFormatter-->>Caller: Returns indices where (i % mPeerDupHeadFactor == 0)
sequenceDiagram
participant Caller
participant MLACacheFormatter
Caller->>MLACacheFormatter: needSendCache(selfTpRank, targetInfo)
alt Attention Data Parallelism enabled
MLACacheFormatter-->>Caller: Returns based on ratio of tensor parallel ranks per DP group
else Attention Data Parallelism disabled
MLACacheFormatter-->>Caller: Returns based on ratio of tensor parallelism
end
Caller->>MLACacheFormatter: pickRecvConnections()
MLACacheFormatter-->>Caller: Returns [0, 1, ..., mDomainPPSize-1]
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
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. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
✨ 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
|
|
/bot run --add-multi-gpu-test |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp (1)
78-78: Remove commented dead code.This commented line should be removed as per coding guidelines that state "Avoid dead code in C++."
- // int selfTpRank = selfIdx % selfConfig.getParallelConfig().mTensorParallelism;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp(3 hunks)cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp(3 hunks)cpp/tests/batch_manager/cacheTransceiverTest.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/tensorrt_llm/batch_manager/cacheFormatter.cppcpp/tests/batch_manager/cacheTransceiverTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
**/*.{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/tensorrt_llm/batch_manager/cacheFormatter.cppcpp/tests/batch_manager/cacheTransceiverTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: in cpp/tensorrt_llm/batch_manager/datatransceiverimpl.cpp, the existing `mmtxformap` mutex in datase...
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/cacheFormatter.cppcpp/tests/batch_manager/cacheTransceiverTest.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
📚 Learning: in tensorrt_llm/executor/worker.py, the lora adapter cache optimization logic that checks `is_adapte...
Learnt from: amitz-nv
PR: NVIDIA/TensorRT-LLM#5616
File: tensorrt_llm/executor/worker.py:375-384
Timestamp: 2025-07-17T09:01:27.402Z
Learning: In tensorrt_llm/executor/worker.py, the LoRA adapter cache optimization logic that checks `is_adapter_in_cpu_cache()` and conditionally passes None for weights/config has a known race condition issue that cannot be solved with simple error handling or verification checks. This is a known limitation that requires a more comprehensive solution.
Applied to files:
cpp/tensorrt_llm/batch_manager/cacheFormatter.cppcpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp
🔇 Additional comments (7)
cpp/tests/batch_manager/cacheTransceiverTest.cpp (3)
1477-1477: LGTM - Minor formatting improvement.Removing the extraneous blank line improves code consistency.
1567-1567: Test assertion correctly updated for simplified cache sending logic.The change from
falsetotruereflects the updatedMLACacheFormatter::needSendCachelogic that now uses a fixed modulo condition instead of destination data parallel rank comparisons.
1573-1573: Test assertion correctly updated for simplified cache sending logic.The change from
truetofalsereflects the updatedMLACacheFormatter::needSendCachelogic, complementing the logic change for the contextRank=0 case and ensuring test consistency with the simplified modulo-based cache transfer conditions.cpp/tensorrt_llm/batch_manager/cacheFormatter.cpp (2)
75-95: LGTM: Logic simplification removes unnecessary complexity.The function correctly simplifies the cache sending decision by removing dependency on destination data parallel rank and focusing solely on the sender's tensor parallel rank within its data parallel group modulo the duplication factor. This aligns with the PR objective of reverting kvcache transfer logic.
120-144: LGTM: Connection selection logic simplified appropriately.The function correctly simplifies the receive connection selection by removing dependency on self data parallel rank and using a straightforward modulo zero check against the peer duplication factor. This maintains the intended filtering behavior while reducing complexity.
cpp/tensorrt_llm/batch_manager/mlaCacheFormatter.cpp (2)
41-54: LGTM: Connection selection dramatically simplified.The function correctly simplifies the receive connection selection by removing complex offset calculations and returning a straightforward sequence from 0 to
mDomainPPSize-1. This aligns with the revert objective and reduces complexity while maintaining the essential domain pipeline size constraint.
56-85: LGTM: Cache sending logic appropriately simplified.The function correctly simplifies the cache sending decision by removing unused destination data parallel rank variables and focusing on tensor parallelism size comparisons with straightforward divisibility checks. The logic maintains the essential attention data parallelism handling while reducing complexity.
|
PR_Github #14467 [ run ] triggered by Bot |
|
PR_Github #14467 [ run ] completed with state |
|
@chuangz0 Just curious why this is reverted? |
d247a93 to
bcc7a76
Compare
|
/bot run --add-multi-gpu-test |
|
PR_Github #14517 [ run ] triggered by Bot |
I encountered some bug when gen DP size >context TP size . and there are no such case in CI. |
|
PR_Github #14517 [ run ] completed with state |
Signed-off-by: Chuang Zhu <111838961+chuangz0@users.noreply.github.com>
bcc7a76 to
e020f21
Compare
|
/bot run --add-multi-gpu-test |
|
PR_Github #14555 [ run ] triggered by Bot |
|
PR_Github #14555 [ run ] completed with state |
Summary by CodeRabbit
Bug Fixes
Tests
Description
REVERT PR #6657
Test Coverage
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.