[#14679][fix] Fix fused-QKV TP sharding for Phi-3/Phi-4 - #15475
[#14679][fix] Fix fused-QKV TP sharding for Phi-3/Phi-4#15475guan404ming wants to merge 2 commits into
Conversation
6f022ca to
302dcbe
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughFixes a bug in ChangesPhi-4 Shape Mismatch Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hi @govind-ramnarayan could you help take a look at this one, thanks! |
302dcbe to
72671fb
Compare
|
Hi @guan404ming ! Thank you for the contribution! We're currently in the process of moving over from our heuristic-based sharder to a model based on agentically-generated modeling hints. The idea is that we will have modeling code provide hints as to how each operation would like to be sharded, which provides more transparency and doesn't have as many corner cases as the purely structural approach. See e.g. With this in mind, a preferable solution would be to see if we can port the Microsoft-Phi models over to the new sharding IR without issue by adding similar "sharding hints" to the model. If it is not possible, then this solution to fix the legacy sharder looks fine to me, but I would like @greg-kwasniewski1 to confirm. He has a PR up to change our default mode of sharding to the new model-hints approach here: #14786 , which I think is fine with your current PR because it preserves the legacy approach as a fall-back, but I'd like to make sure the combination does not cause issues. |
4007c72 to
3436daf
Compare
|
Thanks @govind-ramnarayan! Just ported Validated on L4: single-GPU numerics match HF (MLP exact, full-model rmse ratio 0.0016) and the model exports cleanly; |
3436daf to
dbb3953
Compare
|
Hi @govind-ramnarayan could you help take another look at this one, really appreciate for your help! |
85e8aea to
c00f93f
Compare
|
Hi @greg-kwasniewski1, could you help take a look at this one, thanks! |
c8947bb to
792dc7c
Compare
govind-ramnarayan
left a comment
There was a problem hiding this comment.
Thanks! Sorry about the delay; we are currently in the process of moving AutoDeploy out of TRTLLM. This code change will still be useful for us so I'll see if I can merge it before the move.
|
/bot run |
|
PR_Github #59313 [ run ] triggered by Bot. Commit: |
|
PR_Github #59313 [ run ] completed with state
|
|
/bot run |
|
PR_Github #59430 [ run ] triggered by Bot. Commit: |
|
@govind-ramnarayan @guan404ming
So let's merge this PR as-is, I agree |
|
PR_Github #59430 [ run ] completed with state
|
792dc7c to
ce5f6b6
Compare
|
/bot run |
|
PR_Github #59510 [ run ] triggered by Bot. Commit: |
|
PR_Github #59510 [ run ] completed with state
|
|
/bot run |
|
PR_Github #59530 [ run ] triggered by Bot. Commit: |
|
PR_Github #59530 [ run ] completed with state
|
|
/bot run |
|
PR_Github #59565 [ run ] triggered by Bot. Commit: |
|
PR_Github #59565 [ run ] completed with state
|
|
Thanks for you all, feel free to let me know if there is anything need to refine! |
|
/bot run |
|
PR_Github #60080 [ run ] triggered by Bot. Commit: |
|
PR_Github #60080 [ run ] completed with state
|
Add modeling_phi3.py with explicit sharding-IR hints on the fused qkv_proj and gate_up_proj projections (output_sizes + split_with_sizes), mirroring modeling_nemotron_h's fused in_proj. Route the Phi-4 registry entries through the IR sharder via enable_sharder_ir.yaml. This fixes fused-QKV TP sharding for Phi-3/Phi-4 (NVIDIA#14679) through the model-hints path instead of patching the legacy structural sharder. Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Signed-off-by: Govind Ramnarayan <105831528+govind-ramnarayan@users.noreply.github.com>
Head branch was pushed to by a user without write access
4610f53 to
3f24beb
Compare
Summary by CodeRabbit
Release Notes
New Features
microsoft/phi-4,microsoft/Phi-4-reasoning,microsoft/Phi-4-reasoning-plus) for automatic deployment.Bug Fixes
Description
close #14679
_determine_fused_weight_dimscomputed the q/k/v split sizes but never returned them, so tensor-parallel column sharding of a fused qkv_proj got None and broke Phi-3/Phi-4 at TP≥2 (reduction-dim mismatch [s44*s70, 3840] X [2560, 5120]).Test Coverage
test_determine_fused_weight_dims_qkv-> New regression test. Exports a fused-QKV block and asserts_determine_fused_weight_dimsreturns the [q, k, v] split sizes (not None).test_tp_sharding.py-> Guards that the broader column/row TP-sharding path still produces correct sharded outputsPR 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)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.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
To see a list of available CI bot commands, please comment
/bot help.