Skip to content

[None][fix] Let KV cache manager use single stream for cache block transfer (onboard/offload)#9560

Closed
eopXD wants to merge 3 commits intoNVIDIA:mainfrom
eopXD:single-onboard-offload-stream
Closed

[None][fix] Let KV cache manager use single stream for cache block transfer (onboard/offload)#9560
eopXD wants to merge 3 commits intoNVIDIA:mainfrom
eopXD:single-onboard-offload-stream

Conversation

@eopXD
Copy link
Collaborator

@eopXD eopXD commented Nov 30, 2025

Summary by CodeRabbit

  • Refactor

    • Streamlined KV cache transfer management by simplifying block copy operations in the transfer system.
    • Optimized CUDA stream resource allocation by reusing active streams instead of creating new allocations.
  • Tests

    • Re-enabled previously skipped accuracy tests to improve validation coverage.
    • Added new test case to the H100 test suite for extended coverage.

✏️ Tip: You can customize this high-level summary in your review settings.

Description

This MR aims to achieve functional correctness first for block copy. We leverage the single default stream for onboard/offload action.

The merge request has 3 parts of change:

  • Remove separate stream for onborad/offload. For functional correctness, lets use one for now.
  • Use current_stream as the stream for onboard/offload. Ideally we should now what are the streams that are going to be triggered ahead, or in another way, we should let onboard/offload register on default stream, and ideally future event execution that are kv cache block related should also respect events that are already registered to default stream.
  • Add test coverage to ensure the above fix is valid

Test Coverage

Test coverage are enabled that was originally unable to pass due to accuracy drop. With correct block movement, the test shall pass now.

  - accuracy/test_llm_api_pytorch.py::TestGemma3_1BInstruct::test_auto_dtype_vswa_reuse_low_memory_available_partial_reuse
  - accuracy/test_llm_api_pytorch.py::TestGemma3_1BInstruct::test_auto_dtype_vswa_reuse_disable_overlap_scheduler

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • Any new dependencies have been scanned for license and vulnerabilities

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • Please check this after reviewing the above items as appropriate for this PR.

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.

Details

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 the stage-list parameter 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.md
and the scripts/test_to_stage_mapping.py helper.

kill

kill

Kill all running builds associated with pull request.

skip

skip --comment COMMENT

Skip 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-pipeline

Reuse 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.

@eopXD eopXD requested a review from a team as a code owner November 30, 2025 12:35
@eopXD eopXD changed the title Single onboard offload stream [None][fix] Let KV cache manager use single stream for block transfer (onboard/offload) Nov 30, 2025
@eopXD eopXD changed the title [None][fix] Let KV cache manager use single stream for block transfer (onboard/offload) [None][fix] Let KV cache manager use single stream for cache block transfer (onboard/offload) Nov 30, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 30, 2025

📝 Walkthrough

Walkthrough

These changes simplify KV cache transfer management by removing explicit onboard and offload buffer managers and their associated pending offload tracking. The implementation is refactored to use full-block copies through a single buffer manager, reuse existing CUDA streams instead of allocating new ones, and eliminate complex synchronization logic.

Changes

Cohort / File(s) Summary
KV Cache Transfer Manager Refactoring
cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h, cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
Removed public data members mOnboardManager, mOffloadManager, and mPendingOffloads map from header. Constructor no longer initializes CUDA stream members. Simplified DRAM copy logic from partial copies to straightforward full-block copies via buffer manager. Removed pending offload event tracking and stream synchronization, with syncTransfers() now empty.
CUDA Stream Management
tensorrt_llm/_torch/pyexecutor/resource_manager.py
Changed stream initialization from allocating a new CUDA stream with torch.cuda.Stream() to reusing the current active stream via torch.cuda.current_stream() in two locations.
Test Enablement and Configuration
tests/integration/defs/accuracy/test_llm_api_pytorch.py, tests/integration/test_lists/test-db/l0_h100.yml
Removed two pytest.mark.skip decorators from TestGemma3_1BInstruct test methods to enable test execution. Added corresponding test entry to H100 test configuration list.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • kvCacheTransferManager.cpp: Requires careful review of the synchronization logic removal and implications of replacing partial copies with full-block copies. Verify that stream handling and memory transfer correctness are preserved.
  • Stream reuse pattern: Confirm that reusing the current CUDA stream in resource_manager.py is safe for all caller contexts and doesn't introduce synchronization issues.
  • Test changes: Verify that the unskipped tests pass and the new test configuration entry is correctly formatted.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: moving KV cache manager to use a single stream for cache block transfers (onboard/offload), which aligns with the code changes and PR objectives.
