Skip to content

Ci/merge llm pipelines - #105

Closed
jizhenjun wants to merge 30 commits into
mainfrom
ci/merge-llm-pipelines
Closed

jizhenjun wants to merge 30 commits into
mainfrom
ci/merge-llm-pipelines

Conversation

@jizhenjun

Copy link
Copy Markdown
Collaborator

No description provided.

yuki-328 and others added 24 commits October 3, 2026 20:56
The W3 gates lived in three separate workflow files, and w3-nightly.yml
declared the same 23:23 nightly schedule as llm-deploy.yml, so the
scheduled W3 run was driven from two files at once. The LLM entry points
were split across files for no benefit.

Fold w3-preparation and w3-full-numeric into llm-deploy.yml as jobs of
that pipeline, drop the w3-validation indirection job, and delete the
former w3-preparation.yml, w3-full-numeric.yml and w3-nightly.yml. The
nightly is now declared once. Job-level concurrency groups are preserved
with their distinct groups, and the expensive W3 jobs keep the same
opt-in guards (run_w3_validation on dispatch, W3_NIGHTLY_ENABLED on
schedule) with no continue-on-error, so a failing numeric gate still
fails the run.

Also delete linux-reproduction-docs.yml, which only ever ran
check_linux_repro_docs.py --check-bash over README/CONTRIBUTING and
benchmarks; the unfiltered pytest run in ci.yml still covers
test_linux_repro_docs.py.

Repoint everything that referenced the deleted files:
tests/test_w3_workflows.py now anchors on job ids in llm-deploy.yml and
asserts the folded files are gone, scripts/package_w3_repro.py ships
llm-deploy.yml as the CI recipe instead of .github/workflows/w3-*, and
the W3 docs and the full-model probe README point at the merged
pipeline's jobs.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

🤖 AI Code Review

共审查 10 个变更文件
⚠️ 另有 19 个文件超过上限(最多 10 个)未审查

📁 .github/workflows/ci.yml

🔴 **torch install may not actually be CPU-only** — Line 100-101: `--index-url https://download.pytorch.org/whl/cpu` only applies to the `torch==2.7.1` command. The very next line `pip install -r requirements/qwen3-small-probe.txt "PyYAML==6.0.3"` has no index override, so if that requirements file (or any of its transitive deps, e.g. `transformers`, `sentencepiece`, `tokenizers`) references `torch`, pip may resolve `torch` from PyPI and reinstall the CUDA build, silently breaking the CPU-only intent.
Suggestion: Use `--extra-index-url` on both lines, or add `--no-deps` on the torch install and validate all deps explicitly, or pin a full constraints file applied to every pip call in this step.

🔴 **`pip check` likely fails** — The prior step ran `pip install -e ".[all]"` which typically pulls in `torch` (with CUDA) plus other packages that may pin `torch>=X` or other conflicting versions. Downgrading to `torch==2.7.1+cpu` here will very likely trigger dependency conflicts that `pip check` reports as errors, failing the whole job.
Suggestion: Install torch CPU *before* `.[all]`, or use a single `pip install` call with a constraints file so resolver picks one consistent torch.

🔴 **Brittle version assertion** — Line 103: `assert torch.__version__ == '2.7.1+cpu'`. The `+cpu` local tag varies by wheel build platform and pip may normalize differently (e.g. no suffix on some resolvers, or `+cpu` on linux vs. other tags on Windows/macOS jobs). A future torch bump will break CI for the wrong reason.
Suggestion: `assert 'cpu' in torch.__version__ and torch.version.cuda is None`, or parse with `packaging.version` and compare major.minor.

🟡 **Inconsistent thread pinning** — Lines 184-186 add `OMP_NUM_THREADS / OPENBLAS_NUM_THREADS / MKL_NUM_THREADS: 1` only to one benchmark step. The other test steps (Topic 04, LICM, etc.) earlier in the file run with default multi-threaded BLAS, which produces non-reproducible timings across CI runners and can mask regressions.
Suggestion: Promote these to `env:` on the `job` level so all Python steps share them.

