[None][refactor] Unify the usage of MPIDist and TorchDist. #10380
Conversation
yuxianq
requested review from
dongxuy04,
greg-kwasniewski1,
joyang-nv and
nv-yilinf
|
/bot run --disable-fail-fast |
📝 WalkthroughWalkthroughThis refactoring replaces MPI-based distributed communication (MPIDist) with a unified Distributed abstraction across the codebase. A new factory method Distributed.get(mapping) provides backend selection with caching. Enhanced allreduce methods with ReduceOp parameters support flexible reduction operations. Imports and function signatures are updated consistently throughout affected modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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. Comment |
![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)
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py (1)
13-80: CUDAGraphRunner now correctly depends onDistributedinstead ofMPIDistUpdating
CUDAGraphRunnerConfig.disttoOptional[Distributed]and importingDistributedkeeps this component aligned with the new abstraction while preserving existingtp_allgatherusage. Given this runner only consultsdistwhenenable_attention_dpandmapping.tp_size > 1, you might optionally add anassert self.config.dist is not Nonein those branches to fail fast if the config is misconstructed, but the refactor itself looks sound.tensorrt_llm/_torch/pyexecutor/model_engine.py (1)
38-39: AligningPyTorchModelEnginewithDistributedis correct; consider tightening theDistributedAPIUpdating:
- the import to
from ..distributed import Distributed, and- the constructor parameter to
dist: Optional[Distributed] = None(propagated intoCUDAGraphRunnerConfig(dist=self.dist))correctly reflects that this engine no longer depends on a concrete MPIDist type and instead uses the unified abstraction. All existing call sites that use
self.dist.tp_allgather(...)remain valid at runtime as long as your concrete Distributed backends implementtp_allgather.For long‑term API clarity and type‑checking, consider adding
tp_allgather(and any other relied‑on TP helpers) to theDistributedABC so that the interface guarantees match whatPyTorchModelEngineandCUDAGraphRunneractually use.Also applies to: 128-171, 428-449
tensorrt_llm/_utils.py (1)
443-451: Consider G_ prefix for module-level dictionary.The dictionary
_torch_reduce_op_to_mpi_dictis a module-level mapping. Per coding guidelines, "Python global variables should use upper snake_case with prefix 'G'". Consider renaming toG_TORCH_REDUCE_OP_TO_MPI_DICTfor consistency with the project's naming convention for global constants.🔎 Proposed naming adjustment
-_torch_reduce_op_to_mpi_dict = { +G_TORCH_REDUCE_OP_TO_MPI_DICT = { torch.distributed.ReduceOp.SUM: MPI.SUM, torch.distributed.ReduceOp.PRODUCT: MPI.PROD, torch.distributed.ReduceOp.MIN: MPI.MIN, torch.distributed.ReduceOp.MAX: MPI.MAX, torch.distributed.ReduceOp.BAND: MPI.BAND, torch.distributed.ReduceOp.BOR: MPI.BOR, torch.distributed.ReduceOp.BXOR: MPI.BXOR, }And update the function:
def torch_reduce_op_to_mpi(op: torch.distributed.ReduceOp) -> MPI.Op: - ret = _torch_reduce_op_to_mpi_dict.get(op) + ret = G_TORCH_REDUCE_OP_TO_MPI_DICT.get(op) assert ret is not None, f'Unsupported reduce op: {op}' return ret
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tensorrt_llm/_ipc_utils.pytensorrt_llm/_torch/auto_deploy/shim/ad_executor.pytensorrt_llm/_torch/distributed/communicator.pytensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.pytensorrt_llm/_torch/pyexecutor/resource_manager.pytensorrt_llm/_utils.pytests/unittest/others/test_kv_cache_transceiver.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
**/*.py: 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
Python files should use snake_case naming:some_file.py
Python classes should use PascalCase naming:class SomeClass
Python functions and methods should use snake_case naming:def my_awesome_function():
Python local variables should use snake_case naming:my_variable = ...
Python variable names that start with a number should be prefixed with 'k':k_99th_percentile = ...
Python global variables should use upper snake_case with prefix 'G':G_MY_GLOBAL = ...
Python constants should use upper snake_case naming: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 in Python for classes and functions, which can be parsed by Sphinx
Python attributes and variables can be documented inline with type and description
Avoid using reflection in Python when functionality can be easily achieved without reflection
When using try-except blocks in Python, limit the except to the smallest set of errors possible
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, using the else block for logic
Files:
tensorrt_llm/_utils.pytensorrt_llm/_ipc_utils.pytensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytests/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.pytensorrt_llm/_torch/distributed/communicator.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/resource_manager.pytensorrt_llm/_torch/auto_deploy/shim/ad_executor.py
**/*.{cpp,h,cu,cuh,py}
📄 CodeRabbit inference engine (CODING_GUIDELINES.md)
All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the year of its latest meaningful modification
Files:
tensorrt_llm/_utils.pytensorrt_llm/_ipc_utils.pytensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytests/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.pytensorrt_llm/_torch/distributed/communicator.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/resource_manager.pytensorrt_llm/_torch/auto_deploy/shim/ad_executor.py
🧠 Learnings (22)
📚 Learning: 2025-08-14T06:36:40.701Z
Learnt from: timlee0212
Repo: NVIDIA/TensorRT-LLM PR: 6886
File: tensorrt_llm/_torch/models/modeling_deepseekv3.py:0-0
Timestamp: 2025-08-14T06:36:40.701Z
Learning: In DeepSeek V3 model (tensorrt_llm/_torch/models/modeling_deepseekv3.py), the disagreement between AllReduce.__init__ guard and _compute_mlp_tp_size logic for MNNVL usage is expected by design. The AllReduce component and MLP TP-size computation intentionally use different criteria for MNNVL availability decisions.
Applied to files:
tensorrt_llm/_utils.pytensorrt_llm/_torch/distributed/communicator.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 Learning: 2025-09-02T13:42:44.885Z
Learnt from: pcastonguay
Repo: NVIDIA/TensorRT-LLM PR: 7455
File: tensorrt_llm/_torch/pyexecutor/py_executor.py:1852-1860
Timestamp: 2025-09-02T13:42:44.885Z
Learning: In MPI communication within TensorRT-LLM pipeline parallelism, different communication types (tokens, logits, termination sync) must use disjoint tag namespaces to avoid message routing collisions when using the same source/destination patterns.
Applied to files:
tensorrt_llm/_ipc_utils.pytests/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.pytensorrt_llm/_torch/distributed/communicator.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 Learning: 2025-09-16T09:30:09.716Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 7763
File: cpp/tensorrt_llm/CMakeLists.txt:297-301
Timestamp: 2025-09-16T09:30:09.716Z
Learning: In the TensorRT-LLM project, NCCL libraries are loaded earlier by PyTorch libraries or the bindings library, so the main shared library doesn't need NCCL paths in its RPATH - the libraries will already be available in the process address space when needed.
Applied to files:
tensorrt_llm/_ipc_utils.pytests/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/distributed/communicator.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 Learning: 2025-09-23T15:12:38.312Z
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/thop/allreduceOp.cpp:352-446
Timestamp: 2025-09-23T15:12:38.312Z
Learning: In TensorRT-LLM NCCL device implementation, NCCL version 2.28+ requirements are handled at runtime in the nccl_device/config layer rather than with compile-time guards. This allows the allreduceOp to remain version-agnostic and delegates version compatibility validation to the appropriate lower-level components that can gracefully handle unsupported configurations.
Applied to files:
tensorrt_llm/_ipc_utils.pytests/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.pytensorrt_llm/_torch/distributed/communicator.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 Learning: 2025-08-26T09:37:10.463Z
Learnt from: jiaganc
Repo: NVIDIA/TensorRT-LLM PR: 7031
File: tensorrt_llm/bench/dataclasses/configuration.py:90-104
Timestamp: 2025-08-26T09:37:10.463Z
Learning: In TensorRT-LLM's bench configuration, the `get_pytorch_perf_config()` method returns `self.pytorch_config` which is a Dict[str, Any] that can contain default values including `cuda_graph_config`, making the fallback `llm_args["cuda_graph_config"]` safe to use.
Applied to files:
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.py
📚 Learning: 2025-08-19T12:45:11.997Z
Learnt from: amitz-nv
Repo: NVIDIA/TensorRT-LLM PR: 7033
File: tensorrt_llm/_torch/pyexecutor/model_engine.py:0-0
Timestamp: 2025-08-19T12:45:11.997Z
Learning: In tensorrt_llm/_torch/pyexecutor/model_engine.py, DoRA (Delta Orthogonal Rank Adaptation) functionality was removed from the PyTorch flow to eliminate issues with inverted DoRA detection logic. The original is_dora condition was checking if scaling_vec_pointer == 0, which was potentially incorrect.
Applied to files:
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/resource_manager.pytensorrt_llm/_torch/auto_deploy/shim/ad_executor.py
📚 Learning: 2025-08-26T09:37:10.463Z
Learnt from: jiaganc
Repo: NVIDIA/TensorRT-LLM PR: 7031
File: tensorrt_llm/bench/dataclasses/configuration.py:90-104
Timestamp: 2025-08-26T09:37:10.463Z
Learning: In TensorRT-LLM, the `get_pytorch_perf_config()` method returns `self.pytorch_config` which can contain default `cuda_graph_config` values, so `llm_args` may already have this config before the extra options processing.
Applied to files:
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.py
📚 Learning: 2025-12-12T03:27:08.565Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 9655
File: tensorrt_llm/_torch/pyexecutor/sampler.py:3031-3031
Timestamp: 2025-12-12T03:27:08.565Z
Learning: In files under tensorrt_llm/_torch/pyexecutor, avoid accessing torch.Tensor objects inside for-loops when iterating over requests. Convert batched tensors to Python lists beforehand using tensor.tolist(), and then iterate over those lists. This improves performance by reducing tensor-bound operations inside hot loops. Apply this pattern to similar code paths that process batches to access simple Python data structures (lists) inside loops.
Applied to files:
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 Learning: 2025-09-24T03:31:28.908Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 7520
File: tensorrt_llm/_torch/pyexecutor/resource_manager.py:605-613
Timestamp: 2025-09-24T03:31:28.908Z
Learning: In TensorRT-LLM Ray orchestrator mode, ProcessGroups are initialized with both Gloo and NCCL backends (e.g., "cuda:nccl,cpu:gloo"), allowing PyTorch distributed to automatically route CPU tensors through Gloo and GPU tensors through NCCL. This eliminates the need for manual device placement when performing allreduce operations on base types.
Applied to files:
tests/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/distributed/communicator.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 Learning: 2025-08-01T15:14:45.673Z
Learnt from: yibinl-nvidia
Repo: NVIDIA/TensorRT-LLM PR: 6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.
Applied to files:
tests/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.py
📚 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/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.py
📚 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/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/pyexecutor/py_executor_creator.py
📚 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:
tests/unittest/others/test_kv_cache_transceiver.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 Learning: 2025-08-06T13:58:07.506Z
Learnt from: galagam
Repo: NVIDIA/TensorRT-LLM PR: 6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.
Applied to files:
tests/unittest/others/test_kv_cache_transceiver.py
📚 Learning: 2025-08-19T12:45:35.429Z
Learnt from: amitz-nv
Repo: NVIDIA/TensorRT-LLM PR: 7033
File: tensorrt_llm/_torch/pyexecutor/model_engine.py:2086-2092
Timestamp: 2025-08-19T12:45:35.429Z
Learning: DoRA (Delta Orthogonal Rank Adaptation) functionality has been removed from the PyTorch flow in tensorrt_llm/_torch/pyexecutor/model_engine.py. The is_dora field is computed but not used downstream in the PyTorch flow, so converting it to a tensor would be wasteful overhead.
Applied to files:
tensorrt_llm/_torch/pyexecutor/py_executor_creator.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 Learning: 2025-08-14T15:38:01.771Z
Learnt from: MatthiasKohl
Repo: NVIDIA/TensorRT-LLM PR: 6904
File: cpp/tensorrt_llm/pybind/thop/bindings.cpp:55-57
Timestamp: 2025-08-14T15:38:01.771Z
Learning: In TensorRT-LLM Python bindings, tensor parameter collections like mla_tensor_params and spec_decoding_tensor_params are kept as required parameters without defaults to maintain API consistency, even when it might affect backward compatibility.
Applied to files:
tensorrt_llm/_torch/pyexecutor/py_executor_creator.py
📚 Learning: 2025-08-27T14:23:55.566Z
Learnt from: ixlmar
Repo: NVIDIA/TensorRT-LLM PR: 7294
File: tensorrt_llm/_torch/modules/rms_norm.py:17-17
Timestamp: 2025-08-27T14:23:55.566Z
Learning: The TensorRT-LLM project requires Python 3.10+ as evidenced by the use of TypeAlias from typing module, match/case statements, and union type | syntax throughout the codebase, despite some documentation still mentioning Python 3.8+.
Applied to files:
tensorrt_llm/_torch/pyexecutor/py_executor_creator.py
📚 Learning: 2025-08-14T15:43:23.107Z
Learnt from: MatthiasKohl
Repo: NVIDIA/TensorRT-LLM PR: 6904
File: tensorrt_llm/_torch/attention_backend/trtllm.py:259-262
Timestamp: 2025-08-14T15:43:23.107Z
Learning: In TensorRT-LLM's attention backend, tensor parameters in the plan() method are assigned directly without validation (dtype, device, contiguity checks). This maintains consistency across all tensor inputs and follows the pattern of trusting callers to provide correctly formatted tensors.
Applied to files:
tensorrt_llm/_torch/pyexecutor/py_executor_creator.py
📚 Learning: 2025-09-23T15:12:38.312Z
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/thop/allreduceOp.cpp:352-446
Timestamp: 2025-09-23T15:12:38.312Z
Learning: In TensorRT-LLM NCCL device allreduce implementation (cpp/tensorrt_llm/thop/allreduceOp.cpp), the goto pattern in runNCCLAllReduceDeviceFusion is intentionally used for future extensibility, allowing multiple switch cases to fallback to the default handler. While not aesthetically ideal, this pattern supports adding more fusion cases later that can reuse the same fallback logic.
Applied to files:
tensorrt_llm/_torch/distributed/communicator.py
📚 Learning: 2025-10-13T19:45:03.518Z
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: tests/unittest/_torch/multi_gpu/test_nccl_device.py:138-149
Timestamp: 2025-10-13T19:45:03.518Z
Learning: In test_nccl_device.py, the NCCL device AllReduce implementation compares the entire residual tensor on each rank, unlike the UB implementation which compares per-rank chunks. The residual chunking calculations in the test are intentionally overridden to reflect this design difference.
Applied to files:
tensorrt_llm/_torch/distributed/communicator.py
📚 Learning: 2025-08-26T06:07:02.166Z
Learnt from: shaharmor98
Repo: NVIDIA/TensorRT-LLM PR: 7231
File: tensorrt_llm/_torch/pyexecutor/_util.py:504-509
Timestamp: 2025-08-26T06:07:02.166Z
Learning: In tensorrt_llm/_torch/pyexecutor/_util.py, when calling model_engine.set_lora_model_config(), pass model_binding_config.mlp_hidden_size directly without multiplying by mapping.tp_size, as the mlp_hidden_size from get_bindings_model_config() is already the per-TP rank value needed for LoRA weight packaging.
Applied to files:
tensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/resource_manager.py
📚 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:
tensorrt_llm/_torch/pyexecutor/resource_manager.py
🧬 Code graph analysis (7)
tensorrt_llm/_ipc_utils.py (1)
tensorrt_llm/_torch/distributed/communicator.py (4)
Distributed(35-134)get(42-46)tp_allgather(427-428)tp_allgather(685-698)
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py (1)
tensorrt_llm/_torch/distributed/communicator.py (1)
Distributed(35-134)
tests/unittest/others/test_kv_cache_transceiver.py (1)
tensorrt_llm/_torch/distributed/communicator.py (2)
Distributed(35-134)get(42-46)
tensorrt_llm/_torch/pyexecutor/py_executor_creator.py (3)
tensorrt_llm/_utils.py (1)
get_sm_version(769-771)tests/integration/defs/conftest.py (1)
get_sm_version(1910-1913)tensorrt_llm/_torch/distributed/communicator.py (2)
Distributed(35-134)get(42-46)
tensorrt_llm/_torch/distributed/communicator.py (3)
tensorrt_llm/_utils.py (3)
torch_reduce_op_to_mpi(454-457)mpi_disabled(550-552)mpi_comm(521-522)tensorrt_llm/mapping.py (1)
Mapping(363-542)cpp/tensorrt_llm/runtime/utils/mpiUtils.cpp (2)
allreduce(396-404)allreduce(396-396)
tensorrt_llm/_torch/pyexecutor/model_engine.py (1)
tensorrt_llm/_torch/distributed/communicator.py (1)
Distributed(35-134)
tensorrt_llm/_torch/pyexecutor/resource_manager.py (1)
tensorrt_llm/_torch/distributed/communicator.py (5)
Distributed(35-134)get(42-46)allreduce(129-134)allreduce(447-453)allreduce(668-682)
⏰ 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)
tensorrt_llm/_torch/auto_deploy/shim/ad_executor.py (3)
52-71: Imports forDistributedandinitialize_or_skiplook consistentSwitching to
from ...distributed import Distributedand importinginitialize_or_skipfrom the common distributed utilities is consistent with the new abstraction and keeps auto-deploy aligned with the rest of the stack. No issues here.
881-931: Draft model engine now correctly uses the unifiedDistributedinterfacePlumbing
dist: Distributedintocreate_draft_model_engine_maybeand passing it down toPyTorchModelEnginematches the new distributed abstraction and keeps target/draft engines on the same backend. The change is type- and behavior-preserving relative to the previous MPIDist usage.
996-1170: VerifyDistributed.get+ broadcast ordering when MPI is disabledThe new flow:
- builds
dist_mappingfrommpi_world_size()/mpi_rank()- creates
dist = Distributed.get(dist_mapping)- calls
port = dist.broadcast(dist.get_free_port())- then calls
initialize_or_skip(rank, world_size, port)is fine for the MPI-backed case, but when
mpi_disabled()isTrueandDistributed.getreturns a TorchDist backend,dist.broadcastmay rely on a torch.distributed process group thatinitialize_or_skiphas not yet created.Please double-check that:
- In non-MPI setups, either
Distributed.getdoes not return a TorchDist that requires an initialized PG at this point, or- TorchDist.broadcast / get_free_port are implemented so they work before PG initialization, or
- This code path is guaranteed to run only under MPI-backed configurations.
If not, you may need to initialize the process group (or a minimal rendezvous) before using
dist.broadcastin the TorchDist path.tensorrt_llm/_torch/pyexecutor/py_executor_creator.py (2)
16-17: Reusingget_sm_version()from_utilsis a good consolidationImporting
get_sm_versionfromtensorrt_llm._utilsand using it for the MLA/SM gating logic avoids duplicating the CUDA property lookup in this module and keeps SM-version checks consistent with the rest of the codebase. No issues from a correctness standpoint.Also applies to: 446-480
30-31:Distributed.get(mapping)integration increate_py_executorlooks correctCreating
mapping = _get_mapping(...)and thendist = Distributed.get(mapping=mapping)before wiringdistintoPyTorchModelEngineandcreate_py_executor_instancealigns this path with the new distributed abstraction and the usage inModelEngine/CUDAGraphRunner. Assuming Distributed implementations preserve the previous MPIDist semantics for tp/pp groups, this is a behavior-preserving refactor.Please ensure your Distributed backends are exercised by the existing PyExecutor tests (including attention‑DP and TP/PP configurations) to confirm no regressions in all‑gather/all‑reduce behavior under the new abstraction.
Also applies to: 305-307
tests/unittest/others/test_kv_cache_transceiver.py (1)
9-10: Tests correctly updated to useDistributed.get(mapping=...)Switching the tests to:
- import
Distributedand- construct
dist = Distributed.get(mapping=mapping)forMapping(world_size=1, rank=0)keeps the kv‑cache transceiver tests aligned with the new distributed abstraction while remaining behaviorally equivalent in the single‑process case. This should help catch regressions in the Distributed implementation going forward.
Also applies to: 72-90, 140-156
tensorrt_llm/_utils.py (1)
454-457: LGTM!The function correctly maps PyTorch ReduceOp to MPI.Op with proper type hints and error handling. The assertion approach is appropriate for detecting unsupported operations early.
tensorrt_llm/_torch/pyexecutor/resource_manager.py (2)
13-13: LGTM!The import correctly maintains the namespace and follows the project's coding guidelines for imports.
800-804: LGTM!The refactoring correctly replaces MPI-based communication with the unified Distributed abstraction. The use of
ReduceOp.MINensures all ranks use the smallestmax_tokensvalue, which is the correct behavior for resource allocation consistency.Based on learnings, the Ray orchestrator's dual backend (NCCL/Gloo) automatically handles device placement, so no manual tensor conversion is needed here.
tensorrt_llm/_ipc_utils.py (2)
107-109: LGTM!The import and instantiation of the Distributed abstraction correctly replaces the previous conditional MPI/torch path logic, simplifying the code while maintaining functionality.
120-120: LGTM!The use of
dist.tp_allgather(local_handle.reserved)correctly replaces the previous allgather implementation. This gathers CUDA IPC memory handles across the TP group, which is the appropriate scope for tensor-parallel communication. Thetp_allgathermethod handles the underlying type automatically.tensorrt_llm/_torch/distributed/communicator.py (6)
5-5: LGTM!The
lru_cacheimport is correctly added to support the new factory method caching.
22-23: LGTM!The imports for
torch_pybind11_abiandtorch_reduce_op_to_mpiare correctly added and used in the implementation.
40-46: LGTM!The factory method with caching correctly selects between
TorchDistandMPIDistbased on the MPI availability. Thelru_cacheoptimization is appropriate for avoiding repeated instantiation with the sameMapping.
128-134: LGTM!The abstract method signature correctly defines the allreduce interface with a flexible
objparameter and an explicitReduceOpparameter withSUMas the default. This aligns with standard distributed reduction conventions.
447-453: LGTM!The MPI-based allreduce implementation correctly converts PyTorch ReduceOp to MPI.Op using the utility function and performs the reduction through the MPI communicator.
668-682: LGTM!The PyTorch-based allreduce implementation correctly handles type conversions for base types (int/float) and performs the reduction using
torch.distributed.all_reduce. Based on learnings, the Ray orchestrator's dual backend (NCCL/Gloo) automatically routes operations appropriately.
|
PR_Github #30347 [ run ] triggered by Bot. Commit: |
|
PR_Github #30347 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #30354 [ run ] triggered by Bot. Commit: |
|
PR_Github #30354 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #30388 [ run ] triggered by Bot. Commit: |
|
PR_Github #30388 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #30427 [ run ] triggered by Bot. Commit: |
|
PR_Github #30427 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #30434 [ run ] triggered by Bot. Commit: |
|
PR_Github #30434 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #30454 [ run ] triggered by Bot. Commit: |
|
PR_Github #30454 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #30694 [ run ] triggered by Bot. Commit: |
|
PR_Github #30694 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #30740 [ run ] triggered by Bot. Commit: |
|
PR_Github #30740 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #30818 [ run ] triggered by Bot. Commit: |
|
PR_Github #30818 [ run ] completed with state
|
Signed-off-by: Yuxian Qiu <[email protected]>
|
/bot run --disable-fail-fast |
|
PR_Github #30879 [ run ] triggered by Bot. Commit: |
tensorrt_llm/_torch/autotuner.py
Outdated
| """Setup distributed communication state for autotuning.""" | ||
| self.mapping = mapping | ||
| self._dist = dist | ||
| self._dist = Distributed.get(mapping) |
There was a problem hiding this comment.
Hi @yuxianq. Do we have some similar methods for mapping? Or mapping is not a singleton across the programm?
I am also worried that some tests do not call setup_distributed_state before running tunable op will also encounter the similar issue that merge process can not work as expected.
There was a problem hiding this comment.
Mapping is not a singleton by design. Can we raise RuntimeError when some multi-gpu tests start merge process without calling setup_distributed_state?
| # TODO: Unified tp barrier for both MPIDist and TorchDist. | ||
| if hasattr(self._dist, "tp_comm"): | ||
| self._dist.tp_comm.barrier() | ||
| self._dist.tp_barrier() |
There was a problem hiding this comment.
Thanks for completing this part.
|
PR_Github #30879 [ run ] completed with state
|
Signed-off-by: Yuxian Qiu <[email protected]>
Signed-off-by: Yuxian Qiu <[email protected]>
Signed-off-by: Yuxian Qiu <[email protected]>
|
/bot run --disable-fail-fast |
|
PR_Github #30997 [ run ] triggered by Bot. Commit: |
|
PR_Github #30997 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
8 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
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.