[None][fix] AutoDeploy: return fused_weight_dims so fused QKV split sizes are rescaled under TP - #15351
Conversation
…izes are rescaled under TP _determine_fused_weight_dims computed the fused dims into a local variable but fell off the end of the function with no return statement (and was annotated -> None), so callers always received None. As a result the "if fused_weight_dims is not None" branch in _process_column_sharding never ran, and the downstream split_with_sizes / slice sizes were left at their pre-sharding values after a column shard. For Phi-4 at TP=2 the fused QKV output dim is halved by the column shard (7680 to 3840) while the split sizes stay at [5120, 1280, 1280], so the wrong shape propagates until a row-parallel projection expects 2560 and the matmul fails. Fix: return the computed fused_weight_dims and correct the annotation to Optional[list]. No other logic changes; the existing rescaling branch now receives the dims it was always meant to get. Adds CPU-only regression tests for _determine_fused_weight_dims covering the fused QKV split case, the unfused (no fusion) case, and the multi-linear case. Signed-off-by: Srijan Upadhyay <srjnupadhyay@gmail.com>
📝 WalkthroughWalkthroughA missing return statement in ChangesFused Weight Dims Fix and Tests
Estimated code review effort: 2 (Simple) | ~10 minutes Related issues: Fixes Suggested labels: bug, auto-deploy, tests Suggested reviewers: (repository maintainers familiar with auto_deploy sharding transforms) 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tensorrt_llm/_torch/auto_deploy/transform/library/sharding.py (1)
2709-2711: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
list[int] | Noneinstead ofOptional[list].Per repo typing conventions, prefer built-in generics with the
|syntax overtyping.Optional, and specify the element type rather than a barelist.♻️ Proposed fix
def _determine_fused_weight_dims( linear_nodes: List[Node], -) -> Optional[list]: +) -> list[int] | None:As per coding guidelines, "Prefer built-in types
list,dict,tupleovertyping.List,typing.Dict,typing.Tuple; use|syntax instead oftyping.Union" and "Always annotate functions with return types... annotate class members and variables when necessary."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/_torch/auto_deploy/transform/library/sharding.py` around lines 2709 - 2711, Update the return type annotation on _determine_fused_weight_dims to use the repo’s preferred built-in generic syntax: replace Optional[list] with list[int] | None. Keep the change localized to the _determine_fused_weight_dims function signature in sharding.py, and make sure any related type hints in nearby code follow the same built-in generics style.Source: Coding guidelines
tests/unittest/auto_deploy/singlegpu/transformations/library/test_determine_fused_weight_dims.py (1)
45-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood regression coverage for the split path; slice/chunk branches remain untested.
The three tests solidly cover the
split_with_sizesregression path and twoNone-return guards. However,_determine_fused_weight_dimsalso haselifbranches fortorch.ops.aten.sliceusers (fused QKV via slicing, sharding.py lines 2731-2746) andtorch.ops.aten.chunkusers (sharding.py lines 2748-2754), both of which are also affected by the same missing-return bug and are not exercised here.Coverage is sufficient for the immediate Phi-4 regression (split-based fusion) but insufficient for the helper as a whole. Consider adding
test_fused_qkv_slice_returns_split_sizesandtest_fused_qkv_chunk_returns_split_sizesto this same file to close the gap, or track as a fast follow-up if out of scope for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/auto_deploy/singlegpu/transformations/library/test_determine_fused_weight_dims.py` around lines 45 - 102, Add regression tests for the untested fused paths in `_determine_fused_weight_dims`: create one test that feeds a linear node into `torch.ops.aten.slice` and another into `torch.ops.aten.chunk`, then assert the helper returns the expected fused sizes instead of None. Reuse the existing test setup pattern in `test_determine_fused_weight_dims.py` and the helper `_linear_node` so the new coverage matches the current split-based test and exercises the `slice` and `chunk` branches in `_determine_fused_weight_dims`.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tensorrt_llm/_torch/auto_deploy/transform/library/sharding.py`:
- Around line 2709-2711: Update the return type annotation on
_determine_fused_weight_dims to use the repo’s preferred built-in generic
syntax: replace Optional[list] with list[int] | None. Keep the change localized
to the _determine_fused_weight_dims function signature in sharding.py, and make
sure any related type hints in nearby code follow the same built-in generics
style.
In
`@tests/unittest/auto_deploy/singlegpu/transformations/library/test_determine_fused_weight_dims.py`:
- Around line 45-102: Add regression tests for the untested fused paths in
`_determine_fused_weight_dims`: create one test that feeds a linear node into
`torch.ops.aten.slice` and another into `torch.ops.aten.chunk`, then assert the
helper returns the expected fused sizes instead of None. Reuse the existing test
setup pattern in `test_determine_fused_weight_dims.py` and the helper
`_linear_node` so the new coverage matches the current split-based test and
exercises the `slice` and `chunk` branches in `_determine_fused_weight_dims`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 14b2a658-40df-45e2-8b75-0b889269e4db
📒 Files selected for processing (2)
tensorrt_llm/_torch/auto_deploy/transform/library/sharding.pytests/unittest/auto_deploy/singlegpu/transformations/library/test_determine_fused_weight_dims.py
Satisfies the formatting pre-commit hook, which merges an import statement onto a single line when it fits within the configured line length rather than keeping it in parenthesized multi-line form. Signed-off-by: Srijan Upadhyay <srjnupadhyay@gmail.com>
fcf4411 to
bbea605
Compare
|
/bot run |
|
PR_Github #58495 [ run ] triggered by Bot. Commit: |
|
PR_Github #58495 [ run ] completed with state |
Summary
Fixes #11220.
_determine_fused_weight_dims()computes fused projection dimensions for fused QKV layouts but never returns the computed value. As a result,_process_column_sharding()always receivesNoneand skips downstream split-size rescaling during tensor parallel sharding.Root Cause
The helper populates
fused_weight_dimsbut falls through without a return statement despite callers expecting the value.Fix
NonetoOptional[list]fused_weight_dimsValidation
Added CPU-only regression tests for:
The fused QKV test reproduces the regression (the helper returns
Nonebefore the fix); the other two pin existing behavior.Summary by CodeRabbit