🟡 **Summary language change may confuse external contributors** — Lines 130, 132, 134, 684, 689, 693: swapping English for Chinese in `$GITHUB_STEP_SUMMARY` is fine if the whole org reads Chinese, but public/external PRs will now see mixed-language summaries (the earlier unchanged lines still say "LLVM", "ScratchV", "Register Allocation", "JUnit XML").
Suggestion: Either translate all user-facing summary strings in this file consistently, or keep English in summaries and Chinese only in code comments.

🟡 **Inline package alongside `-r` is easy to misread** — Line 101: `pip install -r requirements/qwen3-small-probe.txt "PyYAML==6.0.3"` mixes a requirements file with an inline specifier. This works but is a common footgun (wording like `-r foo.txt "bar"` gets read as one arg).
Suggestion: Add a one-line comment `# PyYAML pinned here because it's a test-only dep not in qwen3-small-probe.txt`, or move the pin into the requirements file.

💭 **Step name could describe the outcome, not the mechanism** — Line 97: "Install pinned CPU dependencies for Qwen and W3 tests". Fine, but consider "Install CPU torch (2.7.1) + Qwen/W3 test deps" so the pinned version and CPU-ness are visible in the workflow UI without clicking in.

💭 **Assertion inside `python -c` will exit 1 on failure, which is good** — but the message printed will be a bare traceback; consider `assert ..., f"got torch {torch.__version__!r}, cuda={torch.version.cuda!r}"` for a self-describing failure.

📁 .github/workflows/topic06-benchmark.yml

No issues found. The change is a straightforward localization fix — replacing English with Chinese for consistency with the surrounding context (the if branch already outputs Chinese text). No functional impact, no security concern, encoding is fine (GitHub Actions uses UTF-8 by default).

Approve ✅


📁 .gitignore

🟡 Redundant pattern — .venv-linux/ is already covered by .venv-*/. Remove the first line to avoid confusion.

💭 Wildcard scope — .venv-*/ will also match directories like .venv-prod/, .venv-dev/ outside the project. If you only want to ignore venvs under the repo root, this is fine, but be aware it applies at any level.


📁 CONTRIBUTING.md

🟡 Scope mismatch — This bullet covers deployment platform/version requirements, not contribution process. CONTRIBUTING.md should guide how to contribute (PRs, code style, tests), not prescribe infrastructure specifics. Consider moving to docs/CONTRIBUTING-deployment.md or the referenced reproduction doc itself.

🟡 Version hard-coding — Embedding Ubuntu 24.04 / Python 3.12 here means contributors must parse CONTRIBUTING.md to know current versions, and every bump requires editing this file. Prefer linking to the authoritative source and summarizing only the policy (e.g., "Use the platform specified in the reproduction contract for all deployment evidence").

🟡 Discouraging language — "Personal development environments are not acceptance evidence" reads as hostile to the typical OSS contributor flow (local-first dev → CI validates). If the intent is "CI evidence is required for merge," say that directly. Otherwise, rephrase to: "Final acceptance requires CI-verified results; local runs are exploratory only."

💭 Internal jargon — W1 and later is unreadable to new contributors. Spell out the meaning or drop it (the reproduction doc link carries the detail anyway).

💭 Bare link target — No verification that docs/llm-deploy-v1.0/LINUX_REPRODUCTION.md exists in-repo. If it doesn't, the link is broken on arrival.

Suggested minimal rewrite:

- Deployment acceptance evidence must come from CI runs on the platform
  defined in [the Linux reproduction contract](docs/llm-deploy-v1.0/LINUX_REPRODUCTION.md).
  Local results are exploratory; commit identity and platform details must be preserved.

📁 LICENSES/Eigen-MPL-2.0.txt

🟡 建议:确认许可证文件准确性 — 整个文件为新增的 MPL 2.0 完整文本。

  • 建议对照 Mozilla 官方发布的 MPL 2.0 文本 逐字符 diff,确保无 typo/篡改。许可证文件一旦入库会被 SPDX/合规工具依赖,任何偏差都可能引发合规风险。

