From c35b86b8bbf717b0879ce888aba437290a566c0b Mon Sep 17 00:00:00 2001 From: mrciffa <49000955+davide221@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:33:42 +0200 Subject: [PATCH] ci: make manual speed profiles trustworthy --- .github/workflows/speed-profile.yml | 67 +++++++++++++++++++++-------- docs/specs/speed-profile.md | 36 +++++++++++----- 2 files changed, 76 insertions(+), 27 deletions(-) diff --git a/.github/workflows/speed-profile.yml b/.github/workflows/speed-profile.yml index a9c958d9a..55a24e8b3 100644 --- a/.github/workflows/speed-profile.yml +++ b/.github/workflows/speed-profile.yml @@ -1,9 +1,10 @@ name: Speed Profile -# Report-only speed profile for the inference engine. Runs on the self-hosted -# RTX 3090 (lucebox-rtx3090) on PRs that touch the engine or the optimizations, and on -# manual dispatch. It NEVER blocks a PR (continue-on-error: true) — it publishes a -# report to the run summary + uploads the JSON / markdown / nsys trace as artifacts. +# Speed profile for the inference engine. Runs on the self-hosted RTX 3090 +# (lucebox-rtx3090) on PRs that touch the engine or the optimizations, and on manual +# dispatch. PR runs are report-only; manual runs fail when the runner is not ready or +# the performance result is missing, noisy, or regressed. Every successful run publishes +# a report to the run summary + uploads the JSON / markdown / nsys trace as artifacts. # # Why report-only: perf has run-to-run variance (thermals, clocks, scheduling). # Gating a merge on a noisy absolute number produces false failures. We surface the @@ -30,7 +31,9 @@ jobs: name: Speed profile (self-hosted RTX 3090, sm_86) runs-on: [self-hosted, lucebox-rtx3090] timeout-minutes: 30 - continue-on-error: true # report-only: a slow/failed profile must not block the PR + # Keep automatic PR profiling advisory, but make a manual promotion check honest: + # setup/profile failures and invalid performance evidence must return a red run. + continue-on-error: ${{ github.event_name == 'pull_request' }} # Model paths live on the runner, not in the repo (multi-GB weights). They are # overridable via repo variables so the runner owner can point at whatever is @@ -64,8 +67,8 @@ jobs: run: | # The weights are staged on the self-hosted runner out of band. If they are # absent the engine binary aborts with a cryptic gguf "No such file" error, so - # check up front and SKIP cleanly instead — this job is report-only, and a - # missing model on the runner is an environment issue, not a PR defect. + # check up front. A PR run skips cleanly because a missing runner asset is not + # a PR defect; a manual promotion check fails because it produced no evidence. target="$MODELS/$TARGET_MODEL" draft="$MODELS/$DRAFT_MODEL" present=true @@ -94,6 +97,10 @@ jobs: echo "Available draft candidates under \`$MODELS\`:" find "$MODELS" -maxdepth 4 -type f \( -name '*.gguf' -o -name '*.safetensors' \) -print 2>/dev/null | sort | sed 's/^/ - /' || true } >> "$GITHUB_STEP_SUMMARY" + if [ "$GITHUB_EVENT_NAME" = "workflow_dispatch" ]; then + echo "::error title=Speed profile unavailable::Model weights not found under $MODELS — see the run summary." + exit 1 + fi echo "::warning title=Speed profile skipped::Model weights not found under $MODELS — see the run summary." fi @@ -131,17 +138,21 @@ jobs: if: steps.models.outputs.present == 'true' run: | cd server - # Use a committed baseline if one is staged so the report can flag a - # regression. Seed it once from a green `main` run's profile.json artifact - # (see docs/specs/speed-profile.md); without it the delta is simply skipped. - # The path is overridable so the runner owner can point elsewhere. + # Use the runner-staged baseline so a PR cannot weaken its own comparison. + # Seed it from a stable `main` run's profile.json artifact and configure + # LUCEBOX_SPEED_BASELINE (see docs/specs/speed-profile.md). The repository + # path remains a convenient fallback for local/report-only use. baseline="${LUCEBOX_SPEED_BASELINE:-scripts/speed-baseline.json}" baseline_arg=() if [ -f "$baseline" ]; then baseline_arg=(--baseline "$baseline" --regress-pct "${LUCEBOX_SPEED_REGRESS_PCT:-0.10}") echo "Regression check against baseline: $baseline" else - echo "No baseline at $baseline — regression flagging disabled this run." + if [ "$GITHUB_EVENT_NAME" = "workflow_dispatch" ]; then + echo "::error title=Speed baseline missing::No baseline at $baseline; a manual promotion check cannot detect regressions." + exit 1 + fi + echo "::warning title=Speed baseline missing::No baseline at $baseline — regression flagging disabled this PR run." fi # Use 128 generated tokens per prompt by default: long enough to reduce # startup/noise effects while keeping the serialized 3090 queue bounded. @@ -174,16 +185,27 @@ jobs: echo "Profiler produced no report (the run failed earlier — see logs)." >> "$GITHUB_STEP_SUMMARY" fi - - name: Flag losslessness / regressions (annotations, non-blocking) + - name: Validate profile result if: always() && steps.models.outputs.present == 'true' run: | - [ -f server/profile.json ] || exit 0 - # Report-only: emit warnings, never fail. A losslessness FAIL means the fast + if [ ! -f server/profile.json ]; then + if [ "$GITHUB_EVENT_NAME" = "workflow_dispatch" ]; then + echo "::error title=Speed profile missing::The profiler produced no profile.json artifact." + exit 1 + fi + echo "::warning title=Speed profile missing::The profiler produced no profile.json artifact." + exit 0 + fi + # A losslessness FAIL means the fast # path changed the output and it is NOT run-to-run noise (AR agreed with # itself) — worth triaging (real bug vs batched-verify FP). Inconclusive - # prompts (engine intrinsically nondeterministic) are NOT failures. + # prompts (engine intrinsically nondeterministic) are NOT failures. Keep this + # advisory until the profiler records the first-divergence logit gap and can + # distinguish a near-tie FP flip from a logic bug. python3 - <<'PY' import json + import os + d = json.load(open("server/profile.json")) ll, reg, noise = d.get("lossless", {}), d.get("regression", {}), d.get("summary", {}).get("noise", {}) if ll and not ll.get("lossless", True): @@ -197,6 +219,17 @@ jobs: print(f"::warning title=Noisy speed profile::{','.join(noise.get('metrics', []))} exceeded " f"the relative stddev threshold ({noise.get('threshold_rsd', 0)*100:.1f}%). " "Treat small deltas as below the profiler detection threshold.") + if os.environ.get("GITHUB_EVENT_NAME") == "workflow_dispatch": + failures = [] + if not reg: + failures.append("the baseline comparison is missing") + elif reg.get("regressed"): + failures.append("a performance regression exceeded the configured threshold") + if noise.get("noisy"): + failures.append("the measurements were too noisy for a promotion decision") + if failures: + print(f"::error title=Speed profile rejected::{'; '.join(failures)}.") + raise SystemExit(1) PY - name: Reset GPU clocks @@ -212,5 +245,5 @@ jobs: server/profile.json server/profile.md server/profile.nsys-rep - if-no-files-found: warn + if-no-files-found: ${{ github.event_name == 'workflow_dispatch' && 'error' || 'warn' }} retention-days: 30 diff --git a/docs/specs/speed-profile.md b/docs/specs/speed-profile.md index 7896ee266..8b70c81df 100644 --- a/docs/specs/speed-profile.md +++ b/docs/specs/speed-profile.md @@ -45,17 +45,20 @@ and they stay fixed so every run is comparable over time: variance (thermals, clock boosting, scheduler jitter), then reports mean and stddev for the headline metrics. Use `--reps 3` for a faster smoke profile when needed, but PR comparisons should prefer 5+. -- **`--noise-rsd-pct 0.05`** — report-only noise threshold. If any tracked +- **`--noise-rsd-pct 0.05`** — noise threshold. If any tracked headline metric has relative stddev above 5%, the markdown calls the profile **NOISY** and tells reviewers to treat small deltas as below the profiler - detection threshold. + detection threshold. Automatic PR profiles report this as a warning; a manual + promotion profile fails because noisy measurements are not valid release evidence. **Rule:** keep these consistent. A delta vs the baseline is only a valid regression signal if both runs used the same config — if you ever change a parameter, re-seed the baseline (you cannot compare across configs). When baseline and current 1σ intervals overlap and the delta is smaller than `--regress-pct`, the report marks that row as -**noisy / overlap** instead of inviting reviewers to chase a ghost regression. All of -these states are warnings only; the profiler remains report-only. +**noisy / overlap** instead of inviting reviewers to chase a ghost regression. These +states are warnings on automatic PR profiles. A manual dispatch is the promotion check: +it fails on a missing runner model, missing baseline/report, noisy result, or a +performance regression outside the configured threshold. ## Losslessness gate (and why a bit-exact compare is too strict on its own) @@ -88,7 +91,9 @@ path genuinely changed the output — but that is still not proven a logic bug: the batched-verify FP effect above (verify scores draft tokens as a batch vs AR one-at-a-time). Classifying a FAIL as bug-vs-FP needs the **logit gap** at the first mismatch (near-tie = FP, clear gap = bug) — a follow-up the binaries don't emit yet. CI -surfaces a FAIL as a non-blocking `::warning::` for triage; it stays report-only. +surfaces a FAIL as a non-blocking `::warning::` for triage, including on manual +promotion profiles. It cannot safely become a blocking signal until the binaries expose +the logit gap at the first mismatch and the profiler can classify FP near-ties. ## CI settings @@ -98,6 +103,13 @@ temporarily override those values with repo variables `LUCEBOX_SPEED_N_GEN`, `LUCEBOX_SPEED_REPS`, and `LUCEBOX_SPEED_NOISE_RSD_PCT`, but PR-to-PR comparisons should keep them fixed. +Automatic pull-request runs are advisory and never block a merge. A manual dispatch is +strict enough to use as promotion evidence: it requires the staged target/draft models, +an existing baseline at `LUCEBOX_SPEED_BASELINE`, a complete JSON report, stable +measurements, and no regression beyond `LUCEBOX_SPEED_REGRESS_PCT` (10% by default). +Correctness divergences remain explicit warnings for the reason described above; use the +normal GPU correctness suite as the blocking correctness gate. + ## Run it locally ```bash @@ -114,8 +126,9 @@ python3 scripts/profile.py \ ## Comparing against a baseline -The profiler is **report-only**, but it can diff the current run against a saved -profile so reviewers see a single regression table instead of two separate reports. +The profiler can diff the current run against a saved profile so reviewers see a single +regression table instead of two separate reports. The delta is advisory on PR events and +blocking on manual promotion runs. The comparison is a JSON round-trip: 1. **Capture a baseline.** Run the profiler on the reference commit and keep its @@ -125,9 +138,12 @@ The comparison is a JSON round-trip: python3 scripts/profile.py ... --out-json scripts/speed-baseline.json ``` - Commit `scripts/speed-baseline.json` so every later run compares against the same - reference. Re-seed it whenever you change a profiler parameter (`--budget`, - `--n-gen`, `--reps`, …): you cannot compare across configs. + For CI, stage that file outside the checkout on the self-hosted runner and set the + repository variable `LUCEBOX_SPEED_BASELINE` to its absolute path. Keeping the + promotion baseline outside the PR checkout prevents a candidate from weakening its + own comparison. `scripts/speed-baseline.json` remains the local fallback. Re-seed the + runner baseline from a reviewed `main` run whenever you intentionally change a + profiler parameter (`--budget`, `--n-gen`, `--reps`, …): configs cannot be compared. 2. **Compare a later run.** Point `--baseline` at that file and set the regression threshold: