You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
🔴 Bug: Missing path filter — on: push / pull_request has no paths filter. This benchmark workflow runs on every commit/PR in the repo, wasting CI minutes on unrelated changes.
Suggestion: Add paths: [ '**topic06**', '**Topic06**', 'benchmark_reports/**', 'scripts/run_topic06_benchmarks.py', '.github/workflows/topic06-benchmark.yml' ] or equivalent.
🔴 Bug: Cascading failure hides results — The "Run supported correctness gates" step runs three sequential commands; if the first (activation) fails, the elementwise and loop categories are never executed, and the diagnostic report step is also skipped. A single flaky category blocks all diagnostics.
Suggestion: Use continue-on-error: true on the correctness gates step, or run each category as a separate job, so you get the full diagnostic picture even when a category fails.
🟡 Magic number: --benchmark 3 — Line 36: the meaning of 3 is opaque. Future readers will have to trace into the script to understand what it controls.
Suggestion: Add a comment (e.g., # 3 = full sweep including edge cases) or replace with a named flag.
🟡 Missing pip cache — No actions/cache or built-in pip cache (cache: 'pip' on setup-python). Benchmark dependency installs will re-download wheels on every run.
Suggestion: Add cache: 'pip' to the setup-python step, or add a cache step for ~/.cache/pip.
🟡 Missing concurrency — Rapid successive pushes will spawn parallel benchmark runs, doubling CI load and potentially producing conflicting reports.
Suggestion: Add concurrency: group: topic06-benchmark-${{ github.ref }}, cancel-in-progress: true.
🟡 Upload may reference non-existent directories — benchmark_reports/topic06/cases/ and benchmark_reports/topic06/failures/ may not exist if no failures are generated. upload-artifact@v4 can emit warnings or produce empty archives.
Suggestion: Ensure the script creates these directories (even if empty) before the upload step, or remove the paths that aren't guaranteed.
💭 Nit: --fail-on-test-failure semantics unclear — The flag name suggests it fails the job on test failure, but since you want diagnostics regardless, consider whether this flag is what you actually want or if --strict would be clearer.
💭 Nit: Python version pinning — "3.11" is pinned but not documented with a reason. A brief comment explaining why 3.11 is required (vs 3.10 or 3.12) helps future maintainers.
📁 Makefile
🟡 **Magic number: `--benchmark 3`** — `bench-topic06` hardcodes this value with no explanation of what "3" means or why it's sufficient to cover both "DSL correctness + TinyFive" as the comment claims.
Suggestion: Add a brief comment explaining what benchmark 3 is, or accept it as a variable (e.g. `BENCHMARK ?= 3`).
🟡 **No dependency on prerequisites** — `bench-topic06` has no prerequisite to ensure `scripts/run_topic06_benchmarks.py` exists or that required packages are installed. If the script is missing, the failure message (`python3: can't open file ...`) is less clear than a Makefile-level check.
Suggestion: Consider a lightweight guard (e.g. `@test -f scripts/run_topic06_benchmarks.py || (echo "Missing: ..." && exit 1)`), or document that the user must run `make install` first.
💭 **No output directory creation** — Unlike `bench:` which writes to `benchmark_reports/`, there's no indication where topic06 benchmarks land. If the script creates files, the target doesn't ensure the directory exists.
Suggestion: If the script writes reports, add an `@mkdir -p <dir>` prerequisite or let the script handle it and document that.
💭 **Hardcoded `python3`** — Other targets in the file may use a different interpreter (virtualenv, conda, etc.). Consistency check: does the rest of the Makefile assume `python3` or a specific environment?
Suggestion: If the project uses a virtualenv, consider `$(PYTHON)` with a default `PYTHON ?= python3`.
📁 README.md
🔴 Data accuracy: test count dropped — Line 65: tests/ originally stated "348 个单元测试". The new text "单元测试及课题 06 的 23 个 DSL 用例" drops the 348 count entirely. If those 348 unit tests still exist, the number was lost. Either preserve it (348 个单元测试 + 23 个 DSL 用例) or add a note explaining the count changed.
🔴 DSL case count ambiguity — Line 64: The original had "23 个 DSL 基准用例" under benchmarks/. The new text moves "23 个 DSL 用例" to tests/ while benchmarks/ now says "通用基准与课题 06 性能基线". Are the 23 cases physically moved, or do both dirs still reference them? If moved, the benchmarks description is vague. If duplicated, both descriptions should clarify the relationship.
🟡 README coupled to a single topic — Lines 64-66 now name-drop "课题 06" in three consecutive directory descriptions. If Topic 06 is rotated out or renamed, the main README breaks. Consider keeping directory descriptions generic and putting Topic-06 specifics in the linked doc (06-性能测试套件使用说明.md) only.
🟡 Link path may not exist — Line 108: [课题 06 测试套件](docs/topics/06-性能测试套件使用说明.md) — verify this file is actually added in the same PR. If it's a dangling link, the nav table is broken on merge.
🔴 Hardcoded absolute Windows paths — compile_command leaks D:\anaconda3\python.exe and D:\PycharmProjects\ScratchV\.... These are non-portable and expose dev-machine paths in a committed artifact. Other developers or CI on Linux/macOS cannot reproduce the command.
Suggestion: Store relative or environment-templated commands (e.g., ${PYTHON} + repo-relative paths), or strip the command to a canonicalized form before committing.
🟡 Missing trailing newline — Diff explicitly flags \ No newline at end of file. Violates POSIX convention and causes noisy diffs on future edits.
Suggestion: Add \n at EOF (most linters catch this — ensure your JSON formatter does too).
🟡 Inconsistent numeric types between perf_counters and cost_model — perf_counters uses integers ("add": 4), cost_model uses floats ("dynamic_add": 4.0). Same values, two types. If this is generated from different code paths, downstream consumers doing === comparison will silently fail.
Suggestion: Unify to one type across both objects.
🟡 output_dtype: null contradicts expected_type: "scalar" — The field explicitly says scalar, so dtype should be determinable (e.g., int32, float64). null makes the field useless for schema validation or cross-case analysis.
Suggestion: Populate output_dtype from the DSL type inference; if genuinely unknown, add a reason field.
🟡 ci95_instr_count: 0.0 with benchmark_runs: 3 — Zero variance is plausible for a deterministic scalar benchmark, but 3 runs is the minimum to compute any meaningful CI. If this is expected, document it; otherwise consider bumping the run count for non-deterministic cases.
💭 total_time_sec (0.829) doesn't decompose into visible sub-components — compile_time_sec (0.085) + simulation_time_sec (0.147) + interpreter_time_sec (0.129) ≈ 0.361, leaving ~0.47s unaccounted (presumably the 3 benchmark runs). Adding a benchmark_time_sec field would make the report self-documenting.
🔴 Missing newline at EOF — Final line lacks trailing newline. POSIX convention and most tools expect one; will cause spurious diffs on every subsequent edit.
Suggestion: Ensure the file ends with \n.
🟡 Hardcoded Windows paths in compile_command — Backslash paths (D:\\anaconda3\\...) make this file non-portable and unparseable on Linux/macOS CI. If this file is checked in, any path change invalidates it.
Suggestion: Store paths relative to project root, or omit the absolute command from the report artifact.
🟡 cost_model_comparison is extremely redundant — 9 metrics × 5 fields (current/baseline/delta/delta_pct/regressed) when all values are identical. This is 100+ lines of JSON that add no information.
Suggestion: If all deltas are zero, store just "cost_model_regressed": false (which already exists) and skip the per-metric breakdown, or emit the comparison only when regressions are detected.
🟡 perf_counters.add = 3 for a single add + ReLU — For a + bias followed by ReLU, you'd expect 1 add + 1 branch (or a single fused compare-and-select). 3 adds suggests the compiler is expanding ReLU into an add sequence, which may indicate a missed optimization.
Suggestion: Verify the assembler output at build/topic06/relu_add.s — if ReLU compiles to multiple adds instead of a branch/move, that's a compiler optimization gap, not a report bug.
💭 expected_type: "scalar" but expected: 3 (int, no decimal) — The interpreter produced 3.0 (float). The schema treats the expected value as an integer but actual as float. Minor type mismatch risk if consumed programmatically.
Suggestion: Ensure expected matches the type implied by expected_type, or document that the comparison is type-coerced.
💭 output_dtype: null, output_shape: null — For a scalar output, these are arguably unnecessary. If the schema allows null for scalars, fine; otherwise consider "output_dtype": "float32" and "output_shape": [].
🟡 Machine-specific paths — Lines containing compile_command: embeds Windows absolute paths (D:\anaconda3\python.exe, D:\PycharmProjects\...). These break reproducibility on other environments/CI runners. Consider replacing with relative paths or a canonical placeholder.
🟡 Baseline is self-referential — Every cost_model_comparison entry has delta: 0 and delta_pct: 0.0 because baseline equals current exactly. This means the regression detector can never fire. Ensure baseline values come from a prior run, not the current run's own metrics.
🟡 Type inconsistency — expected: 4 (int) vs actual: 4 (int) vs interpreter_actual: 4.0 (float). The comparison logic must handle this. Consider normalizing all numeric results to the same type to avoid subtle float/int comparison bugs downstream.
💭 Missing newline at EOF — Last line lacks trailing newline. Harmless but triggers warnings from git diff, git commit, and some linters.
💭 output_dtype: null / output_shape: null — For a scalar expected type these are plausibly null, but consider documenting the convention explicitly so it's not mistaken for incomplete metadata.
💭 Over-precision on timings — compile_time_sec: 0.08641380001790822 carries float noise beyond meaningful precision. Consider rounding to ~6 decimals for readability and to avoid spurious diffs.
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
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.
本次更新
当前结果
已知限制