🟡 建议:确认文件命名规范 — 路径为 LICENSES/Eigen-MPL-2.0.txt。

  • 这是 SPDX 推荐布局(LICENSES/<SPDX-ID>.<ext>),但文件名含产品名 Eigen- 而非纯 SPDX ID。若项目同时被 SPDX 工具扫描,标准文件名应为 LICENSES/MPL-2.0.txt,产品归属信息通常放在 REUSE.toml/REUSE.md 或 .reuse/dep5 中通过 License: Eigen OR MPL-2.0 之类的表达式声明。请确认团队是否有意偏离 SPDX 规范。

🟡 建议:SPDX 头部与 License-Ref 检查 — 373 行文件本身无问题,但请确认:

  • 使用该许可证的源文件是否有对应的 SPDX-License-Identifier: MPL-2.0 头注释(REUSE 标准要求)。
  • 若 Eigen 原始仓库是 MPL-2.0 且本仓库是 fork,还需保留 Copyright 声明与 NOTICE 要求(§3.4 禁止移除 license notices)。

💭 nit:行尾/编码 — 确认文件使用 LF 行尾、UTF-8 无 BOM,且末尾有换行符(POSIX 工具习惯)。从 diff 看内容完整,未观察到 CRLF 混入迹象,建议用 file LICENSES/Eigen-MPL-2.0.txt 复核。


整体评价:内容本身与官方 MPL 2.0 文本一致(抽查了 §2.1、§5.1、§6、§7、§10、Exhibit A/B 等关键条款,措辞正确),无功能性问题。主要风险点在命名/SPDX 合规和完整性校验两个流程性问题上,而非文本内容。


📁 THIRD_PARTY_NOTICES.txt

🔴 Blocker: MPL-2.0 copyleft obligation on derived file — Line 8: fp32_eigen.py is MPL-2.0, which requires that modifications to that specific file be distributed under MPL-2.0. The notice says "derived from" but doesn't clarify whether the file is a verbatim copy or a modification. If modified, MPL-2.0 requires the modified portions be distributed under MPL-2.0, and this needs to be explicit per MPL-2.0 §3. Suggestion: State whether this is a copy or modification, and if modification, confirm the license applies to the changed portions.

