[#11529][perf] AD host time attention MD optimization for large context - #11624
Conversation
Signed-off-by: Eran Geva <egeva@nvl72064-T17.cm.cluster>
Signed-off-by: Eran Geva <egeva@oci-hsg-cs-001-login-01.cm.cluster>
📝 WalkthroughWalkthroughThe PR refactors the attention interface module to use PyTorch tensors internally instead of Python lists. It changes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tensorrt_llm/_torch/auto_deploy/custom_ops/attention_interface.py (3)
44-65: Consider documenting the bfloat16 limitation.The
_TORCH_TO_NUMPY_DTYPEmapping excludestorch.bfloat16since numpy doesn't support it natively. The fallback at line 65 handles this correctly, but a brief comment would clarify this intentional omission for future maintainers.📝 Suggested documentation
# Torch dtype → numpy dtype for fast list-to-tensor conversion. # numpy's list→array conversion is ~2-3x faster than torch.tensor(list) for large lists. +# Note: bfloat16 is intentionally excluded (numpy lacks native support); falls back to torch.tensor(). _TORCH_TO_NUMPY_DTYPE: Dict[torch.dtype, np.dtype] = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/_torch/auto_deploy/custom_ops/attention_interface.py` around lines 44 - 65, Add a short comment above the _TORCH_TO_NUMPY_DTYPE mapping (or near the _list_to_tensor function) noting that torch.bfloat16 is intentionally omitted because NumPy lacks native bfloat16 support, and that the fallback path in _list_to_tensor (torch.tensor(...)) will handle bfloat16 tensors; reference the _TORCH_TO_NUMPY_DTYPE constant and the _list_to_tensor function so maintainers know why bfloat16 isn't mapped to a NumPy dtype.
606-619: Properties trigger repeated tensor-to-list conversions.The properties
seq_len,input_pos,cache_loc, andpages_per_seqnow call.tolist()on each access. Innest_sequences, these are accessed multiple times (e.g.,self.input_posat lines 990 and 1044,self.seq_lenat lines 990 and 1044). Each access triggers a new conversion.For large batches, consider caching the list locally within
nest_sequencesto avoid repeated conversions.Based on learnings: "ensure it does not access torch.Tensor objects (CPU or GPU) inside the loop. Instead, the code should use .tolist() to convert batched data tensors to Python lists beforehand."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/_torch/auto_deploy/custom_ops/attention_interface.py` around lines 606 - 619, The property accessors seq_len, input_pos, cache_loc, and pages_per_seq currently call .tolist() on every access causing repeated tensor-to-list conversions; modify nest_sequences to call these properties once at the start (e.g., seq_len_list = self.seq_len, input_pos_list = self.input_pos, cache_loc_list = self.cache_loc, pages_per_seq_list = self.pages_per_seq) and use those local lists throughout the method (instead of repeated self.<prop> accesses) so no tensor .tolist() is performed inside loops; alternatively, compute and store the .tolist() results in local variables immediately after entering nest_sequences and reference those locals everywhere in the method.
214-235: Docstring should specify CPU tensor requirement.The implementation at lines 233-234 calls
.numpy()on the input tensor, which requires the tensor to be on CPU. While internal callers handle this correctly, the method's docstring should clarify this constraint to prevent misuse.📝 Suggested docstring update
"""Store a tensor into the pinned host buffer. Args: name: Name of the tensor to store to. - data: 1-D torch.Tensor to store. + data: 1-D torch.Tensor on CPU to store (GPU tensors not supported). fill_value: Optional value to fill the entire buffer with before storing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tensorrt_llm/_torch/auto_deploy/custom_ops/attention_interface.py` around lines 214 - 235, Update the docstring for the "Store a tensor into the pinned host buffer." method to state that the data argument must be a CPU torch.Tensor (1-D) because the implementation calls .numpy() on the input (which requires the tensor to be on CPU); explicitly document any expectations about dtype conversion (the method will convert to the buffer dtype if needed) and mention that callers should provide data on CPU to avoid runtime errors when invoking host_view[:length].numpy() and data.numpy().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tensorrt_llm/_torch/auto_deploy/custom_ops/attention_interface.py`:
- Around line 44-65: Add a short comment above the _TORCH_TO_NUMPY_DTYPE mapping
(or near the _list_to_tensor function) noting that torch.bfloat16 is
intentionally omitted because NumPy lacks native bfloat16 support, and that the
fallback path in _list_to_tensor (torch.tensor(...)) will handle bfloat16
tensors; reference the _TORCH_TO_NUMPY_DTYPE constant and the _list_to_tensor
function so maintainers know why bfloat16 isn't mapped to a NumPy dtype.
- Around line 606-619: The property accessors seq_len, input_pos, cache_loc, and
pages_per_seq currently call .tolist() on every access causing repeated
tensor-to-list conversions; modify nest_sequences to call these properties once
at the start (e.g., seq_len_list = self.seq_len, input_pos_list =
self.input_pos, cache_loc_list = self.cache_loc, pages_per_seq_list =
self.pages_per_seq) and use those local lists throughout the method (instead of
repeated self.<prop> accesses) so no tensor .tolist() is performed inside loops;
alternatively, compute and store the .tolist() results in local variables
immediately after entering nest_sequences and reference those locals everywhere
in the method.
- Around line 214-235: Update the docstring for the "Store a tensor into the
pinned host buffer." method to state that the data argument must be a CPU
torch.Tensor (1-D) because the implementation calls .numpy() on the input (which
requires the tensor to be on CPU); explicitly document any expectations about
dtype conversion (the method will convert to the buffer dtype if needed) and
mention that callers should provide data on CPU to avoid runtime errors when
invoking host_view[:length].numpy() and data.numpy().
|
/bot run |
|
PR_Github #36413 [ run ] triggered by Bot. Commit: |
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #36428 [ run ] triggered by Bot. Commit: |
|
PR_Github #36428 [ run ] completed with state |
Signed-off-by: Eran Geva <egeva@oci-hsg-cs-001-login-01.cm.cluster>
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #36654 [ run ] triggered by Bot. Commit: |
|
@lucaslie please approve the PR |
|
PR_Github #36654 [ run ] completed with state
|
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #36768 [ run ] triggered by Bot. Commit: |
Signed-off-by: Eran Geva <19514940+MrGeva@users.noreply.github.com>
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #36783 [ run ] triggered by Bot. Commit: |
|
PR_Github #36783 [ run ] completed with state
|
|
/bot run --extra-stage "DGX_B200-4_GPUs-AutoDeploy-1, DGX_H100-4_GPUs-AutoDeploy-1" --disable-fail-fast |
|
PR_Github #36814 [ run ] triggered by Bot. Commit: |
|
@lucaslie fixed the issue, can you please review? |
|
PR_Github #36814 [ run ] completed with state |
… context (NVIDIA#11624) Signed-off-by: Eran Geva <egeva@nvl72064-T17.cm.cluster> Signed-off-by: Eran Geva <egeva@oci-hsg-cs-001-login-01.cm.cluster> Co-authored-by: Eran Geva <egeva@nvl72064-T17.cm.cluster> Co-authored-by: Eran Geva <egeva@oci-hsg-cs-001-login-01.cm.cluster>
… context (NVIDIA#11624) Signed-off-by: Eran Geva <egeva@nvl72064-T17.cm.cluster> Signed-off-by: Eran Geva <egeva@oci-hsg-cs-001-login-01.cm.cluster> Co-authored-by: Eran Geva <egeva@nvl72064-T17.cm.cluster> Co-authored-by: Eran Geva <egeva@oci-hsg-cs-001-login-01.cm.cluster>
Summary by CodeRabbit
Release Notes
New Features
Refactor
InputBuffer.store()method signature to accepttorch.Tensorinstead of lists.Description
Test Coverage
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 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.