Description check ✅ Passed PR description covers the three main changes (remove separate stream, use current_stream, add test coverage) and lists specific test cases, but lacks a clear problem statement and rationale for why the original approach caused issues.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp (1)

98-100: Consider removing unused numTokensToCopy parameter.

After the simplification to full-block copies, numTokensToCopy is no longer used in any code path (DRAM, POSIX, or GDS). Consider removing this parameter from copyBlock, onboard, and offload function signatures in a follow-up cleanup.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0398875 and 40453d9.

📒 Files selected for processing (5)
  • cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h (0 hunks)
  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp (2 hunks)
  • tensorrt_llm/_torch/pyexecutor/resource_manager.py (1 hunks)
  • tests/integration/defs/accuracy/test_llm_api_pytorch.py (0 hunks)
  • tests/integration/test_lists/test-db/l0_h100.yml (1 hunks)
💤 Files with no reviewable changes (2)
  • tests/integration/defs/accuracy/test_llm_api_pytorch.py
  • cpp/include/tensorrt_llm/batch_manager/kvCacheTransferManager.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,cu}

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.{cpp,h,cu}: Closing braces of namespaces should have a comment saying the namespace it closes (e.g., } // namespace foo)
Prefer const or constexpr variables over #define whenever possible, as the latter are not visible to the compiler
A variable that is not modified after its initialization should be declared as const
Except 0 (only used in comparison for checking signness/existence/emptiness) and nullptr, true, false, all other literals should only be used for variable initialization and should be replaced with named constants
Use Allman indentation style for braces in C++
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 first letter lowercase (e.g., thisIsASubDir and thisIsAFilename.cpp)
All filenames involved in compilation of 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 a lower case 'g' (e.g., gDontUseGlobalFoos)
Non-magic-number global variables that are static or defined in an anonymous namespace should use camel case prefixed by a lower case 's' (e.g., sMutableStaticGlobal)
Locally visible static variables should use camel case with lowercase prefix 's' as the first letter of the name (e.g., static std::once_flag sFlag;)
Public, private and protected class member variables should use camel case prefixed with 'm' (e.g., mNbFooValues), though the 'm' pre...

Files:

  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
