bench(moe_deepseek): scope autotune(True) to pre-warm only - #3301
nv-yunzheq merged 1 commit into
Conversation
Addresses Codex review on PR flashinfer-ai#3292: when bench_gpu_time falls back to CUDA events (i.e. --no-cuda-graph --no-cupti), the autotune(True) context covers the entire measurement loop, so every measured iteration's choose_one runs in tuning mode and walks the cache before launching kernels. CUDA events queued into an idle stream fire when the host issues them, so the host-side cache-lookup gap ends up inside the event interval and inflates the reported latency. Post-flashinfer-ai#3126 the inflation is much smaller than the original pollution f3beb602 fought (no tensor synthesis on cache hit, just a dict lookup), but it can still be ~us-scale -- meaningful at very small N where total per-iter is ~10-20us. Default mode (graphs+cupti both on) is unaffected: CUPTI bypasses event-based timing and CUDA graphs capture host-side autotune cost at capture time, not replay time. This change only matters for the double-disable debug path. Move the autotune scope to wrap each backend's pre-warm + sync only; bench_gpu_time runs outside the scope, so its measurement loop hits the autotune cache via the non-tuning fast path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 benchmark refactors autotuning scope from a global wrapper around the token-count measurement loop to localized pre-warm phases within each backend. Three backend functions and their orchestration layer receive a ChangesAutotune Scope Relocation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the autotuning logic in the DeepSeek-V3 MoE benchmarks by moving the autotune context from the main benchmark loop to the pre-warm phase of individual backend functions. This change ensures that autotuning occurs on the default stream to avoid illegal memory access errors during CUDA graph capture and prevents host-side cache lookups from interfering with GPU timing measurements. I have no feedback to provide as there were no review comments.
<!-- .github/pull_request_template.md --> ## 📌 Description `CuteDslMoEWrapper` currently passes `self._forward_with_tactic` as a bound method into `CuteDslFusedMoENvfp4Runner`, creating a strong reference cycle: `wrapper -> runner -> bound method -> wrapper`. When the wrapper is used with `use_cuda_graph=True`, this can keep wrapper-owned CUDA graph resources alive after user code has dropped the wrapper, until Python cyclic GC eventually runs. This PR replaces that bound-method callback with a weakref trampoline. The runner can still call into a live wrapper, but it no longer owns the wrapper lifetime. This prevents stale wrapper CUDA resources from surviving across same-process tests or later autotune runs. ## 🔍 Related Issues #3286 #3301 #3252 ## 🚀 Pull Request Checklist Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete. ### ✅ Pre-commit Checks - [x] I have installed `pre-commit` by running `pip install pre-commit` (or used your preferred method). - [x] I have installed the hooks with `pre-commit install`. - [x] I have run the hooks manually with `pre-commit run --all-files` and fixed any reported issues. > If you are unsure about how to set up `pre-commit`, see [the pre-commit documentation](https://pre-commit.com/). ## 🧪 Tests Adds a focused regression test that warms a CUDA-graph wrapper, verifies it is finalized before cyclic GC, and then runs a subsequent autotuned wrapper call to ensure the output remains NaN-free. - [x] Tests have been added or updated as needed. - [x] All tests are passing (`unittest`, etc.). ## Reviewer Notes <!-- Optional: anything you'd like reviewers to focus on, concerns, etc. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling and cleanup of CUDA-graph wrappers to prevent resource leaks and provide a clear error when a wrapper is no longer available. * **Tests** * Added lifetime tests covering CUDA-graph wrappers before and after autotune; verify stable, non-NaN outputs during autotune. * **Documentation** * Updated comment about cold-L2 cache behavior and noted follow-up to re-enable it once a related issue is addressed. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/flashinfer-ai/flashinfer/pull/3340?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📌 Description
Scopes
autotune(True)to wrap only the per-backend pre-warm pass instead of the entire benchmark loop, so the measurement iterations themselves run withis_tuning_mode=False.Verified on B200 in NGC rc14 across all four measurement modes (graphs × CUPTI, on × off) at
--num-tokens 128 --ep 8. Default mode (graphs + CUPTI) is byte-identical: CuteDSL 0.147 ms / TRTLLM 0.143–0.144 ms on both branches, matching the #3292 baseline. The CUDA-events fallback path (--no-cuda-graph --no-cupti) drops from CuteDSL 1.389 ms → 0.197 ms (~7×); at N=1 EP=16, CUTLASS drops from 2.162 ms → 0.051 ms (42×). Graphs-on / CUPTI-off and graphs-off / CUPTI-on modes track accordingly — no regression in any graphs-enabled path, substantial improvement in every graphs-disabled path.Also updates the now-stale
run_benchmarkdocstring.🔍 Related Issues
#3292
#3126
🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Reviewer Notes
Summary by CodeRabbit
Release Notes