修复 Sherpa 本地 ASR 资产校验与部分下载状态#527
Conversation
PR Reviewer Guide 🔍(Review updated until commit c9aa462)Here are some key observations to aid the review process:
|
6cc6919 to
4215437
Compare
appergb
left a comment
There was a problem hiding this comment.
自动化代码审核 — PR #527
由 Agent 自动审核,最终合并决定由维护者把关。
| 严重度 | 数量 | 结论 |
|---|---|---|
| CRITICAL | 0 | pass |
| HIGH | 2 | 建议合并前修复 |
| MEDIUM | 2 | info |
| LOW | 1 | note |
[HIGH] hasSherpaPartial 未跟进新的 selectedSherpaPartialBytes — PR 自己想修的 bug 仍部分残留
位置:openless-all/app/src/pages/LocalAsr.tsx,约第 1242–1245 行。
PR 新增了 selectedSherpaPartialBytes = Math.max(selectedSherpaProgressBytes, selectedSherpaDownloadedBytes) 作为"是否存在部分下载"的统一信号,下方 showSherpaDownloadProgress 也按它判断了。但 hasSherpaPartial 仍然只读 selectedSherpaDownloadProgress?.bytesDownloaded,没有回退到 catalog 的 selectedSherpaDownloadedBytes:
const hasSherpaPartial =
(selectedSherpaDownloadProgress?.bytesDownloaded ?? 0) > 0;后果:刷新页面后即使 downloadedBytes > 0,hasSherpaPartial 仍是 false — 这正是 PR 描述里要修的"页面刷新后部分下载状态丢失"。同时 hasSherpaPartial 还会经过 canDeleteSelectedSherpa 影响删除按钮的可用性,两个分支不一致。
修复:
const hasSherpaPartial = selectedSherpaPartialBytes > 0;[HIGH] selectedSherpaCatalogHasDir 用 fileSizeMb !== null 当作"有本地文件"的代理 — 删除按钮永远可用
位置:LocalAsr.tsx,约第 1253 行。
const selectedSherpaCatalogHasDir =
typeof selectedSherpaCatalog?.fileSizeMb === "number";fileSizeMb 是 catalog 里每个 model 都有的静态字段(mock catalog 里四个条目都硬编码了值)。这导致 selectedSherpaCatalogHasDir 对任何已知 model 都是 true,进而 selectedSherpaHasLocalFiles 永远 true,canDeleteSelectedSherpa 永远 true — 哪怕刚加载、本地什么都没下载,删除按钮也是亮的。
修复:从 selectedSherpaHasLocalFiles 里移除 selectedSherpaCatalogHasDir,guard 用 cached === true || downloadedBytes > 0 即可。
[MEDIUM] required_dir_is_valid 的 _ 分支静默返回 false — 新增 alias 时容易踩坑
位置:openless-all/app/src-tauri/src/asr/local/sherpa.rs,required_dir_is_valid。
_ 分支返回 false,如果将来加新 model alias 使用目录资产却忘了加分支,所有文件都会被认为"缺失",且没有任何编译/运行期警告。required_path_is_dir 同样依赖隐式的 matches! 列表,两个 match 必须手工保持同步,耦合脆弱。
建议:_ 分支加 debug_assert!(false, "…") 或 unreachable!(),或把 dir-layout 校验与 is-dir 判断合并到一处。
[MEDIUM] 归档型 model(qwen3)的 downloaded_bytes 在解压后可能双重计算
位置:openless-all/app/src-tauri/src/asr/local/sherpa_download.rs,downloaded_release_archive_bytes(约 257–274 行)。
解压成功后 .tar.bz2 可能仍与解压目录共存。函数先 meta.len() 看归档文件 — 如果归档未被删除而解压目录也存在,返回的是压缩归档大小而非实际磁盘占用(两者不同),刷新后进度条会失真。
不是本 PR 引入,但本 PR 让它更显眼。
[LOW] 合成 progress 对象的 file: "" / fileIndex: 0
hasSherpaPartial 为 true 但没有正在跑的下载时,代码合成了一个 file: "", fileIndex: 0 的 progress 给 DownloadProgressBlock。如果该组件按 "Downloading file 1 of N" 渲染,会显示空文件名。需要确认 DownloadProgressBlock 优雅处理空文件名。
结论:WARNING — HIGH#1 让 PR 自身宣称要修的 bug 仍部分残留,HIGH#2 让删除按钮永远可用,都建议合并前修复。
🤖 Generated by code-reviewer agent.
|
Persistent review updated to latest commit 4215437 |
4215437 to
c9aa462
Compare
|
Persistent review updated to latest commit c9aa462 |
appergb
left a comment
There was a problem hiding this comment.
自动化复审 — PR #527(commit c9aa462 之后)
Follow-up review after author addressed my earlier review.
| 上次 HIGH 问题 | 当前状态 |
|---|---|
HIGH #1: hasSherpaPartial 未跟进 selectedSherpaPartialBytes |
✅ 已修复 |
HIGH #2: selectedSherpaCatalogHasDir 用 fileSizeMb 作代理 |
✅ 已修复 |
| 新增 | ❌ HIGH(新):downloadedBytes 读自错误的 state slice |
✅ HIGH #1 已修复
引入了 selectedSherpaPartialBytes = Math.max(selectedSherpaProgressBytes, selectedSherpaDownloadedBytes),hasSherpaPartial 现在读 selectedSherpaPartialBytes > 0。正确。
✅ HIGH #2 已修复
旧的 selectedSherpaCatalogHasDir = typeof selectedSherpaCatalog?.fileSizeMb === "number" 已移除。canDeleteSelectedSherpa 改为 selectedSherpaHasLocalFiles || hasSherpaPartial,selectedSherpaHasLocalFiles 为 cached === true || selectedSherpaDownloadedBytes > 0,两条路径都有实际含义。
❌ 新增 HIGH — downloadedBytes 读自错误的 state slice
位置:openless-all/app/src/pages/LocalAsr.tsx,约第 1243 行:
const selectedSherpaDownloadedBytes =
selectedSherpaCatalog?.downloadedBytes ?? 0 // BUGselectedSherpaCatalog 的类型是 SherpaOnnxCatalogModel | undefined。看 localAsr.ts:399 的类型定义,SherpaOnnxCatalogModel 只有 alias / displayName / cached / fileSizeMb,根本没有 downloadedBytes 字段。这个字段属于 LocalAsrModelStatus,挂在 models state 上,不在 sherpaCatalog 上。
后果:selectedSherpaCatalog?.downloadedBytes 在运行时永远是 undefined,selectedSherpaDownloadedBytes 永远是 0。这静默破坏了几条逻辑:
selectedSherpaHasLocalFiles里downloadedBytes > 0那条分支死掉了 — 只有cached翻转后才能删 partial 状态的 model。selectedSherpaDownloadProgressForDisplay里 fallback progress 块永远显示bytesDownloaded: 0,取消下载时进度条会归零而不是停在已下载位置。- 取消下载的 optimistic update(约 1093–1096 行)写的是
modelsstate 上的downloadedBytes,但 fix 读的是sherpaCatalog,两边永远不同步。
TS 不会报错 — 因为对没有该字段的类型用 ?. 是合法访问,编译期静默给 undefined。
修复:从 models 取:
const selectedSherpaModel = models.find(m => m.id === selectedSherpaAlias);
const selectedSherpaDownloadedBytes = selectedSherpaModel?.downloadedBytes ?? 0;或者把 downloadedBytes 真正加到 SherpaOnnxCatalogModel 类型 + catalog 数据源上(如果设计意图就是 catalog 该带这个字段)。
LOW — Prepare 按钮 guard 简化
Prepare 按钮 guard 从 sherpaBusy !== null || !sherpaAvailable || cached !== true 简化为 sherpaBusy !== null || !sherpaAvailable,允许对已 cached model 重新 prepare。是个合理的 UX 改动,但请作者确认这就是想要的行为。
结论:WARNING — 两个原 HIGH 都修了,但 fix 引入了一个新 HIGH(state slice 读错)。建议合并前修正 selectedSherpaDownloadedBytes 的取值来源。
🤖 Generated by code-reviewer agent.
当前状态
export interface SherpaOnnxCatalogModel {
alias: SherpaOnnxModelAlias
displayName: string
cached: boolean
downloadedBytes: number
fileSizeMb: number | null
} |
撤回上一条新增 HIGH —— 我看错了@weikeyi 抱歉,你是对的,我那条新增 HIGH 是误报,撤回。 确认你说的情况:当前 PR branch(commit export interface SherpaOnnxCatalogModel {
alias: SherpaOnnxModelAlias
displayName: string
cached: boolean
downloadedBytes: number
fileSizeMb: number | null
}我的 reviewer agent 拿的是本地 clone 的旧版(base 分支)类型,没把这次 PR 同时改了类型那部分纳入。 当前两条原 HIGH 都已修,新增 HIGH 是误报,从我这边看 PR #527 没有阻塞合并的问题了。 LOW 项( 🤖 Generated by code-reviewer agent. |
User description
概要
修复 Sherpa 本地 ASR 在 Qwen3-ASR 模型资产校验和部分下载状态展示上的问题。
主要改动:
tokenizer.jsonvocab.json+merges.txtdownloadedBytes,用于前端恢复本地部分下载状态。downloadedBytes、fileSizeMb和实时下载 progress 综合判断本地文件状态。cached === true,允许在本地文件存在但缓存状态未完整识别时尝试准备。背景
Qwen3-ASR 的 tokenizer 资产可能不是单一的
tokenizer.json文件,而是vocab.json+merges.txt的组合布局。此前校验逻辑无法识别这种合法布局,可能导致模型已经下载/解压后仍被判定为未缓存或不可用。另外,Sherpa 下载状态主要依赖前端实时 progress。页面刷新后,如果本地已经存在部分下载或部分解压文件,但没有实时下载事件,UI 可能无法正确显示继续下载、删除和进度状态。
Foundry 检查结论
本次同时检查了 Foundry Local Whisper 的类似风险。
Foundry 当前模型资产仍是固定文件布局,没有发现类似 Qwen3 tokenizer 这种目录型、多布局资产校验需求。因此本 PR 不修改 Foundry 的后端模型校验逻辑。
验证
node node_modules\typescript\bin\tsc --noEmit --pretty falsecargo test qwen3_tokenizer_accepts_supported_layouts --libgit diff --checkPR Type
Bug fix, Tests
Description
Support dual Qwen3 tokenizer layouts
Restore Sherpa partial download state
Enable prepare without cached flag
Add regression coverage for tokenizer assets
Diagram Walkthrough
File Walkthrough
sherpa.rs
Accept supported Qwen3 tokenizer layoutsopenless-all/app/src-tauri/src/asr/local/sherpa.rs
tokenizer.jsonorvocab.jsonplusmerges.txt.required_path_is_dirlogic aligned with the tokenizer directoryrequirement.
intermediate state.
LocalAsr.tsx
Restore Sherpa download state from catalogopenless-all/app/src/pages/LocalAsr.tsx
downloadedByteswhen no liveprogress exists.
active progress.
cachedis unset.after refresh.