|
| 1 | +# pytorch-advisor-coverage |
| 2 | + |
| 3 | +A standalone AWS Lambda that drives down the `/flaky_trunk` page's |
| 4 | +**unclassified** bucket — the isolated, non-persistent trunk reds that have no |
| 5 | +attaching advisor verdict and that autorevert never evaluates (~40% of trunk |
| 6 | +reds). It dispatches the `claude-autorevert-advisor.yml` workflow on those reds, |
| 7 | +keyed to the OBSERVED trunk commit. The workflow writes the verdict JSON to S3; |
| 8 | +the existing `clickhouse-replicator-s3` Lambda ingests it into |
| 9 | +`misc.autorevert_advisor_verdicts`. |
| 10 | + |
| 11 | +It vendors a few self-contained helpers from the sibling `pytorch_auto_revert` |
| 12 | +package (ClickHouse/GitHub client factories, the dispatch primitive, |
| 13 | +`RetryWithBackoff`, `parse_datetime`) but does NOT reuse its partition/signal |
| 14 | +logic — targeting is a direct ClickHouse enumeration. |
| 15 | + |
| 16 | +## What "unclassified" means (reused verbatim from /flaky_trunk) |
| 17 | + |
| 18 | +Eligibility reuses the exact category=5 definition from torchci |
| 19 | +`flaky_trunk_jobs/query.sql`: a trunk red is **unclassified** when it |
| 20 | + |
| 21 | +- is red at a trunk commit, AND |
| 22 | +- has NO attaching advisor verdict (related/revert/infra_issue/not_related/garbage) |
| 23 | + keyed to that commit, AND |
| 24 | +- is NOT structurally persistent (no adjacent hard-red on the previous or next |
| 25 | + trunk commit) — i.e. an isolated single-commit hard-red, a retry-green, or a |
| 26 | + green→red→green flake. |
| 27 | + |
| 28 | +The advisor classifies these well (a feasibility test caught a real regression |
| 29 | +the persistence heuristic misses). |
| 30 | + |
| 31 | +## The revert-isolation invariant (load-bearing) |
| 32 | + |
| 33 | +The verdicts table is a **live control input**: autorevert reads it back and a |
| 34 | +high-confidence `revert`/`related` verdict triggers a real revert. Its read-back |
| 35 | +is an EXACT `signal_key` match. |
| 36 | + |
| 37 | +Every coverage verdict carries `signal_key = "coverage_" + <native job signal_key>` |
| 38 | +(the normalized job key — config kept, shard index + runner dropped). The prefix |
| 39 | +keeps it out of autorevert's extracted key set, so it is **never attached and |
| 40 | +never drives a revert or veto** — the same mechanism the PR-side Dr.CI path |
| 41 | +(`dr_ci_`) already uses. |
| 42 | + |
| 43 | +- The prefix is a **hard-coded module constant** `COVERAGE_SIGNAL_KEY_PREFIX` |
| 44 | + (`config.py`), NOT env/event configurable. |
| 45 | +- It **must stay equal** to the `'^coverage_'` literal stripped in the |
| 46 | + `advisor_agg` CTE of torchci's flaky_trunk `query.sql` files (`flaky_trunk_jobs`, |
| 47 | + `flaky_trunk_timeseries`, `flaky_trunk_entity_runs`, `flaky_trunk_runner_labels`); |
| 48 | + that strip normalizes a coverage verdict onto the native job so it classifies |
| 49 | + the red on /flaky_trunk (see "How verdicts land on /flaky_trunk" below). |
| 50 | +- A dispatch-time guard refuses to POST if the outgoing key is not safely |
| 51 | + prefixed (never equal to the native key). |
| 52 | + |
| 53 | +## How a red becomes a dispatch |
| 54 | + |
| 55 | +1. **Enumerate** currently-unclassified trunk reds for the window — one row per |
| 56 | + NORMALIZED job (config kept; shard index + runner dropped), reusing the |
| 57 | + flaky_trunk CTE chain — plus a few green baseline-before commits of that job |
| 58 | + (a second CH query). |
| 59 | +2. **Windowless dedup**: skip any red that already has a `coverage_`-prefixed |
| 60 | + verdict for `(repo, observed commit, key)` — no time filter. |
| 61 | +3. **Log-readability pre-filter**: HEAD-check the raw log at |
| 62 | + `ossci-raw-job-status/log/{job_id}` and skip stubs (<1000 bytes) or missing |
| 63 | + logs (~16% are unusable). Counted as `skipped_no_log`. |
| 64 | +4. **Build** the isolated-red `signal_pattern` (failed suspect + green baselines) |
| 65 | + and **dispatch** with `suspect_commit = observed trunk commit`, `pr_number="0"`. |
| 66 | + |
| 67 | +The advisor gets the diff from the workflow's OWN checkout of `suspect_commit`, |
| 68 | +so the dispatch token needs no PR-read scope (`pr_number` is just metadata). |
| 69 | + |
| 70 | +## How verdicts land on /flaky_trunk |
| 71 | + |
| 72 | +Because a coverage verdict is keyed at the normalized-job level (`"coverage_" + |
| 73 | +<native job signal_key>`), the `/flaky_trunk` SQL strips the leading `coverage_` |
| 74 | +in `advisor_agg` and joins it on the SAME normalized job the page displays. A |
| 75 | +coverage verdict therefore classifies — and reclassifies out of the unclassified |
| 76 | +bucket — that normalized job at the observed commit, and the page prefers a native |
| 77 | +verdict over a coverage one for the same (commit, job). No confidence gate is |
| 78 | +applied: any non-`unsure` verdict is a classification. |
| 79 | + |
| 80 | +## What it does NOT do |
| 81 | + |
| 82 | +- Writes **nothing** to ClickHouse or S3. It only READS ClickHouse, HEAD-checks |
| 83 | + S3 logs, and POSTs `workflow_dispatch`. |
| 84 | +- The minted GitHub installation token is scoped to `actions:write` only — it |
| 85 | + cannot push or revert. |
| 86 | + |
| 87 | +## Ongoing vs backfill |
| 88 | + |
| 89 | +- **Ongoing** (`MODE=ongoing`, the EventBridge cron): enumerate `[now - HOURS, now)`. |
| 90 | +- **Backfill** (`MODE=backfill`): tile `[AS_OF_START, AS_OF_END)` into |
| 91 | + `AS_OF_STEP_HOURS` chunks and dispatch each chunk's unclassified reds, |
| 92 | + throttled + resumable. |
| 93 | + |
| 94 | +### Running a long backfill (resume cursor) |
| 95 | + |
| 96 | +A single Lambda invocation is bounded by a wall-clock budget and the dispatch |
| 97 | +cap; when it stops early it returns `next_as_of`. Re-invoke with |
| 98 | +`as_of_start = next_as_of` until it is `null`: |
| 99 | + |
| 100 | +```jsonc |
| 101 | +{"mode": "backfill", "as_of_start": "2026-02-19", "as_of_end": "2026-08-18"} |
| 102 | +// response -> {"next_as_of": "2026-02-20T00:00:00", ...} ; repeat until null |
| 103 | +``` |
| 104 | + |
| 105 | +Locally, one process completes the whole range (unlimited budget, only the gap |
| 106 | +throttle applies): |
| 107 | + |
| 108 | +```bash |
| 109 | +MODE=backfill AS_OF_START=2026-02-19 AS_OF_END=2026-08-18 \ |
| 110 | + python -m advisor_coverage.backfill |
| 111 | +``` |
| 112 | + |
| 113 | +## Kill switch |
| 114 | + |
| 115 | +- Set `DRY_RUN="true"` (the deploy default): logs intended dispatches, never |
| 116 | + POSTs. Flipping to `"false"` arms real dispatch. |
| 117 | +- Or disable the EventBridge rule for the ongoing cron. |
| 118 | + |
| 119 | +## Throttle |
| 120 | + |
| 121 | +- `MAX_DISPATCHES_PER_RUN` (default 10) — per invocation, clamped by a compiled |
| 122 | + `HARD_CAP` (100) and the Lambda timeout budget; env/event may only LOWER it. |
| 123 | +- `DISPATCH_GAP_SECONDS` (default 3) — sleep between dispatches (floored to 1s). |
| 124 | + |
| 125 | +Cross-invocation duplicates (a red re-dispatched before its verdict lands) are |
| 126 | +accepted: safe (prefixed → no reverts) and bounded by the throttle. Intra-run |
| 127 | +duplicates from overlapping windows are suppressed in memory. |
| 128 | + |
| 129 | +## Known limitation |
| 130 | + |
| 131 | +Backfill chunks are enumerated with a persistence lookback/lookahead margin |
| 132 | +(`PERSISTENCE_MARGIN_HOURS`) on each side, so a red at a chunk boundary still |
| 133 | +sees its neighbouring trunk commits for the lag/lead persistence check (reds are |
| 134 | +dispatched only within the core chunk). The one remaining edge is inherent: the |
| 135 | +newest trunk commit in ongoing mode has no lookahead (the next commit doesn't |
| 136 | +exist yet), so a just-observed red that will turn out persistent may be |
| 137 | +dispatched once. This is bounded, safe (non-reverting), and matches |
| 138 | +/flaky_trunk's own newest-commit semantics. |
| 139 | + |
| 140 | +## Environment variables |
| 141 | + |
| 142 | +| Var | Default | Notes | |
| 143 | +|---|---|---| |
| 144 | +| `CLICKHOUSE_HOST` / `CLICKHOUSE_PORT` | `localhost` / `8443` | read-only CH | |
| 145 | +| `CLICKHOUSE_USERNAME` / `CLICKHOUSE_PASSWORD` | `""` | reuse `CLICKHOUSE_USER_AUTO_REVERT`; password from Secrets Manager | |
| 146 | +| `CLICKHOUSE_DATABASE` | `default` | | |
| 147 | +| `GITHUB_APP_ID` / `GITHUB_INSTALLATION_ID` | `""` / `0` | GitHub App; PEM from Secrets Manager | |
| 148 | +| `GITHUB_APP_SECRET` / `GITHUB_TOKEN` | `""` | base64 PEM / raw token (local dev) | |
| 149 | +| `SECRET_STORE_NAME` | `""` | `pytorch-autorevert-secrets` in prod | |
| 150 | +| `REPO_FULL_NAME` | `pytorch/pytorch` | pinned to an allowlist | |
| 151 | +| `WORKFLOWS` | empty = all | optional JSON array / CSV filter | |
| 152 | +| `HOURS` | `24` | ongoing enumeration window | |
| 153 | +| `MIN_RUNS` | `20` | min total runs for a job to be enumerated (matches /flaky_trunk) | |
| 154 | +| `MODE` | `ongoing` | `ongoing` \| `backfill` | |
| 155 | +| `AS_OF_START` / `AS_OF_END` | — | backfill range (UTC) | |
| 156 | +| `AS_OF_STEP_HOURS` | `24` | backfill chunk size | |
| 157 | +| `MAX_DISPATCHES_PER_RUN` | `10` | see Throttle | |
| 158 | +| `DISPATCH_GAP_SECONDS` | `3` | see Throttle | |
| 159 | +| `DRY_RUN` | `true` | `false` arms real dispatch | |
| 160 | +| `LOG_LEVEL` | `INFO` | secret-leaking loggers pinned to WARNING regardless | |
| 161 | + |
| 162 | +The `coverage_` prefix is intentionally NOT an env var. |
| 163 | + |
| 164 | +## Local run + tests |
| 165 | + |
| 166 | +```bash |
| 167 | +python -m advisor_coverage # ongoing dry-run (set CLICKHOUSE_* first) |
| 168 | +make test # mocked unit tests, no network |
| 169 | +``` |
| 170 | + |
| 171 | +## Build / deploy |
| 172 | + |
| 173 | +`make deployment.zip` vendors the three needed `pytorch_auto_revert` helper |
| 174 | +modules plus the Python deps. The handler is |
| 175 | +`advisor_coverage.handler.lambda_handler`. Deploy plumbing (terraform + |
| 176 | +EventBridge cron + IAM) lives in `pytorch-gha-infra`. |
0 commit comments