Fix: Missing args in allreduce_fusion MOE finalize call#3046
Fix: Missing args in allreduce_fusion MOE finalize call#3046samuellees wants to merge 2 commits intoflashinfer-ai:mainfrom
Conversation
…n API PR flashinfer-ai#2966 added quant_out, scale_out, and routed_scaling_factor params to trtllm_moe_finalize_allreduce_fusion(). PR flashinfer-ai#2982 (unified API) was developed before flashinfer-ai#2966 merged, and git merge produced no conflict since they touched different files (trtllm_ar.py vs allreduce.py). However the call in allreduce_fusion() was missing the three new positional args, causing TypeError at runtime for kMoEFinalizeARResidualRMSNorm pattern and mypy failure in pre-commit. Fix: - Add quant_out, scale_out, routed_scaling_factor to the finalize call - Add routed_scaling_factor to allreduce_fusion() function signature - Update docstring AI-assisted Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
…ing-args # Conflicts: # flashinfer/comm/allreduce.py
There was a problem hiding this comment.
Code Review
This pull request updates the allreduce_fusion function in flashinfer/comm/allreduce.py to include a new routed_scaling_factor parameter and its corresponding documentation. Additionally, it updates the internal operation call to include quant_out, scale_out, and routed_scaling_factor. I have no feedback to provide.
|
Close because of #3040 |
Problem
On main branch,
allreduce_fusion(pattern=kMoEFinalizeARResidualRMSNorm)crashes withTypeError: missing positional arguments. mypy pre-commit also fails.Root Cause
Two PRs merged to main in sequence, modifying different ends of the same call chain:
quant_out,scale_out,routed_scaling_factortotrtllm_moe_finalize_allreduce_fusion()signatureflashinfer/comm/trtllm_ar.pykMoEFinalizeARResidualRMSNormpattern that callstrtllm_moe_finalize_allreduce_fusion()flashinfer/comm/allreduce.pyPR #2982 was developed before #2966 merged. Git merge produced no conflict since they touched different files, but the call in
allreduce_fusion()was left with the old signature — missing the three new positional args added by #2966.Impact
allreduce_fusion(pattern=kMoEFinalizeARResidualRMSNorm)(pattern 7)allreduce_fusion(pattern=kMoEReductionARResidualRMSNorm)(pattern 6)allreduce_fusion(pattern=0-5)(standard allreduce)trtllm_moe_finalize_allreduce_fusion()direct callerstest_allreduce_fusion_moe_unified_api.pyfinalize testsPractical impact is limited — pattern 7 was just added in #2982 and has no downstream consumers yet.
Fix
flashinfer/comm/allreduce.py:quant_out,scale_out,routed_scaling_factorto thetrtllm_moe_finalize_allreduce_fusion()callrouted_scaling_factor: Optional[float] = Nonetoallreduce_fusion()signatureNo test changes needed — existing tests pass
Nonefor the new args (default values), which is the correct behavior for non-quantized finalize.PR
fix/moe-finalize-missing-argsfix: add missing args to moe_finalize call in unified allreduce_fusion APISummary by CodeRabbit