**/*.{cpp,h,cu,py}

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

All TensorRT-LLM Open Source Software code files should contain an NVIDIA copyright header that includes the current year at the top

Files:

  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
  • tensorrt_llm/_torch/pyexecutor/resource_manager.py
**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.py: The code developed for TensorRT-LLM should conform to Python 3.8+
Indent Python code with 4 spaces; do not use tabs
Always maintain the namespace when importing in Python, even if only one class or function from a module is used (e.g., use from package.subpackage import foo and then foo.SomeClass() instead of from package.subpackage.foo import SomeClass)
Python filenames should use snake_case (e.g., some_file.py)
Python class names should use PascalCase (e.g., class SomeClass)
Python function and method names should use snake_case (e.g., def my_awesome_function():)
Python local variable names should use snake_case, with prefix k for variable names that start with a number (e.g., k_99th_percentile = ...)
Python global variables should use upper snake_case with prefix G (e.g., G_MY_GLOBAL = ...)
Python constants should use upper snake_case (e.g., MY_CONSTANT = ...)
Avoid shadowing variables declared in an outer scope in Python
Initialize all externally visible members of a Python class in the constructor
For Python interfaces that may be used outside a file, prefer docstrings over comments
Python comments should be reserved for code within a function, or interfaces that are local to a file
Use Google style docstrings for Python classes and functions, which can be parsed by Sphinx
Python attributes and variables can be documented inline with type and description (e.g., self.x = 5 followed by """<type>: Description of 'x'""" )
Avoid using reflection in Python when functionality can be easily achieved without reflection
When using try-except blocks in Python, limit the except clause to the smallest set of specific errors possible instead of catching all exceptions
When using try-except blocks in Python to handle multiple possible variable types (duck-typing), keep the body of the try as small as possible and use the else block to implement the logic

Files:

  • tensorrt_llm/_torch/pyexecutor/resource_manager.py
🧠 Learnings (16)
📓 Common learnings
Learnt from: eopXD
Repo: NVIDIA/TensorRT-LLM PR: 6768
File: cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h:0-0
Timestamp: 2025-08-20T06:48:45.368Z
Learning: There is a planned refactoring to move cache block bookkeeping utilities from BlockManager/WindowBlockManager into the GenerationRequest class itself to improve code organization and make responsibilities clearer.
Learnt from: eopXD
Repo: NVIDIA/TensorRT-LLM PR: 6768
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:2010-2045
Timestamp: 2025-08-21T09:41:49.347Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp, updateSequenceCacheBlockOffsets is specifically for updating bookkeeping when blocks are added during the context phase, not for refreshing offsets after detach operations. During detach operations, GenerationRequest::removeFrontBlock handles the necessary cache block bookkeeping internally.
Learnt from: thorjohnsen
Repo: NVIDIA/TensorRT-LLM PR: 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.
Learnt from: eopXD
Repo: NVIDIA/TensorRT-LLM PR: 6768
File: cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h:0-0
Timestamp: 2025-08-20T06:48:45.368Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp, updateSequenceCacheBlockOffsets is only called when adding a sequence, not during detach operations. During detach, the cache block bookkeeping is handled by GenerationRequest::removeFrontBlock.
📚 Learning: 2025-09-09T09:40:45.658Z
Learnt from: fredricz-20070104
Repo: NVIDIA/TensorRT-LLM PR: 7645
File: tests/integration/test_lists/qa/llm_function_core.txt:648-648
Timestamp: 2025-09-09T09:40:45.658Z
Learning: In TensorRT-LLM test lists, it's common and intentional for the same test to appear in multiple test list files when they serve different purposes (e.g., llm_function_core.txt for comprehensive core functionality testing and llm_function_core_sanity.txt for quick sanity checks). This duplication allows tests to be run in different testing contexts.

Applied to files:

  • tests/integration/test_lists/test-db/l0_h100.yml
📚 Learning: 2025-09-17T02:48:52.732Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 7781
File: tests/integration/test_lists/waives.txt:313-313
Timestamp: 2025-09-17T02:48:52.732Z
Learning: In TensorRT-LLM, `tests/integration/test_lists/waives.txt` is specifically for waiving/skipping tests, while other test list files like those in `test-db/` and `qa/` directories are for different test execution contexts (pre-merge, post-merge, QA tests). The same test appearing in both waives.txt and execution list files is intentional - the test is part of test suites but will be skipped due to the waiver.

Applied to files:

  • tests/integration/test_lists/test-db/l0_h100.yml
📚 Learning: 2025-07-28T17:06:08.621Z
Learnt from: moraxu
Repo: NVIDIA/TensorRT-LLM PR: 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:

  • tests/integration/test_lists/test-db/l0_h100.yml
📚 Learning: 2025-08-26T09:49:04.956Z
Learnt from: pengbowang-nv
Repo: NVIDIA/TensorRT-LLM PR: 7192
File: tests/integration/test_lists/test-db/l0_dgx_b200.yml:56-72
Timestamp: 2025-08-26T09:49:04.956Z
Learning: In TensorRT-LLM test configuration files, the test scheduling system handles wildcard matching with special rules that prevent duplicate test execution even when the same tests appear in multiple yaml files with overlapping GPU wildcards (e.g., "*b200*" and "*gb200*").

Applied to files:

  • tests/integration/test_lists/test-db/l0_h100.yml
📚 Learning: 2025-08-13T11:07:11.772Z
Learnt from: Funatiq
Repo: NVIDIA/TensorRT-LLM PR: 6754
File: tests/integration/test_lists/test-db/l0_a30.yml:41-47
Timestamp: 2025-08-13T11:07:11.772Z
Learning: In TensorRT-LLM test configuration files like tests/integration/test_lists/test-db/l0_a30.yml, TIMEOUT values are specified in minutes, not seconds.

Applied to files:

  • tests/integration/test_lists/test-db/l0_h100.yml
📚 Learning: 2025-08-29T14:07:45.863Z
Learnt from: EmmaQiaoCh
Repo: NVIDIA/TensorRT-LLM PR: 7370
File: tests/unittest/trt/model_api/test_model_quantization.py:24-27
Timestamp: 2025-08-29T14:07:45.863Z
Learning: In TensorRT-LLM's CI infrastructure, pytest skip markers (pytest.mark.skip) are properly honored even when test files have __main__ blocks that call test functions directly. The testing system correctly skips tests without requiring modifications to the __main__ block execution pattern.

Applied to files:

  • tests/integration/test_lists/test-db/l0_h100.yml
📚 Learning: 2025-08-21T09:41:49.347Z
Learnt from: eopXD
Repo: NVIDIA/TensorRT-LLM PR: 6768
File: cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp:2010-2045
Timestamp: 2025-08-21T09:41:49.347Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp, updateSequenceCacheBlockOffsets is specifically for updating bookkeeping when blocks are added during the context phase, not for refreshing offsets after detach operations. During detach operations, GenerationRequest::removeFrontBlock handles the necessary cache block bookkeeping internally.

Applied to files:

  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
📚 Learning: 2025-08-20T06:48:45.368Z
Learnt from: eopXD
Repo: NVIDIA/TensorRT-LLM PR: 6768
File: cpp/include/tensorrt_llm/batch_manager/kvCacheManager.h:0-0
Timestamp: 2025-08-20T06:48:45.368Z
Learning: In cpp/tensorrt_llm/batch_manager/kvCacheManager.cpp, updateSequenceCacheBlockOffsets is only called when adding a sequence, not during detach operations. During detach, the cache block bookkeeping is handled by GenerationRequest::removeFrontBlock.

Applied to files:

  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
📚 Learning: 2025-08-15T06:46:54.897Z
Learnt from: eopXD
Repo: NVIDIA/TensorRT-LLM PR: 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/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
📚 Learning: 2025-08-14T21:04:50.248Z
Learnt from: thorjohnsen
Repo: NVIDIA/TensorRT-LLM PR: 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/kvCacheTransferManager.cpp
📚 Learning: 2025-09-23T15:01:00.070Z
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/kernels/nccl_device/config.cu:15-17
Timestamp: 2025-09-23T15:01:00.070Z
Learning: In TensorRT-LLM NCCL device kernels, the <sstream> header is not needed as an explicit include in config.cu because it's provided transitively through other headers. Local compilation testing confirms this works without the explicit include.

Applied to files:

  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
📚 Learning: 2025-08-06T08:18:28.669Z
Learnt from: zhengd-nv
Repo: NVIDIA/TensorRT-LLM PR: 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/kvCacheTransferManager.cpp
📚 Learning: 2025-08-20T06:56:02.889Z
Learnt from: eopXD
Repo: NVIDIA/TensorRT-LLM PR: 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/kvCacheTransferManager.cpp
📚 Learning: 2025-09-23T14:58:05.372Z
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/kernels/nccl_device/config.cu:42-49
Timestamp: 2025-09-23T14:58:05.372Z
Learning: In TensorRT-LLM NCCL device kernels (cpp/tensorrt_llm/kernels/nccl_device/), the token partitioning intentionally uses ceil-like distribution (same token_per_rank for all ranks) to ensure all ranks launch the same number of blocks. This is required for optimal NCCL device API barrier performance, even though it may launch extra blocks for non-existent tokens on later ranks. Runtime bounds checking in the kernel (blockID validation) handles the overshoot cases.

Applied to files:

  • cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp
📚 Learning: 2025-08-17T15:07:01.420Z
Learnt from: amitz-nv
Repo: NVIDIA/TensorRT-LLM PR: 6968
File: cpp/tensorrt_llm/thop/loraOp.cpp:133-141
Timestamp: 2025-08-17T15:07:01.420Z
Learning: In TensorRT-LLM's LoRA implementation, the LoraImpl::run() method handles setStream() internally in _runGemm() (line 51 in lora.cpp), along with setWorkspace(). The stream parameter flows from loraOp.cpp through LoraImpl::run() to _runGemm() where setStream() is called appropriately. Adding setStream() in loraOp.cpp would be redundant and goes against the intended architectural design.

Applied to files:

  • tensorrt_llm/_torch/pyexecutor/resource_manager.py
⏰ 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)
tests/integration/test_lists/test-db/l0_h100.yml (1)

49-49: LGTM!

The test addition aligns with the PR objective to re-enable accuracy tests that previously failed. The placement is consistent with the existing VSWA test grouping, and the single onboard/offload stream changes should ensure this test passes.

tensorrt_llm/_torch/pyexecutor/resource_manager.py (1)

354-356: Simplification to use current CUDA stream aligns with PR objectives.

The change from creating a new dedicated stream to reusing torch.cuda.current_stream() supports the PR's goal of achieving functional correctness via a single default stream. Since the comment notes this stream is currently unused, the change has no immediate behavioral impact but correctly sets up for future host copy features.

cpp/tensorrt_llm/batch_manager/kvCacheTransferManager.cpp (2)

109-116: Simplified full-block copy for DRAM path looks correct.

The simplification to always copy the entire block via mBufferManager.copy() aligns with the PR's objective of using a single stream for correctness. The buffer manager handles the copy operation on its associated stream, ensuring proper ordering.


192-192: Empty syncTransfers() is correct for single-stream architecture.

With the shift to using a single default CUDA stream for all onboard/offload operations, explicit synchronization between streams is no longer needed. GPU operations on the same stream are implicitly ordered, making this empty implementation correct.

@eopXD
Copy link
Collaborator Author

eopXD commented Nov 30, 2025

/bot run --disable-fail-fast

@tensorrt-cicd
Copy link
Collaborator

PR_Github #26307 [ run ] triggered by Bot. Commit: 40453d9

@tensorrt-cicd
Copy link
Collaborator

PR_Github #26307 [ run ] completed with state DISABLED
L0 testing is limited to prioritized users. User eopXD is not in the prioritized list. L0 testing cannot be triggered.

@eopXD
Copy link
Collaborator Author

eopXD commented Nov 30, 2025

/bot run --disable-fail-fast

@tensorrt-cicd
Copy link
Collaborator

PR_Github #26310 [ run ] triggered by Bot. Commit: 40453d9

@tensorrt-cicd
Copy link
Collaborator

PR_Github #26310 [ run ] completed with state SUCCESS. Commit: 40453d9
/LLM/main/L0_MergeRequest_PR pipeline #19976 completed with status: 'FAILURE'

@nvpohanh
Copy link
Collaborator

nvpohanh commented Dec 1, 2025

Remove separate stream for onborad/offload. For functional correctness, lets use one for now.

Functionally, this PR looks good to me. But if we use the same stream for onboard/offload, that means the onboard/offload can no longer overlap. Are we worried about the performance drop caused by this? Or is this just a stopgap fix for functionality correctness and we will enable overlap some time in the future? @eopXD

@eopXD
Copy link
Collaborator Author

eopXD commented Dec 2, 2025

I don't think we are too worried about the performance drop on this. The secondary blocks were broken back then and we rarely see complaints, we don't have much performance user on onboard/offload for now. So I think its a good direction to take the first step by making it right first.

@thorjohnsen
Copy link
Collaborator

thorjohnsen commented Dec 2, 2025

I agree that we should pass the stream that is actually running the kernels to KV cache manager instead of an unused stream, that was always the intended use of the stream argument. However, I don't see why removing onboard and offload streams is necessary, since those two streams already wait for the buffer manager stream. Are there tests that pass with just one stream but fails otherwise? If we pass the right stream to KV cache manager (presumably torch.cuda.current_stream()), but otherwise leave everything as-is, do those tests pass?

# Note that this stream is unused for now. Will be used for copying to host
# when that feature is enabled.
self._stream = torch.cuda.Stream()
self._stream = torch.cuda.current_stream()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find. Passing an unused stream to KV cache manager will prevent any synchronization between kernels and block copies. Are you sure torch.cuda.current_stream() is the right stream? Presumably this method is called once during setup, torch.cuda.current_stream() is probably the default stream at that point. Is that what the kernels are run on, and how sure are we that this won't change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the sound solution is to be aware of all streams afterward, or the compiler itself has a main stream where all stream respects. Let me investigate more on this.

@eopXD eopXD force-pushed the single-onboard-offload-stream branch from 40453d9 to e7ede33 Compare December 3, 2025 14:09
eopXD added 3 commits December 3, 2025 06:11
Signed-off-by: eopXD <yuehtingc@nvidia.com>
…pend on

Signed-off-by: eopXD <yuehtingc@nvidia.com>
Signed-off-by: eopXD <yuehtingc@nvidia.com>
@eopXD eopXD force-pushed the single-onboard-offload-stream branch from e7ede33 to 9538d2f Compare December 3, 2025 14:12
@eopXD
Copy link
Collaborator Author

eopXD commented Dec 3, 2025

@thorjohnsen

If we pass the right stream to KV cache manager (presumably torch.cuda.current_stream()), but otherwise leave everything as-is, do those tests pass?

Passing torch.cuda.current_stream() and giving multiple runs to the test case accuracy/test_llm_api_pytorch.py::TestGemma3_1BInstruct::test_auto_dtype_vswa_reuse_low_memory_available_partial_reuse on H100 PCIE, I see both fails and passes.

@eopXD
Copy link
Collaborator Author

eopXD commented Dec 21, 2025

Dropping the MR, stream sychronization is indeed a problem. We have @SimengLiu-nv to work on that in #10060. Will create another MR that fixes accuracy drop issue.

@eopXD eopXD closed this Dec 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants