Skip to content

fix(w1): 修复两层探测报告与源码归属,兼容固定 ORT 验证 - #93

Merged
jizhenjun merged 1 commit into
ScratchV-Compiler:mainfrom
yuki-328:codex/w1-report-integrity
Oct 4, 2026
Merged

jizhenjun merged 1 commit into
ScratchV-Compiler:mainfrom
yuki-328:codex/w1-report-integrity

Conversation

@yuki-328

@yuki-328 yuki-328 commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

PR #91 合并后,补齐两层探测的证据可靠性:报告写入/关闭/原子替换失败时必须失败,不能留下成功 JSON;源码副本不能借用父目录 Git 身份,QEMU 报告分别记录当前执行环境与原导出环境。

本 PR 仅包含 7 个 W1 文件;W2 完整解析、运行时、后端及 CI 扩展另行提交,修改文件不重叠。基于已合并主干 3bb88e8。

修改

  • Markdown/HTML 先保存、JSON 最后原子发布;保留原始数值/执行错误并增加失败注入测试。
  • 无本目录 .git 时明确未知 Git 身份;扩展源码指纹及当前环境归属检查。
  • 固定 ORT 1.22.1 无 DOUBLE Cos CPU 内核,且 omitted-axes ReduceMean + noop 路径有限制。这三个用例显式使用 ONNX 官方 ReferenceEvaluator,仍执行生产 parser/IR;不跳过用例、不放宽数值门槛。
  • 增加 W1 修复说明和复现命令。

验证

  • 本独立分支:262 passed / 0 skipped。
  • 重新导出真实 Qwen3 结构两层小模型:7 组输入、29 个检查点和不变性通过。
  • 普通/诊断图 × none/all × 7 输入,28/28 次真实 RV64 QEMU 通过,目标最大绝对误差 1.6689300537109375e-6 < 1e-5。
  • 文档链接和 git diff --check 通过。源码指纹与提交前执行代码一致。
  • 验证在 Windows 固定 CPU 环境执行;QEMU 进程墙钟累计 154.702 秒。远端 Linux CI 待本 PR 实际运行,未声称已通过。项目约定的本地 L2 harness 文件缺失,以上实际测试替代其不可用入口。

关联 #92;不自动关闭接口冻结、完整模型第二人复现或 W1 出口评审待办,也不改变计算算法、模型配置和精度门槛。

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

🤖 AI Code Review

共审查 5 个变更文件

📁 docs/llm-deploy-v1.0/W1/W1-报告可靠性修复.md

问题列表

🔴 Bug: 证据路径与复现命令目录名不一致 — 复现命令输出到 output/w1-ir-new / output/w1-qemu-new,但证据部分引用的是 output/w1-publish-ir/ / output/w1-publish-qemu/。读者无法判断哪套是权威产物。建议统一命名或显式说明二者关系。

🟡 Suggestion: 测试断言范围模糊 — "262 passed / 0 skipped" 未标注执行的具体 commit hash 或时间戳。若后续 rebase 导致测试数量变化,此文档无法验证是否仍准确。建议在括号内标注 commit(已有基线 commit,可复用)。

🟡 Suggestion: 缺少回滚/失效条件 — 文档描述了验收条件但未说明在何种情况下该报告应被视为过期(例如上游 ONNX Runtime 版本变更、QEMU 行为回归)。考虑添加"本结论在以下条件变化时失效"。

💭 Nit: 链接健壮性 — Issue #92 使用绝对 URL 指向 GitHub。若仓库迁移或仓库更名(当前组织名 ScratchV-Compiler),链接将失效。可考虑同时标注 issue 标题作为后备引用。

💭 Nit: 命令未设 set -e — 三条 python 命令独立排列,任一失败不阻止后续执行。作为复现指南,可加注释说明"若前一步失败则停止",或用 && 串联。


📁 probes/w2_qwen3_small/run.py

🔴 Bug: fail() corrupts original test result — report["passed"] is set to False even when the actual probe passed. If the caller uses report["passed"] to gate deployments, a transient report-write failure will falsely block. Use a separate field instead:

def fail():
    # Don't mutate the test's semantic result; use a distinct field.
    report["report_success"] = False
    report["report_stage"] = "report-write"
    report["report_error"] = "Incomplete two-layer IR gate evidence"
    report["report_write_errors"] = list(failures)

🟡 Retry return value ignored (line ~395) — write("report.json", ...) on retry discards its return value. If the retry fails, failures gets the new entry but no terminal action is taken beyond the stderr print. Consider making this explicit:

if not write("report.json", json.dumps(report, indent=2, allow_nan=False) + "\n"):
    fail()
    recover_views()
    if not write("report.json", json.dumps(report, indent=2, allow_nan=False) + "\n"):
        report["report_write_errors"] = list(failures)  # already implicit, but be explicit

🟡 Double fail() call when both md/html and JSON writes fail — fail() sets report_write_errors then recover_views() overwrites it. The second fail() call (for the JSON failure) repeats this cycle. Net result is correct but fragile. Consider guarding:

def fail():
    if "report_error" not in report:  # already failed once
        return
    ...

💭 Exception broadening is correct here — subprocess.SubprocessError now also catches TimeoutExpired (subclass), which is needed since timeout=10 was added. Good.

💭 .git check is sound — handles the nested-checkout edge case well; .exists() covers both directory and file (submodule) cases.


📁 tests/test_onnx_qwen_numeric_ops.py

🔴 Bug: ReferenceEvaluator API 调用方式不正确 — 第 33-34 行:

ReferenceEvaluator 的构造函数接受 model_path 参数,但其 run() 方法的签名与 ort.InferenceSession.run() 不兼容。ReferenceEvaluator.run(output_names, feed_dict) 的第一个参数是 output_names,第二个是 feed_dict。当前代码 session.run(None, feed) 对 ReferenceEvaluator 可能不兼容——None 作为 output_names 可能抛出异常,取决于 onnx 版本。

建议:确认 ReferenceEvaluator.run() 支持 output_names=None(取所有输出)。如果不支持,需要显式传入输出名,例如:

ref_outputs = ["y"] if reference_evaluator else None
reference = session.run(ref_outputs, feed)[0]

🟡 缺少对 ReferenceEvaluator 的单元测试验证 — 没有单独测试 reference_evaluator=True 分支是否能正确工作。如果 onnx 版本升级导致 ReferenceEvaluator API 变更,此测试会静默失败或报出与目标测试无关的错误。建议在 CI 中加一个最小的 smoke test。

🟡 注释过长且耦合实现细节 — 第 86-89 行:注释解释了 ORT 1.22.1 不支持 DOUBLE Cos 的 CPU kernel,但将这一环境/依赖限制固化在测试逻辑中。未来 ORT 升级支持后,此代码路径永远无法被触发但也不会被清理。建议:

  • 注释中加上 issue/追踪链接或预期解决条件
  • 考虑加 # pragma: no cover 或 TODO 标记便于后续清理

💭 Nit: compare() 返回值未显式标注类型 — 函数没有返回类型注解,而 session 现在是联合类型 (ReferenceEvaluator | InferenceSession),session.run() 的返回类型在两个实现间不同。IDE 和类型检查器会报 Any,建议加 # type: ignore[union-attr] 或封装为独立函数。

💭 Nit: 条件表达式可读性 — 第 33-34 行:

session = (ReferenceEvaluator(str(path)) if reference_evaluator else
           ort.InferenceSession(str(path), providers=["CPUExecutionProvider"]))

括号内换行风格不一致(左侧无括号分组)。建议提取为辅助函数或至少统一缩进风格。


📁 tests/test_onnx_shape_ops.py

🟡 DRY: duplicated condition — The opset == 18 and axes is None and noop == 1 check appears twice in test_reduce_mean. Extract to a local use_ref_eval = ... at the top of the workaround branch (or right before _compare); a future edit to one copy will drift from the other.

🟡 ReferenceEvaluator API assumption — ReferenceEvaluator.run(None, feed) relies on the output_names=None overload returning outputs in graph order, which is what ORT's InferenceSession.run supports and what this call mirrors. Worth verifying against the pinned onnx version (onnx.reference.ReferenceEvaluator was added in 1.12 and has historically had signature drift, e.g. requiring explicit output names in some releases). If your CI matrix spans multiple onnx versions, a tiny _run_reference(path, feed) helper that asserts the output count would make the failure mode clearer than a bare TypeError here.

🟡 ReferenceEvaluator vs ORT semantics drift — Even inside the workaround, you're now comparing your IR against a different reference implementation than every other parametrization of the test. The ONNX spec is the oracle, but ReferenceEvaluator can itself have bugs (it's pure-Python and often lags ORT kernels). Since the workaround is intentionally narrow (one opset + one attr combination), consider adding a follow-up comment/tagging the case so it's easy to re-baseline against ORT once the upstream bug is fixed, e.g. # TODO(upstream-onnxruntime#XXXX): drop reference_evaluator.

💭 Comment placement — The ORT bug rationale is currently above the actual = _compare(...) line, so it reads as documenting the assignment rather than the whole workaround block. Consider moving the two-line comment to just above the if opset == 18 ... line so it scopes the entire special case, not just the captured variable.

💭 Type hint / keyword-only consistency — _compare gained a keyword-only param (*, reference_evaluator=False). Good call. If there's any other wrapper in the file using positional args against _compare, a quick grep will confirm nothing broke.

Nothing blocking — the workaround is a reasonable response to a documented upstream kernel bug, and scoping it to one specific (opset, axes, noop) tuple is the right amount of precision.


📁 tests/test_qwen3_small_riscv_gate.py

Code Review: tests/test_qwen3_small_riscv_gate.py

🟡 全局 monkeypatch Path 类方法风险 — 第二、三、四个测试将 Path.write_text 和 Path.replace 作为类级别全局替换(如 monkeypatch.setattr(Path, "write_text", write))。如果同一测试期间有其他代码路径触发了 Path 操作,也会被拦截。建议用 monkeypatch.setattr 替换被测模块内对 write_text 的调用点,而非全局替换类方法。

🟡 probe.np.__version__ 属性猴补脆弱性 — 若 numpy 的 __version__ 是 property 或动态计算值,setattr 可能静默失效或引发 AttributeError。建议显式验证赋值成功(如 assert),或使用 mock 替代。

🟡 参数化组合过度 — test_report_io_failure_never_publishes_pass_and_preserves_execution_error 的 3×2×2=12 种组合中,write_before_error 维度主要用于覆盖"写入部分内容后再失败"的场景。如果底层实现保证写后原子 rename(即无部分可见文件),此维度可能不产生实质差异。建议评估是否可合并为 6 个 case。

💭 capsys.readouterr() 调用位置 — 该调用在 test_report_io_failure... 测试的断言链末尾,如果后续断言失败,captured stderr 不会被打印到 traceback 中。建议提前调用并缓存结果:err_output = capsys.readouterr().err。


@jizhenjun
jizhenjun merged commit faab6a8 into ScratchV-Compiler:main Oct 4, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants