[None][fix] Pass num_fused_shared_experts in FP8 MoE fallback-tactic path - #17362
[None][fix] Pass num_fused_shared_experts in FP8 MoE fallback-tactic path#17362brnguyen2 wants to merge 2 commits into
Conversation
…path PR NVIDIA#15297 added a numFusedSharedExpert argument to FP8BlockScaleMoERunner::getValidConfigs and updated the autotuner get_valid_tactics call site, but not the memoized fallback-tactic path used on autotuner cache misses. Any FP8 block-scale MoE forward that misses the profiling cache fails with: RuntimeError: get_valid_configs() is missing value for argument '_6'. Pass num_fused_shared_experts and include it in the memo key, matching get_valid_tactics. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
|
/bot run |
|
PR_Github #64333 [ run ] triggered by Bot. Commit: |
|
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 (1)
WalkthroughFP8 MoE autotuner identity and fallback-tactic validation keys now include ChangesFP8 MoE tactic caching
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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.
Inline comments:
In `@tensorrt_llm/_torch/custom_ops/trtllm_gen_custom_ops.py`:
- Around line 925-926: Update the unique_id() autotuner cache key to include
num_fused_shared_experts, matching the fallback key and get_valid_configs()
parameter validation. Add a regression test that verifies distinct fused
shared-expert counts produce distinct cache identities and do not reuse the same
autotuner entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a41c22c7-f985-4817-9376-3ac92d9f9085
📒 Files selected for processing (1)
tensorrt_llm/_torch/custom_ops/trtllm_gen_custom_ops.py
leslie-fang25
left a comment
leslie-fang25
left a comment
There was a problem hiding this comment.
Duplicated with #17328?
|
PR_Github #64333 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64373 [ run ] triggered by Bot. Commit: |
|
PR_Github #64373 [ run ] completed with state |
Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
| key = (self.top_k, hidden_size, self.intermediate_size, | ||
| self.local_num_experts, num_tokens) | ||
| key = (self.top_k, self.num_fused_shared_experts, hidden_size, | ||
| self.intermediate_size, self.local_num_experts, num_tokens) |
There was a problem hiding this comment.
This tuple is both the memo key and the positional argument list for get_valid_configs(*key) on line 931 — which is why #15297 updated the explicit get_valid_tactics() call site but missed this one. Could the two be split (pass the arguments explicitly, keep key as just the key), so both call sites look the same to a grep? Fine as a follow-up.
There was a problem hiding this comment.
Agreed. That tuple doing double duty as memo key and positional argument list for get_valid_configs(*key) is exactly why #15297 updated the explicit call site but missed this one. Splitting them so both call sites spell out the arguments (and the key stays just a key) would make this greppable; worth doing as a follow-up. Since the identical fix already merged via #17328, this PR is being closed, so the split would go as a separate cleanup against main.
|
Confirmed, @leslie-fang25 — #17328 carries the identical code change (same resulting file content for trtllm_gen_custom_ops.py) and merged first, so main already has the fix, plus the associated waive removals. This PR is superseded and can be closed. |
4 participants
Description
PR #15297 added a
numFusedSharedExpertargument toFP8BlockScaleMoERunner::getValidConfigsand updated the autotunerget_valid_tacticscall site, but not the memoized fallback-tactic path usedon autotuner cache misses. Any FP8 block-scale MoE forward that misses the
profiling cache now fails with:
Observed as a hard failure of
perf/test_perf_sanity.py::test_e2e[aggr_upload-host_perf_deepseek_v3_lite-v3lite_fp8_bs8_128_256]on two independent pre-merge pipelines whose bases include #15297; the
failure is intermittent because it depends on hitting the autotuner
cache-miss path.
Fix: pass
num_fused_shared_expertsand include it in the memo key, matchingthe
get_valid_tacticscall site on the same class (the valid-config setdepends on it, so it belongs in the key).
Test Coverage
Covered by the existing DeepSeek V3 Lite FP8 perf-sanity case, which
exercises the fallback path.
PR Checklist
[None][fix]conventionDev Engineer Review
tensorrt_llm/_torch/custom_ops/trtllm_gen_custom_ops.py.num_fused_shared_expertstoFP8BlockScaleMOERunner::getValidConfigs.num_fused_shared_expertsto the memoization key._6argument runtime error.QA Engineer Review
No test changes.