Unify benchmark timing contracts and add calibrated CI gates - #924
Open
jhinpan wants to merge 4 commits into
Open
Unify benchmark timing contracts and add calibrated CI gates#924jhinpan wants to merge 4 commits into
jhinpan wants to merge 4 commits into
Conversation
Move the final synchronization outside the measured window so the benchmark matches its documented host-dispatch semantics, and lock the ordering with a regression test.
Consolidate equivalent event timers behind the shipped do_bench implementation while preserving distinct profiler, host, graph, and distributed semantics. Carry raw latency and measurement contracts into CI so calibrated regressions fail closed without breaking existing dashboard output.
2 tasks
Apply the repository Black configuration so the focused style check accepts every Python file touched by the umbrella PR.
Collaborator
Author
|
cc @coderfeli This PR is trying to unify benchmarking wit halso adding calibrated CI gates. Could u help review if u get some time~ But not that priority. Thx! |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The problem
FlyDSL times kernels in at least six hand-written CUDA/HIP event loops. They are not copies of each other — they measure physically different things, but nothing in their names or their output says which:
torch.cuda.synchronize()after eachflush_buf.zero_()before each callOn the same kernel these differ by multiples. Each is a legitimate measurement; comparing across them is not. Units drifted along with them — some helpers return
ms, someus, some hand-roll* 1e3.The CI side had the matching gap. The benchmark CSV kept only
tbps/tflopsto three decimals and discarded the raw latency it had just measured, socompare_benchmark.pycould report a ratio but could never fail a run. A performance regression was invisible to CI, and nothing recorded how any number had been produced.Measurements, raw samples, and the design rationale behind the choices below are in the timing audit Gist.
What changes
1. One canonical event timer, with the schedule as an explicit argument
flydsl.do_benchbecomes the single eager CUDA/HIP event implementation. The four loop shapes above are now three named schedules plus a cache-policy flag:isolated— sync after every call (the existing autotuner contract)pipelined— one event pair around all iterations, one averaged sampleper_iter— one event pair per call, a single sync at the endPhysically impossible combinations are rejected at the entry point rather than returning a plausible-looking number:
pipelinedproduces exactly one sample, so it accepts onlystatistic="mean"and refusesprep_fn/ cache flushing.isolateddeliberately keeps the full-devicetorch.cuda.synchronize()instead ofend.synchronize(), because autotuned callables may enqueue work on streams other than the current one.Results come back as
BenchResult, which stores microseconds and carries the schedule, statistic, cache policy, warmup, and iteration count that produced them.2. Equivalent loops become thin wrappers
bench_gpu_us_torch,bench_kernel_us, and the MoE-sorting / TDM-bandwidth / RoPE-cache / a8w4 timers delegate todo_benchwhile preserving their existing behavior, includingbench_kernel_us's branch between pipelined-mean and per-iter-median-with-IQR.Two adjacent fixes fall out of the audit:
bench_wallclockdocumented host dispatch time but included the final GPU drain in the measured window. The sync now happens after the timer stops, locked by a regression test.bench_kernel_usthere is renamedbench_profiled_device_usto match what it measures.3. Benchmark records carry their own measurement contract
run_benchmark.sh --output_csvkeeps its existing throughput columns and appendsavg_us, sample statistics,statistic/instrument/schedule/cache_policy/warmup/iters, andarch. Benchmarks emit a self-describingBenchmark contract: ...line that the parser prefers over its per-op fallback table.The human-readable five-column table is byte-for-byte unchanged, because tag baselines and the dashboard parse it. The only edit there removes field truncation (
%-22.22sto%-22s) so long keys survive intact.The two inline heredoc parsers in
run_benchmark.sh(~140 lines of regex) move intoscripts/benchmark_log_parser.py, which makes them testable.4. A calibrated regression gate
scripts/benchmark_compare.pylifts the relative-and-absolute rule that allreduce has used for a while into a shared helper, and allreduce now calls it. A row regresses only when it is both more than X% slower and more than N microseconds slower, so a 5 us kernel drifting 20% does not page anyone.This is the part that changes CI behavior for everyone: the
current vs mainbenchmark comparison now runs with--fail-on-regressionand can turn a PR job red.Scope of the gate is deliberately narrow, defined by the versioned allowlist in
.github/benchmark_thresholds.json:softmax/layernorm/rmsnormat32768x8192bf16 — three rows per archThe gate compares raw
avg_us, not TB/s. Throughput is derived — change the bytes formula and the number moves without the kernel moving.Fails closed (a broken measurement must not become a way past the gate):
statistic,instrument,schedule,cache_policy,warmup,iters) differs between the two sidesFails open (no false alarms from missing history):
There is also a runner-trust check: when
--archis supplied, every row in both CSVs must report that architecture, otherwise the comparator exits 2 rather than gating gfx950 results against a gfx942 baseline.Two workflow details make the comparison well-formed. The main-baseline worktree is given the current branch's
run_benchmark.shandbenchmark_log_parser.py, so both sides emit the same schema. The pip-installed tag baseline cannot be patched that way, sobenchmark_output_to_csv.pynow emits the same 17-column header with placeholders — those rows land on[GATE SKIP: baseline has no raw us]instead of breaking the parse.Finally, dashboard ingest accepts
conclusion == "failure"in addition to"success". Without that, the first time the gate actually fires the dashboard would go blank exactly when the regression data is most useful.What this PR does not change
run_perftestprofiler attribution, host wall clock, graph replay, and distributed timing measure different quantities and are not folded in. Over-merging is what created the original problem; the docs now say so explicitly.do_bench(fn, warmup, rep, quantiles)keeps its scalar/list milliseconds, isolated schedule, and median statistic.Reviewer guide
Suggested order, roughly one concept at a time:
python/flydsl/autotune.py— the timer, the schedules,BenchResult, and the rejected-combination guardstests/unit/test_benchmark_timer.py— the semantics the timer is now pinned totests/kernels/benchmark_common.py— what a migrated wrapper looks likescripts/benchmark_log_parser.pyandscripts/benchmark_output_to_csv.py— how the contract reaches the CSVscripts/benchmark_compare.py,scripts/compare_benchmark.py,.github/benchmark_thresholds.json— the policy and its calibration.github/workflows/flydsl.yaml— where the gate is wired and why both baselines get the same parsertests/files — mechanical migrationsTest plan
test_benchmark_timer.py), comparator (test_benchmark_compare.py), parser (test_benchmark_log_parser.py), dashboard ingest, and existing autotune suitesbash -n scripts/run_benchmark.sh, workflow YAML parse, threshold JSON parse,git diff --checkFollow-ups
calibrationfield of the JSON.reported/unknowncontract values in the CSV mark benchmarks that have not yet been migrated to a declared schedule.Notes
A repository-wide Ruff run still reports one pre-existing import-order issue in
tests/unit/test_cluster_mcast_gemm_gfx1250.py; no file changed by this PR has lint diagnostics.