Skip to content

fix(onnx): fetch pinned baseline when CI clone is shallow - #97

Merged
FeelTheBeats merged 1 commit into
ScratchV-Compiler:mainfrom
FeelTheBeats:seven_ci_fix
Oct 5, 2026
Merged

FeelTheBeats merged 1 commit into
ScratchV-Compiler:mainfrom
FeelTheBeats:seven_ci_fix

Conversation

@FeelTheBeats

Copy link
Copy Markdown
Contributor

The self-hosted CI sync does a depth-1 fetch, so the pinned baseline commit (20b105e) is absent and git rev-parse fails with rc=128. Resolve the baseline on demand via git fetch and cover it with a regression test.

The self-hosted CI sync does a depth-1 fetch, so the pinned baseline
commit (20b105e) is absent and git rev-parse fails with rc=128. Resolve
the baseline on demand via git fetch and cover it with a regression test.
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

🤖 AI Code Review

共审查 2 个变更文件

📁 benchmarks/bench_onnx_operators.py

Review: benchmarks/bench_onnx_operators.py


🟡 git 参数注入风险 — resolve_commit 中 git fetch 未使用 --end-of-options,而 rev-parse 已正确使用了:

# 当前
git("fetch", "--depth=1", "origin", ref)

# 建议
git("fetch", "--depth=1", "--end-of-options", "origin", ref)

若 ref 以 - 开头(例如 --tags),会被 git fetch 解析为参数而非 refspec,可能改变行为。虽然实际传入的是 baseline_ref(commit hash/tag),但防御性处理一致更好——rev-parse 已经做了,fetch 也应该做。


💭 fetch 后再次失败时无诊断上下文 — 如果 fetch 成功但第二次 rev-parse 仍然失败(例如 ref 在远端也不存在),抛出的异常不会提示"已尝试 fetch"。考虑加一条 stderr 日志或用 logger.warning 标注 fetch 阶段,方便 CI 排障时区分"本地缺失"和"远端也缺失"。


💭 git fetch 失败时的静默传播 — 如果 fetch 因网络问题失败,异常直接向上传播,用户看到的是 raw CalledProcessError。对内部 benchmark 脚本可接受,但如果此模块被外部调用,值得考虑包装一层带上下文的异常。


整体评价:逻辑正确,docstring 清晰,"先试后拉再重试"的三层 fallback 设计合理。主要问题是 fetch 调用缺少 --end-of-options,与同函数中 rev-parse 的防御风格不一致,建议统一。


📁 tests/test_onnx_operator_benchmark.py

🟡 Fragile mock state — The any(call[0] == "fetch" for call in calls) check conflates call history with state. If the logic ever grows (e.g., a fetch handler that itself calls rev-parse), this becomes a footgun. A simple boolean flag would be clearer:

def test_missing_baseline_commit_is_fetched_from_origin(monkeypatch):
    calls = []
    fetched = False

    def fake_git(*arguments, binary=False):
        nonlocal fetched
        calls.append(arguments)
        if arguments[0] == "fetch":
            fetched = True
        if arguments[0] == "rev-parse" and not fetched:
            raise subprocess.CalledProcessError(128, ["git", *arguments])
        return bench.BASELINE if arguments[0] == "rev-parse" else ""

🟡 No call-count assertion — The test passes if resolve_commit fetches once and retries, or skips the first failed rev-parse entirely. Add a check that rev-parse was called exactly twice (once failing, once succeeding) to lock down the expected flow:

assert sum(1 for c in calls if c[0] == "rev-parse") == 2

💭 binary kwarg coupling — The mock assumes bench.git accepts binary as keyword-only. If the real function signature changes, this mock silently mis-routes calls. Consider def fake_git(*args, **kwargs) to be resilient to signature drift.


@FeelTheBeats
FeelTheBeats merged commit 91286f6 into ScratchV-Compiler:main Oct 5, 2026
5 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