Fix multimask batching: load the batched tail at MULTI_MASK_BATCH_SIZE - #8
Fix multimask batching: load the batched tail at MULTI_MASK_BATCH_SIZE#8attevon-admin wants to merge 1 commit into
Conversation
The loader requested wespeaker-multimask-tail-b64.onnx (PRIMARY_BATCH_SIZE) but the exporter writes -b32 (MULTI_MASK_BATCH_SIZE) and every runtime buffer is sized 32. Consequences: the batched session is silently None, so multi_mask_batch_size() falls back to 1 and the CUDA pipeline flushes one chunk at a time (per-chunk fbank + batch-1 predicts, trace: flushes == chunks); and a genuine batch-64 graph placed under the expected name overruns the 32-sized buffers and kills the embedding worker (receiver disconnected). Measured after the fix (RTX 3080 Ti, 36-min AMI meeting ES2004a): batching engages (flushes 33 for 1041 chunks), RTTM bit-identical to the batch-1 path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe multi-mask ONNX session lookup now uses ChangesMulti-mask session lookup
Repository ignore rule
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: ⚪ Minimal · up to This is a localized batching change with no actionable merge-blocking risk remaining beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.) ✨ 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 |
|
| Filename | Overview |
|---|---|
| src/inference/embedding/load/sessions.rs | Correctly changes optional multi-mask session discovery from batch 64 to batch 32, matching exported assets and runtime tensor capacities. |
| .gitignore | Adds a repository-local workspace directory to ignored files without affecting runtime behavior. |
Reviews (1): Last reviewed commit: "Fix multimask batching: load the batched..." | Re-trigger Greptile
Closes reported perf issue from #7.
Before: trace shows
flushes == chunks(batch size 1, per-chunk fbank).After:
flushes=33for 1041 chunks on a 36-min AMI meeting; RTTM bit-identical to the batch-1 path. Crash repro: exporting a true batch-64 multimask graph under the expected-b64name kills the embedding worker ("receiver disconnected") because runtime buffers are sized 32 — reproduced on RTX 3080 Ti.Summary by CodeRabbit
Bug Fixes
Chores