Repository navigation
feat(mimo): add --lang mimo preset for local MiMo-V2.5-ASR - #26
Merged
Merged
Conversation
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds MiMo-V2.5-ASR support with 405 lines of new code in mimo_asr.py and comprehensive test coverage (421 lines, 26 tests). The architecture is well-designed (VAD → MiMo → CAM++ diarization with resume support).
However, I've identified 7 critical defects that block merge:
Critical Issues (Must Fix)
- Resume crash: Missing file existence check before
load_partial()causes FileNotFoundError - IndexError crash: Line 237 assumes no gaps in
completeddict, crashes on incomplete runs - Division by zero: Line 231 crashes when VAD returns empty/single-point segments
- Command injection: Unvalidated
audio_pathinextract_segment()allows arbitrary command execution (CWE-78) - Empty sequence crash:
np.stack()on line 311 crashes when all embeddings are None - Unbound variable:
mimocould be undefined in finally block if load fails - Shell injection: Unvalidated
$MIMO_WEIGHTS_PATHin setup_mimo.sh allows command execution (CWE-78)
Security Vulnerabilities
- Two command injection vulnerabilities (CWE-78) in
mimo_asr.pyandsetup_mimo.shrequire immediate fixes with input validation
All issues have actionable fixes with code suggestions. Please address these defects before merging.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
zxkane
added a commit
that referenced
this pull request
Apr 29, 2026
Amazon Q Developer flagged 7 issues; fixed the real ones and added regression tests. Triage: - [Real] --resume-mimo with no partial file → clear RuntimeError instead of FileNotFoundError (regression test added) - [Real, defensive] completed[] indexed read could KeyError if future refactor allows gaps; now asserts len(completed) == len(vad_segments) with a clear error message - [Real, defensive] _free_mimo(mimo) in finally could NameError if a future refactor moves _load_mimo inside the try; initialize mimo = None and guard - [Cosmetic] extract_segment now validates audio path exists before invoking ffmpeg, so failures surface with a clear message instead of "No such file or directory" from ffmpeg hundreds of segments in (regression test added). Added comment noting that subprocess.run with a list is immune to shell injection — no CWE-78 exposure. Declined suggestions: - "Command injection via audio_path" (mimo_asr.py): false positive. subprocess.run is called with a list (not shell=True); no shell interpretation occurs. File-exists check added for UX reasons only. - "Shell injection via MIMO_WEIGHTS_PATH" (setup_mimo.sh): false positive. Bash does not expand $(...) inside double-quoted variable values; the suggested regex filter would reject valid paths like /mnt/shared-team. - "np.stack crash on all-None embeddings" (assign_speakers_via_cam): already handled — len(embeds) < num_speakers early-returns, which covers len(embeds) == 0 for any num_speakers >= 2, and the guard at function entry short-circuits num_speakers <= 1. - "Division by zero in RTF calc": already guarded by if duration > 0.
Owner
Author
|
/q review |
There was a problem hiding this comment.
Review Summary
This PR adds a --lang mimo preset for local MiMo-V2.5-ASR inference with substantial implementation work (1158 insertions across 10 files). The core implementation appears well-structured with good error handling, retry logic, and resume capability.
Critical Issue (Must Fix)
Security Vulnerability: Line 1339 in transcribe_funasr.py sets the global HF_HOME environment variable (os.environ["HF_HOME"] = mimo_weights), which modifies process-wide state. This can interfere with other HuggingFace operations and cause cache conflicts. The mimo_asr.transcribe_with_mimo function already accepts a weights_path parameter, so this global modification is unnecessary and should be removed.
Recommendation
Address the critical security issue before merge. The rest of the implementation shows good practices including proper error handling, input validation, and comprehensive testing coverage.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Design for adding --lang mimo preset that runs XiaomiMiMo/MiMo-V2.5-ASR locally on GPU, reusing existing FSMN VAD + CAM++ diarization so output format matches --lang zh. Covers Python 3.12 migration, opt-in install via INSTALL_MIMO=1, GPU/VRAM preflight checks, per-segment retry, partial-state resume via --resume-mimo, and unit test plan.
14-task TDD plan for --lang mimo preset. Each task ships a focused commit: module scaffold, preflights (CUDA/VRAM, weights/repo), partial state I/O with audio hash, per-segment retry, VAD + segment extraction, CAM++ clustering adapter, orchestrator with resume, CLI wiring, opt-in setup_mimo.sh, Python 3.12 migration in setup_env.sh, SKILL.md docs, and CHANGELOG.
Pyright flagged that the tuple default (0.5, 2.0, 5.0) does not match the annotation 'list'. Sequence[float] accepts both tuple and list and is the right type for a values-to-iterate-over parameter. Also updated the plan so Tasks 5 and 8 use the correct annotation and Sequence is imported in the Task 1 scaffold.
Prior commit worked around a missing huggingface_hub package in the dev environment by (a) using isinstance-on-Exception in production code and (b) defining a local LocalEntryNotFoundError class in the test. Both were environment workarounds that polluted the code. The package is now installed; revert to the clean 'from huggingface_hub import snapshot_download' / 'from huggingface_hub.errors import LocalEntryNotFoundError' pattern specified in the plan. Test updates to import the real class while keeping the existing fake-module patch.dict for verifying the error path.
Add _extract_speaker_embedding and assign_speakers_via_cam using CAM++ embeddings + KMeans clustering; 1 new test brings suite to 18 passed.
Amazon Q Developer flagged 7 issues; fixed the real ones and added regression tests. Triage: - [Real] --resume-mimo with no partial file → clear RuntimeError instead of FileNotFoundError (regression test added) - [Real, defensive] completed[] indexed read could KeyError if future refactor allows gaps; now asserts len(completed) == len(vad_segments) with a clear error message - [Real, defensive] _free_mimo(mimo) in finally could NameError if a future refactor moves _load_mimo inside the try; initialize mimo = None and guard - [Cosmetic] extract_segment now validates audio path exists before invoking ffmpeg, so failures surface with a clear message instead of "No such file or directory" from ffmpeg hundreds of segments in (regression test added). Added comment noting that subprocess.run with a list is immune to shell injection — no CWE-78 exposure. Declined suggestions: - "Command injection via audio_path" (mimo_asr.py): false positive. subprocess.run is called with a list (not shell=True); no shell interpretation occurs. File-exists check added for UX reasons only. - "Shell injection via MIMO_WEIGHTS_PATH" (setup_mimo.sh): false positive. Bash does not expand $(...) inside double-quoted variable values; the suggested regex filter would reject valid paths like /mnt/shared-team. - "np.stack crash on all-None embeddings" (assign_speakers_via_cam): already handled — len(embeds) < num_speakers early-returns, which covers len(embeds) == 0 for any num_speakers >= 2, and the guard at function entry short-circuits num_speakers <= 1. - "Division by zero in RTF calc": already guarded by if duration > 0.
flash-attn source build requires the CUDA toolkit (nvcc), which is absent on most AWS GPU instances (driver-only). E2E test on g6e.4xlarge (L40S, driver CUDA 12.2, no nvcc) failed setup_mimo.sh step 3 with 'ERROR: nvcc not found'. Now detect installed torch version + cxx11abi + python minor at install time and download the matching pre-built wheel from the flash-attn GitHub release (v2.7.4.post1 ships 12 wheels covering torch 2.2-2.7 cross cp312 cross TRUE/FALSE). Only fall back to source build when no matching wheel exists AND nvcc is present. Verified matching wheel available for the instance under test: torch 2.6.0+cu124, cp312, cxx11abi=False -> cu12torch2.6cxx11abiFALSE. No behavior change when source build is required.
…outs huggingface_hub uses two incompatible path conventions: - HF_HOME=$X → weights land under $X/hub/models--…/snapshots/<rev>/ - snapshot_download(cache_dir=$X) → weights land under $X/models--…/… MIMO_WEIGHTS_PATH is described to users as an HF_HOME-style path (and setup_mimo.sh sets it that way during download), so the hub/ subdir ends up on disk. But passing that same path as cache_dir= at runtime fails with LocalEntryNotFoundError. Introduce _resolve_hf_snapshot helper that probes both layouts and returns the one that resolves. Use it in both require_mimo_installed() and _load_mimo(). E2E on AWS g6e.4xlarge (L40S) reproduced the bug after a successful INSTALL_MIMO=1 run left 34 GB under $MIMO_WEIGHTS_PATH/hub/ — preflight failed with 'MiMo weights not found'. Fix locked in with 3 regression tests for hub/bare/missing cases (229 total tests passing).
… deps
Two e2e findings from AWS g6e.4xlarge (L40S):
1. Upstream MiMo MimoAudio __init__ signature is
(model_path, mimo_audio_tokenizer_path, device=None)
— our _load_mimo was calling tokenizer_path=… which raises
TypeError: unexpected keyword argument 'tokenizer_path'.
The old name likely comes from an earlier README. Match the real
signature as of commit 98641d5.
2. MiMo's requirements.txt is missing two runtime imports:
- einops (imported via internal audio tokenizer modules)
- addict (imported via the 3D-Speaker gender classifier path,
which our pipeline loads as part of CAM++)
Install them explicitly in setup_mimo.sh step [2/4] so a fresh
install doesn't fail on ModuleNotFoundError mid-inference.
No tests added: both are wiring fixes against specific upstream
versions. A unit test that mocks MimoAudio wouldn't catch this
because the signature check happens inside real upstream code.
…rsion E2E finding on g6e.4xlarge (L40S): after MiMo loads on GPU, FunASR's CAM++ speaker model also runs on GPU and its spk_embedding output is a CUDA torch.Tensor. np.asarray() cannot view CUDA memory and raises TypeError: can't convert cuda:0 device type tensor to numpy. Use Tensor.cpu() to copy the tensor to host memory first. The failure is silently caught by the existing try/except and returns None from every segment. Downstream 'len(embeds) < num_speakers' trips the speaker=0 fallback, so diarization degrades to single-speaker output without failing loud. Detect torch.Tensor via hasattr(emb, 'detach'), then detach().cpu().numpy() before np.asarray. Two regression tests added: one for the CUDA-tensor path (asserts .cpu() is called), one for the pre-existing numpy path (asserts float32 coercion still works). 231 total tests passing.
Parallel fix to 8c50271 in rescore_montage_speakers. The inner extract_embedding helper has the same np.array(cuda_tensor) bug, which caused the montage re-scoring to fail silently on all embeddings: WARNING: embedding extraction failed at 285330ms: can't convert cuda:0 device type tensor to numpy. Downstream 'Only 0 speaker profile(s) built' then skipped montage re-scoring entirely. This happens on any GPU run (was latent before --lang mimo; MiMo just makes it reliably visible because CAM++ ends up on the same GPU). Same pattern: hasattr(emb, 'detach') → detach().cpu().numpy(), then np.array coerces to float32.
Two reports from the e2e validation run on AWS g6e.4xlarge (L40S 46 GB) against three real Chinese podcasts spanning 1 h to 6.75 h: - 2026-04-29-mimo-e2e-results.md: what ran, wall-clock + RTF, speaker diarization results, qualitative ASR accuracy diff, 5 bugs found and fixed (commits 89500b7, 2a84c54, bf8c0e1, 8c50271, 9861a01), artifact layout. - 2026-04-30-mimo-vs-funasr-perf-cost.md: head-to-head against a previously-run FunASR GPU benchmark. MiMo is ~8x slower and ~10-19x more expensive per episode on the same GPU, but clearly more accurate on proper nouns and code-switching. Private references to the upstream episode source repo, real podcast guest/host names, AWS instance IDs, and IAM role names have been redacted or generalized per project policy (CLAUDE.md).
Amazon Q Developer flagged os.environ["HF_HOME"] = mimo_weights in transcribe_funasr.py::main() as unnecessary global state modification (PR #26 review thread). For the CLI path this is safe: main() always passes weights_path= explicitly to transcribe_with_mimo(), _resolve_hf_snapshot then passes weights_path as explicit cache_dir= to huggingface_hub, and MimoAudio.__init__ receives absolute snapshot dirs (no Hub resolution, so HF_HOME is never consulted). Note: mimo_asr.transcribe_with_mimo() still falls back to os.environ.get("HF_HOME") when its weights_path parameter is None (mimo_asr.py:189) — that fallback is for direct library consumers, not the CLI. Those consumers set HF_HOME themselves if they want that behavior; we no longer do it for them at the CLI layer. 269 tests still pass.
Owner
Author
|
/q review |
There was a problem hiding this comment.
Summary
Reviewed PR #26: MiMo-V2.5-ASR integration for local GPU inference. The implementation adds a new --lang mimo preset with comprehensive testing (26 new tests, all passing) and proper resume functionality.
Critical Issue
Security: External repository cloned without integrity verification (CWE-494). Add commit hash pinning to prevent potential code injection attacks.
Architecture
The integration is well-designed:
- Clean separation via new
mimo_asr.pymodule (405 lines) - Reuses existing VAD + CAM++ infrastructure
- Resume support with SHA256 validation
- GPU-only with clear fail-fast preflight checks
- Comprehensive mocked test suite (421 lines, CI-safe)
The code quality is high with proper error handling, detailed docstrings, and defensive programming patterns throughout.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Amazon Q Developer flagged 'git clone --depth 1' of the MiMo upstream repo as CWE-494 (download of code without integrity check) on PR #26. Pin to commit 210ef16815b187e05ccc38d627af0d61677afe88 (validated against our MimoAudio kwarg contract and dep list). Override with MIMO_PINNED_COMMIT=<sha> to trial a newer upstream. Clone pattern changed to 'git clone --filter=blob:none --no-checkout' followed by 'fetch --depth 1 origin <sha>' and 'checkout --detach <sha>' — optimal for fetching a single pinned commit without full history. Post-checkout rev-parse HEAD verification catches tampered on-disk state that git checkout itself wouldn't detect. Idempotent: short-circuits if already at the pin. Also documented in CHANGELOG 1.7.0 entry. 269 tests still pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
--lang mimopreset that runs XiaomiMiMo/MiMo-V2.5-ASR locally on a CUDA GPU, reusing the existing FSMN VAD + CAM++ diarization so output format matches--lang zhexactly.Architecture is a VAD + MiMo + CAM++ sandwich — MiMo outputs text per VAD segment, timestamps come from VAD, speaker labels come from CAM++ clustering. Phase 2/3 (merge, speaker map, gender, LLM cleanup) are unchanged.
INSTALL_MIMO=1 bash setup_env.shclones the MiMo repo, installsflash-attn==2.7.4.post1, downloads ~20 GB of weights to$MIMO_WEIGHTS_PATH→$HF_HOME→~/.cache/huggingface.gc.collect()+torch.cuda.empty_cache()between attempts). On final failure, saves*_mimo_partial.jsonwith audio SHA256;--resume-mimopicks up where it left off.--mimo-audio-tag,--mimo-batch,--mimo-weights-path,--resume-mimo.--hotwordsand--batch-sizeare ignored with--lang mimo.Breaking environment change
setup_env.shnow requires Python 3.12. Existing.venv/directories created with earlier Python are detected and rebuilt on the next run. Expect a 2–3 GB re-download of FunASR dependencies.Version bumped 1.6.0 → 1.7.0.
Spec + plan
docs/superpowers/specs/2026-04-29-mimo-asr-integration-design.mddocs/superpowers/plans/2026-04-29-mimo-asr-integration.md(14 TDD tasks)Stats
10 files changed, 1158 insertions, 15 deletions. New module
mimo_asr.py(405 lines) + mocked test suitetest_mimo_asr.py(421 lines, 26 tests, CI-safe — no GPU or weights required).Test Plan
Automated (already passing)
python3 transcribe_funasr.py --helpshows all 4 new flags and `mimo` in `--lang` choicesManual (requires real GPU + real weights, not in CI)
Follow-ups (not in scope)