🔴 Blocker: Missing SPDX identifiers — The entire document uses prose license names ("MIT", "MPL-2.0") without SPDX IDs. Add SPDX machine-readable identifiers for each license (e.g., SPDX-License-Identifier: MPL-2.0, SPDX-License-Identifier: MIT-0 if that's what MLAS uses). Many compliance tools (ScanCode, FOSSA) require SPDX for automated analysis.

🟡 Inconsistent version pinning — Eigen entry pins to a commit hash (1d8b82b…), but ONNX Runtime pins to a release tag (v1.22.1). These are different reproducibility strategies. A commit hash is stronger; consider pinning ONNX Runtime to its commit hash too for build reproducibility.

🟡 Missing upstream repository URLs — Both entries name the upstream project but provide no source URL. Add:

  • Eigen: https://gitlab.com/libeigen/eigen/
  • ONNX Runtime: https://github.com/microsoft/onnxruntime

Without URLs, a downstream consumer must guess which "Eigen" is meant (there are forks and mirrors).

🟡 No SPDX header on this file itself — Consider adding # SPDX-License-Identifier: CC0-1.0 at the top. NOTICE/THIRD_PARTY files are typically CC0 so they're unencumbered — the file itself shouldn't inherit any copyleft from the licenses it references.

🟡 Verify LICENSES/ directory exists — Line 9 references LICENSES/Eigen-MPL-2.0.txt as a sibling artifact. Add a test or CI check that this file exists and is non-empty. A dangling reference here is a legal compliance failure.

💭 Header could reference package name/version — Line 1 says "ScratchV is distributed under MIT" but doesn't reference which version. If this file ships in a release, tie it to a version so it's clear which artifact this notice applies to.

💭 Consider adding SPDX-License-Identifier: MIT to referenced files inline — Beyond this aggregate notice, the actual source files (fp32_eigen.py, fp32_reference.py) should each carry an SPDX header at the top per common practice. If they don't, add them.


📁 benchmarks/bench_onnx_operators.py

Review: benchmarks/bench_onnx_operators.py

整体变更质量很高,意图清晰、错误信息可读性强。以下为几点评审意见:


🟡 建议:显式检查 .git 的类型,而非仅 .exists() — 第 46 行

.exists() 对文件和目录都返回 True。虽然这在功能上是正确的(.git 可以是文件,用于 submodule/worktree),但当前写法可能让读者困惑——看起来像是只打算匹配目录。建议加一行注释说明,或改用更精确的写法:

git_meta = ROOT / ".git"
if not (git_meta.is_dir() or git_meta.is_file()):

这样语义更清晰,且后续如果 ROOT 被替换成 symlink,is_dir() 会自动 follow。


🟡 建议:错误信息中区分"无 Git 元数据"与"在另一个 checkout 中" — 第 47–48 行

当前错误信息暗示只有"压缩包提取"这一种场景。但实际上如果用户把项目克隆到了非 Git 目录(比如手动拷贝文件),也会触发同样的 RuntimeError,而消息不会指出根本原因。考虑让消息同时覆盖两种情况:

raise RuntimeError(
    f"No .git metadata found at {ROOT}. "
    "This function requires a Git checkout for baseline comparison. "
    "If you extracted a source archive, re-clone with `git clone` instead."
)

💭 Nit:字符串拼接可改为 f-string 统一风格 — 第 48 行

raise RuntimeError("Historical baseline comparison requires a Git checkout at "
                   f"{ROOT}; an extracted source archive has no Git history")

第一行没有插值变量,第二行有。可以用单个 f-string 统一:

raise RuntimeError(f"Historical baseline comparison requires a Git checkout at {ROOT}; "
                   "an extracted source archive has no Git history")

总结: 这个防护逻辑本身是正确的且必要,防止了在非 Git 环境中静默产生错误的基准比较结果。主要改进空间在于错误信息的精确性和 .exists() 的类型检查语义。


📁 benchmarks/test_regalloc/regalloc.md

整体变更将 Windows PowerShell 命令迁移到 Linux/Bash 并补充了环境准备说明,方向正确,可读性提升明显。

🟡 **第 290 行:命令与注释不一致** — 注释说"运行全部 4 项 benchmark",但实际只调用
  bench_regalloc_linear 一个模块。下方"单独运行某项"列的是 bench_simple / bench_dense /
  bench_cnn / bench_pseudo,与 bench_regalloc_linear 不是同一个入口。
  建议明确 bench_regalloc_linear 是否内部聚合了这 4 项,或者把命令改为依次调用 4 个模块。
💭 **第 288 行:`pip install -e ".[all]"`** — 对 benchmark 用户来说 `[all]` 可能拉入无关重依赖。
  如果 benchmark 依赖可独立声明(如 `.[benchmarks]`),建议用更轻量的 extra,加快首次跑通速度。
💭 **第 299 行:`llvm_available=false` 说明** — 建议补一句"安装 `pip install llvmlite` 即可启用",
  让读者知道如何修复,而不是只说"不能作为对比通过"。
💭 **第 288 行:Python 3.12** — 可加一句"3.12+"说明兼容范围,避免 3.11/3.13 用户不确定。

📁 docs/guide/00-环境搭建指南.md

🟡 标题/范围不一致 — 文件名为 00-环境搭建指南.md(通用),但内容已收窄为 Linux-only。新增的说明文字虽然在顶部解释了范围,但读者在目录页看到文件名时仍会期待多平台内容。
Suggestion: 考虑将文件名改为 00-Linux环境搭建指南.md 或在 TOC 入口处标注 (Linux),减少误导。

🟡 硬编码版本可能过期 — 新增段落写死 Ubuntu 24.04。24.04 是 LTS,短期内没问题,但后续维护时容易忘记更新。
Suggestion: 改为 Ubuntu LTS(当前推荐 24.04) 之类的表述,或者加一个"最后验证日期"注释。

💭 内部链接可验证性 — [Linux 固定环境指南](../llm-deploy-v1.0/LINUX_REPRODUCTION.md) 路径假设该文件存在且同级目录下有 llm-deploy-v1.0/ 子目录。如果 CI 或预览环境不会渲染相对链接,读者可能拿到 404。
Suggestion: 确认该文件已在同 PR 或之前提交中入库;如果有文档 CI(如 markdown-link-check),跑一遍验证。

💭 删除 macOS 段落后的信息缺口 — 原 macOS 段落虽然被移除,但如果有 macOS 用户偶然翻到此页,目前只会看到 Ubuntu 命令(sudo apt 在 macOS 上不会生效)。新增说明已覆盖此场景("个人开发环境不作为 Linux 验收依据"),可接受,无需补充。


📁 docs/guide/04-故障排除FAQ.md

💭 Nit: 占位符风格一致性 — 新格式 path/to/resolved-file 更直观,但建议检查全文其他占位符是否统一风格(部分用 <占位符>,部分用 path/to/xxx),避免读者困惑哪些需要替换、哪些是字面量。



⚠️ 未审查的文件

  • docs/llm-deploy-v1.0/LINUX_REPRODUCTION.md
  • docs/llm-deploy-v1.0/W1/IR解释器-使用说明.md
  • docs/llm-deploy-v1.0/W1/IR解释器-开发文档.md
  • docs/llm-deploy-v1.0/W1/IR解释器-设计文档.md
  • docs/llm-deploy-v1.0/W1/README.md
  • docs/llm-deploy-v1.0/W1/W1-执行计划.md
  • docs/llm-deploy-v1.0/W1/W1-报告可靠性修复.md
  • docs/llm-deploy-v1.0/W1/W1-探测方案.md
  • docs/llm-deploy-v1.0/W1/W1-本地收尾与审查报告.md
  • docs/llm-deploy-v1.0/W1/interfaces.md
  • docs/llm-deploy-v1.0/W1/risks.md
  • docs/llm-deploy-v1.0/W1/算子补齐与单测-benchmark.html
  • docs/llm-deploy-v1.0/W1/算子补齐与单测-benchmark.json
  • docs/llm-deploy-v1.0/W1/算子补齐与单测-benchmark.md
  • docs/llm-deploy-v1.0/W1/算子补齐与单测报告.txt
  • docs/llm-deploy-v1.0/W2/README.md
  • docs/llm-deploy-v1.0/W2/W1修复与W2本地验收报告.md
  • docs/llm-deploy-v1.0/W2/W2-分支验收与发布说明.md
  • docs/llm-deploy-v1.0/W3/Linux-CI与Nightly.md

jizhenjun and others added 5 commits October 11, 2026 21:00
The step summaries mixed English prose with bilingual "English / 中文"
strings. Make the prose Chinese across the pipelines while keeping
identifiers and status tokens in English.

Kept in English on purpose: gate and step ids
(numeric:ir-qwen3-small, unit:frontend-ops), metric names, artifact names,
tool names, W1/W2/W3, and every label that starts with a status token
(PASS, FAIL, NOT RUN, CANCELLED, UNCONFIRMED, MISSING REPORT, INVALID
REPORT, PARTIAL/INCOMPLETE (not acceptance), FAIL (step did not succeed)).
GitHub outcome tokens such as success/failure/skipped are echoed
unchanged, so a step's raw result stays greppable and the existing
assertions on it keep working.

Left alone: the shared renderers in probes/numeric_summary.py and
probes/w3_summary.py, which feed both the CI summary and the archived
report.md artifacts and were already Chinese; and the benchmark reports
that ci.yml cats into its summary, which belong to the individual
benchmark scripts.

Tests updated where they pinned the old wording: test_w2_actions_summary
asserted the bilingual strings and the "No ... acceptance result" phrase;
test_w3_actions_summary asserted "No full-model QEMU result", the
"... cases recorded" coverage strings and the NOT RUN tally, which drops
from 5 to 4 because "NOT RUN by this job" is no longer an English token;
test_w3_workflows asserted the two W3 acceptance and execution-scope
sentences.

Co-Authored-By: Claude Code <noreply@anthropic.com>
"自动门槛", "必过验收" and "W2 前端、后端与宿主运行时" were newly
invented labels with no prior use in this repository, so they cannot be
looked up or checked by a reader. Replace them with wording the repo
already owns:

- "W3 automated gates"    -> "W3 验收"        (job name "W3 / required
                                                acceptance"; "CI 验收",
                                                docs/llm-deploy-v1.0/开发计划.md:148)
- "W2 required acceptance" -> "W2 验收"        (job name "W2 / required
                                                acceptance")
- "W2 frontend, backend and host runtime"
                          -> "W2 单元门槛"     (step name "Summarize W2
                                                unit gates"; "单元测试",
                                                开发计划.md:154; "硬门槛",
                                                开发计划.md:160)

The remaining translated strings reuse existing repository vocabulary
(批准, 签署, 寄存器分配, 不变量, 流水线耗时, 门槛, 验收, 用例, 覆盖).

Co-Authored-By: Claude Code <noreply@anthropic.com>
Give every summary section a plain "W<N> 测试项" heading instead of a
bespoke descriptive title, so the section name is uniform and directly
identifies the week under test:

  W2 unit gates        -> "## W2 测试项"
  W1 交付物状态        -> "## W1 测试项"
  probe:qwen3-onnx     -> "## W1 测试项"   (job "W1 / full Qwen3 ...")
  frontend:parse-qwen3 -> "## W2 测试项"   (job "W2 / complete Qwen3 frontend")
  w3-acceptance        -> "## W3 测试项"
  w3-execution-summary -> "## W3 测试项"
  check_w2_ci_results  -> "## W2 测试项"
  summarize_w3_ci.py   -> "## W3 测试项"

The prepare/full distinction that the W3 titles carried is already in
the body text, and the summary reference now points at the job ids
"w3-preparation" and "w3-full-numeric" instead of the old titles.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Four Chinese labels were translations of English terms that have no prior
use in this repository, so a reader has nothing to check them against.
Restore the original wording:

  "探针"            -> "Probe" / "probe step did not complete"
  "完整七用例"      -> "Complete seven-case"
  "记录覆盖"        -> "Recorded coverage"
  "概要预算"        -> "summary budget"

Terms the repository already owns stay in Chinese: 门槛 (20 files),
验收 (45), 回归测试 (20), 流水线耗时 (1). "测试项" is the user's own
wording for the section headings and is unchanged.

Co-Authored-By: Claude Code <noreply@anthropic.com>
dfefb00 translated "host IR" into "宿主 IR" in the W3 summary table.
probes/w2_runtime/README.md already titles this axis "W2 Host 运行时验收",
so keep the English term here too.

Co-Authored-By: Claude Code <noreply@anthropic.com>
The W2 Actions page dumped every probe's report.md plus per-step outcome
lines from four steps across three jobs, which answered none of the
questions the W2 gates exist for.

Add scripts/summarize_w2_ci.py, which renders exactly three tables from
the six output/<dir>/report.json reports: which operators each layer
supports (marking only the ones this run actually exercised), the worst
max_abs / relative_l2 / cosine against ORT and the independent NumPy
references, and the per-chain timing plus a total.

The w2-acceptance job downloads the four W2 artifacts and becomes the
single writer of the W2 section. Every other W2 summarize step now
writes to the step log instead, as does scripts/check_w2_ci_results.py,
whose exit code remains the acceptance gate.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@jizhenjun jizhenjun closed this Oct 13, 2026
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