|
| 1 | +--- |
| 2 | +name: fix-ci |
| 3 | +description: Iteratively fix CI failures for a Julia package PR through a commit-push-triage loop, catching CPU and GPU compilation issues locally before they reach CI |
| 4 | +--- |
| 5 | + |
| 6 | +# Fixing CI through an iterative loop |
| 7 | + |
| 8 | +You are fixing CI for a pull request by iterating: triage every failure, fix |
| 9 | +what can be verified locally, commit small formatter-clean commits, push, and |
| 10 | +triage the next build. Continue until CI is green or you hit a decision only |
| 11 | +the user can make. The workflow below was developed on a large ClimaCore.jl |
| 12 | +refactoring PR; adapt the specifics to the repository at hand. |
| 13 | + |
| 14 | +## 0. Secure access to CI results before starting |
| 15 | + |
| 16 | +You cannot fix what you cannot read, and hours spent replicating a failure |
| 17 | +locally are wasted if one log line would have explained it — ask for missing |
| 18 | +access the moment you need a log. Before the first fix: |
| 19 | +- **GitHub Actions**: raw job logs require authentication. Check `gh auth |
| 20 | + status`; if `gh` is missing or logged out, STOP and ask the user to run |
| 21 | + `brew install gh` and `gh auth login` — do not spend hours replicating |
| 22 | + failures locally that one log line would explain. Then use |
| 23 | + `gh api repos/<org>/<repo>/commits/<sha>/check-runs` for statuses and |
| 24 | + `gh api repos/<org>/<repo>/actions/jobs/<job id>/logs` for logs |
| 25 | + (check-run id == job id). |
| 26 | +- **Buildkite** (CliMA's GPU/MPI CI): public, no auth needed. |
| 27 | + Build state: `curl -s -H "Accept: application/json" |
| 28 | + https://buildkite.com/<org>/<pipeline>/builds/<N>.json`. Failed steps: |
| 29 | + `.../builds/<N>/data/steps?exclude_group_steps=true&state=failed` (each has |
| 30 | + `label` and `statistics.latest_job_id`). Job log: |
| 31 | + `.../organizations/<org>/pipelines/<pipeline>/builds/<N>/jobs/<job>/log` |
| 32 | + with the same Accept header (strip HTML tags, unescape entities). |
| 33 | +- Map a commit to its builds via |
| 34 | + `gh api repos/<org>/<repo>/commits/<sha>/status`. |
| 35 | +- Downstream-package failures may come from **registered dependency versions** |
| 36 | + rather than the repos themselves (grep `~/.julia/packages/` too), and tests |
| 37 | + may be skipped locally because binary artifacts are platform-specific, so a |
| 38 | + local pass does not always predict CI. |
| 39 | + |
| 40 | +## 1. Run the loop |
| 41 | + |
| 42 | +- Triage every hard failure of a build before fixing anything; cluster |
| 43 | + failures by root cause (one fix often clears many jobs). Read logs for the |
| 44 | + *first* error and for which testsets completed — but remember only the |
| 45 | + top-level `@testset` prints a summary, so "no summary" does not mean the |
| 46 | + first inner testset crashed. |
| 47 | +- Fix one root cause per commit, verify it locally (section 3), format with |
| 48 | + the same JuliaFormatter major version the CI action uses (`format(".")` |
| 49 | + before every commit), write a message explaining cause and verification, |
| 50 | + and push. Pushes auto-cancel in-flight builds: never push while waiting on |
| 51 | + a diagnostic job's results. |
| 52 | +- For failures that only manifest on CI (runtime GPU errors, segfaults), you |
| 53 | + can get a full answer in ONE round with a temporary soft-fail diagnostic |
| 54 | + job: run each suspect case via `Distributed.remotecall_eval` on a worker |
| 55 | + process, catch `ProcessExitedException`, respawn, and print a PASS/CRASH |
| 56 | + map. Remove temporary jobs once the cause is fixed. |
| 57 | +- Track your progress; when new failures appear after your own push, suspect |
| 58 | + your latest commits first and bisect against the previous build's results. |
| 59 | + |
| 60 | +## 2. Distinguish design from detail |
| 61 | + |
| 62 | +- Never redesign or bypass a feature the PR exists to introduce just to make |
| 63 | + a test pass. If a central function misbehaves, find the lower-level cause |
| 64 | + inside it. (On this PR, twice a `foreach_point` problem was "solved" by |
| 65 | + adding an index-based workaround that circumvented the PR's kernel-fusion |
| 66 | + design; both times the correct fix was a small bug underneath.) |
| 67 | +- Stop and ask the user before changing public APIs, algorithmic behavior, or |
| 68 | + anything they described as a goal of the PR. Do NOT stop for mechanical |
| 69 | + choices (test tolerances, comment wording, internal helper structure) — |
| 70 | + pick the option supported by measurements and keep moving. |
| 71 | +- Fix problems at the level where the design lives, in their most general |
| 72 | + form. Recurring shapes of this rule, all from review rounds on this PR: |
| 73 | + - If a validation helper rejects legitimate inputs, generalize the |
| 74 | + validation; do not switch call sites to an unchecked variant. |
| 75 | + - If one code path needs an index style the slice machinery cannot provide, |
| 76 | + check whether that path should be using the machinery at all, instead of |
| 77 | + adding a parallel operator to the machinery. |
| 78 | + - When a pattern is justified by measurement (e.g. passing arguments |
| 79 | + separately instead of capturing them in closures), apply it to every |
| 80 | + sibling call site, not just the one that was measured. |
| 81 | + - Prefer facts derived from existing machinery (e.g. `return_type` of the |
| 82 | + actual operator) over hand-maintained parallel tables of the same facts — |
| 83 | + but verify the derivation constant-folds wherever the fact is consumed, |
| 84 | + including inside kernels. |
| 85 | + - A value-domain branch on argument properties often replaces several |
| 86 | + dispatch methods, eliminating whole ambiguity classes; conversely, a |
| 87 | + variadic method that overlaps a typed method is an ambiguity waiting to |
| 88 | + happen (require two leading typed arguments, or split by argument type). |
| 89 | +- When a test's expectation is wrong rather than the code (stale baselines, |
| 90 | + `@test_broken` that now passes), update the expectation — but only flip |
| 91 | + markers that pass in *every* invocation across at least two builds, and use |
| 92 | + `skip` instead of `broken` when the pass/fail set is unstable. |
| 93 | + |
| 94 | +## 3. Verify locally before pushing; use CI only for what needs hardware |
| 95 | + |
| 96 | +- CPU: run the failing test files directly (match CI's flags, e.g. |
| 97 | + `--check-bounds=yes`, same project environment). |
| 98 | +- GPU without a device: almost everything except instruction selection and |
| 99 | + runtime errors can be checked locally. Use the TestCompilation module in |
| 100 | + this folder (`include("test_compilation.jl")`): **first run |
| 101 | + `test_compilation_tests.jl` to completion** — if its own tests fail in your |
| 102 | + environment, fix that before trusting any `@test_compilation` result. Then |
| 103 | + check every function you touched over the argument types CI uses. |
| 104 | +- If a CUDA device IS available locally, iterate on the actual failing GPU |
| 105 | + tests locally instead. |
| 106 | +- Benchmarks: allocation counts (`@allocated`) are deterministic and safe to |
| 107 | + run alongside other work; timing benchmarks need a quiet machine. Keep |
| 108 | + small ns-per-point A/B scripts for hot paths, and compare against the main |
| 109 | + branch (via a git worktree) whenever a perf regression is suspected — |
| 110 | + micro-sentinels do not catch inference-budget collapse; whole-model or |
| 111 | + whole-loop measurements do. |
| 112 | +- Machine performance drifts over hours (this session saw two stable levels |
| 113 | + about 8% apart), so never compare a fresh measurement against numbers |
| 114 | + recorded earlier: benchmark the candidate and the baseline INTERLEAVED in |
| 115 | + the same session (stash/unstash or worktrees), and rerun at least twice — |
| 116 | + a single elevated run right after heavy compilation is usually warmup. |
| 117 | +- For a hot-loop throughput regression, two cheap IR checks localize the |
| 118 | + cause faster than any profile: count vector instructions (`<N x double>` |
| 119 | + in `code_llvm`) and non-cold `invoke`s in `code_typed` of the loop. Two |
| 120 | + traps found this way: LLVM cannot vectorize across the carry branches of |
| 121 | + a flattened CartesianIndices iteration (`@simd` fixes it by splitting the |
| 122 | + inner dimension — but test with realistic inner trip counts, since ≤4 |
| 123 | + never vectorizes and hides the win), and Julia's inliner silently bails |
| 124 | + on closures over LARGE argument types (a flat broadcast inlines, a |
| 125 | + 3-op nested tree does not) — force it with a callsite `@inline f(...)`. |
| 126 | + Micro-benchmarks built from single-level expressions miss both. |
| 127 | +- A fix validated on one argument family is not validated: launch-path |
| 128 | + machinery here receives plain layouts, `Broadcasted` trees, and |
| 129 | + `FusedMultiBroadcast`s, and three consecutive CI rounds each failed on |
| 130 | + the family the previous fix was not tested with. Enumerate the argument |
| 131 | + families a changed function can receive and check every one. |
| 132 | +- Know the blind spots of a default test run: branches guarded by |
| 133 | + `num_threads > 1` or GPU-only defaults never execute in single-threaded |
| 134 | + CPU tests (two hard launch-time bugs on this PR hid exactly there). Run |
| 135 | + the relevant test files with `--threads=4` as well, and check device code |
| 136 | + paths with the compilation checker even when CPU tests are green. |
| 137 | + |
| 138 | +## 4. Isolate low-level issues and fix them robustly |
| 139 | + |
| 140 | +- Chase symptoms down to a single primitive with JET/`@test_compilation`, |
| 141 | + then fix the primitive, not the symptom. Examples from this PR: JET noise |
| 142 | + from `Threads.threadpoolsize`'s unreachable throw branch (fixed by calling |
| 143 | + the branch-free internal `Threads._nthreads_in_pool`), thread detection via |
| 144 | + a task-local-storage marker instead of fragile globals, and GPUArrays' |
| 145 | + uninferrable derived-`CuArray` views (fixed by one `stable_view` helper |
| 146 | + that replicates `Base.view`'s SubArray construction). |
| 147 | +- Using un-exported internals is acceptable when the fix is small, guarded |
| 148 | + (`@static if isdefined(...)` with a public-API fallback), and documented |
| 149 | + with the reason. |
| 150 | +- Rules that recur on GPUs: kernel-launched closures may capture only isbits |
| 151 | + values (derive types from argument types inside the closure, never capture |
| 152 | + a `Type`); error paths that build strings at runtime cannot compile in |
| 153 | + kernels (use static messages or move checks to the host); `Adapt` does not |
| 154 | + descend into `Base.Pair` or unregistered wrappers (write explicit |
| 155 | + `adapt_structure` rules); partial-rank views/reshapes of device arrays pull |
| 156 | + in `SignedMultiplicativeInverse` string-throwing constructors (index at |
| 157 | + full rank). |
| 158 | + |
| 159 | +## 5. Re-test hypotheses as the code evolves |
| 160 | + |
| 161 | +- A fix that was correct last week may be dead code today. After each |
| 162 | + significant change, re-run the checks that motivated earlier workarounds; |
| 163 | + delete workarounds whose cause is gone (this PR's `SubArray`/ |
| 164 | + `ReshapedArray` nesting machinery became removable once GPU slicing |
| 165 | + stopped reshaping entirely). |
| 166 | +- Keep several hypotheses alive when debugging: record them, design one |
| 167 | + experiment that discriminates between them (the CI crash-map probe, an |
| 168 | + A/B stash benchmark, a JET diff), and re-check the losers later — a wrong |
| 169 | + hypothesis about one failure may be right about another. |
| 170 | +- Fixes stacked while another regression is still present are all suspect: |
| 171 | + every intermediate measurement is contaminated by the unfixed problem, so |
| 172 | + effects get attributed to the wrong edit. When that has happened, redo the |
| 173 | + attribution in a clean room — construct a minimal baseline containing only |
| 174 | + the agreed fixes, measure it, then add each candidate edit one at a time |
| 175 | + (and afterwards remove each retained edit once) with benchmarks and tests |
| 176 | + after every step. On this PR, a clean-room pass revealed that two of six |
| 177 | + "necessary" fixes did nothing and one claimed "restoration" restored code |
| 178 | + that had never existed. |
| 179 | +- Never describe a change as restoring previous behavior without diffing the |
| 180 | + actual git history; memory of what the code used to look like is not |
| 181 | + evidence. |
| 182 | + |
| 183 | +## 6. Annotations conceal as often as they cure |
| 184 | + |
| 185 | +- `@generated`, `@assume_effects`, `@constprop`, `@inline`, and |
| 186 | + `recursion_relation` overrides each have one legitimate trigger. Before |
| 187 | + adding one, demonstrate the trigger (e.g. inference gives up ONLY under |
| 188 | + budget pressure); after adding one, re-test the original symptom — if it is |
| 189 | + still there, the annotation was hiding a structural problem (on this PR a |
| 190 | + `@constprop` on `getproperty` masked a value-domain chain that needed |
| 191 | + restructuring into `Val`-typed calls). |
| 192 | +- Prefer optionally-generated functions (`if @generated`) over plain |
| 193 | + `@generated` when the arguments may not be statically known: plain |
| 194 | + generated functions infer to `Any` for unknown static parameters. |
| 195 | +- `Base.@assume_effects :foldable` does not guarantee folding under inference |
| 196 | + budget pressure — verify with the kernel-IR check when GPU code depends on |
| 197 | + the fold. |
| 198 | + |
| 199 | +## 7. Downstream breakage from renames |
| 200 | + |
| 201 | +- When a PR renames or removes internal names that other packages use, add a |
| 202 | + `deprecated.jl` to the module with plain `const` aliases (no deprecation |
| 203 | + warnings), and **export** any alias whose old name was exported. Only alias |
| 204 | + names whose semantics survived; leaving a changed-meaning name undefined is |
| 205 | + better than silently resolving it to something subtly different. Find the |
| 206 | + full list by grepping the downstream repos AND the registered versions of |
| 207 | + satellite packages in the depot, then verify by running the smallest |
| 208 | + downstream test suite locally with the package `Pkg.develop`ed. |
0 commit comments