|
1 | 1 | # PR Review Checklist |
2 | 2 |
|
3 | 3 | This checklist covers areas that CI cannot check. Skip items related to linting, formatting, type checking, and import ordering. |
| 4 | + |
| 5 | +## Infrastructure |
| 6 | + |
| 7 | +### GitHub Actions Storage |
| 8 | + |
| 9 | +**Intent.** Artifacts and caches stored on GitHub are billed to the PyTorch |
| 10 | +org. ExecuTorch CI has a cheaper alternative: the `gha-artifacts` S3 bucket. |
| 11 | +This check catches PRs that add GitHub-hosted storage nobody needs, or that |
| 12 | +put large data on GitHub when S3 would work. It is about cost, not |
| 13 | +correctness. Required, bounded storage is not a finding. |
| 14 | + |
| 15 | +#### What counts as GitHub-hosted storage |
| 16 | + |
| 17 | +In scope: |
| 18 | +- `actions/upload-artifact` steps. |
| 19 | +- The `upload-artifact: <name>` input to the pytorch/test-infra reusable |
| 20 | + workflows (`linux_job_v2.yml`, `macos_job.yml`, `windows_job.yml`) **without** |
| 21 | + `upload-artifact-to-s3: true`. This uploads everything the job writes to |
| 22 | + `${RUNNER_ARTIFACT_DIR}` (plus `dist/*.whl` and `artifacts-to-be-uploaded/`) |
| 23 | + to GitHub, with the repository's default retention. |
| 24 | +- `actions/cache` steps and the `cache:` option of `actions/setup-*` actions. |
| 25 | + |
| 26 | +Out of scope, so never flag these: |
| 27 | +- S3 uploads: `upload-artifact-to-s3: true`, `seemethere/upload-artifact-s3`, |
| 28 | + test-infra's `upload-artifact-s3` action, or `aws s3 cp` to `gha-artifacts`. |
| 29 | +- Upload steps whose stored contents the PR does not change. |
| 30 | + |
| 31 | +#### Constraints to know before suggesting S3 |
| 32 | + |
| 33 | +- S3 uploads only work on PyTorch's AWS runners (e.g. `linux.*` and |
| 34 | + `macos-*` runners used through test-infra). Jobs with `runs-on: |
| 35 | + ubuntu-latest` or other GitHub-hosted runners cannot use S3. Do not |
| 36 | + suggest S3 for those jobs. |
| 37 | +- test-infra's `download-artifact:` input and `actions/download-artifact` |
| 38 | + only read GitHub storage. If a downstream job reads the artifact that way, |
| 39 | + switching the upload to S3 also means changing the consumer to fetch |
| 40 | + `https://gha-artifacts.s3.amazonaws.com/${{ github.repository }}/${{ github.run_id }}/artifacts/<file>`. |
| 41 | + `android-release-artifacts.yml` follows this pattern. Include the consumer |
| 42 | + change in your suggestion. |
| 43 | +- The S3 bucket is publicly readable, so never suggest it for anything |
| 44 | + sensitive. |
| 45 | + |
| 46 | +#### What to flag |
| 47 | + |
| 48 | +For each in-scope line the PR adds or changes: |
| 49 | + |
| 50 | +- [ ] **No consumer** - Search `.github/` for consumers of the artifact name: |
| 51 | + `download-artifact: <name>`, `actions/download-artifact` with `name: <name>`, |
| 52 | + or a matching `pattern:`. If nothing in the repo consumes it and the PR |
| 53 | + description gives no use for it, ask whether it is needed. If it is only for |
| 54 | + occasional manual debugging, suggest `upload-artifact-to-s3: true` (on AWS |
| 55 | + runners) or dropping it. |
| 56 | +- [ ] **Large payload on GitHub** - Models (`.pte`, `.ptd`, `.bin`, |
| 57 | + `.safetensors`), checkpoints, wheels, app bundles (`.apk`, `.aar`, `.ipa`, |
| 58 | + `.app`), and whole build trees should go to S3 unless a GitHub-only consumer |
| 59 | + needs them. As a rough threshold, anything over about 100 MB per upload |
| 60 | + deserves this question. |
| 61 | +- [ ] **Path broader than the consumer needs** - For example, the job copies |
| 62 | + all of `cmake-out/`, a build directory, or a model cache into |
| 63 | + `${RUNNER_ARTIFACT_DIR}`, but the consumer only uses one binary or report. |
| 64 | + Name the files the consumer actually reads. |
| 65 | +- [ ] **Fan-out and duplication** - Look for an upload inside a `matrix:` whose |
| 66 | + payload does not vary with the matrix value, multiple jobs uploading the same |
| 67 | + files, or a new upload on high-frequency triggers (`pull_request`, `push` to |
| 68 | + `main`, short `schedule:` crons) where `workflow_dispatch` or a nightly run |
| 69 | + would do. |
| 70 | +- [ ] **Retention** - An explicit `retention-days` longer than the consumer |
| 71 | + needs. An artifact consumed by a later job in the same run needs 1 day. |
| 72 | +- [ ] **Cache churn** - `actions/cache` keys that include `github.sha`, |
| 73 | + `github.run_id`, or timestamps, so each entry is written once and never |
| 74 | + restored. Also flag large caches on jobs that rarely run. |
| 75 | +- [ ] **Indirect growth** - Non-workflow changes, such as a script or test that |
| 76 | + starts writing models, logs, or build outputs into `${RUNNER_ARTIFACT_DIR}`, |
| 77 | + `artifacts-to-be-uploaded/`, or `dist/`. These grow an existing, |
| 78 | + unchanged GitHub upload. |
| 79 | + |
| 80 | +#### When a finding is confirmed |
| 81 | + |
| 82 | +A storage finding is **confirmed** only when all of these hold: |
| 83 | +1. The responsible line is in this PR's diff. |
| 84 | +2. You searched `.github/` for consumers and found none, or found one that |
| 85 | + needs only part of the payload. |
| 86 | +3. The PR description, nearby comments, and the workflow give no stated use |
| 87 | + (release, HUD, debugging, another repo). |
| 88 | +4. A cheaper alternative exists under the constraints above. |
| 89 | + |
| 90 | +If any condition is unclear, for example because the consumer might be |
| 91 | +outside this repo, write the finding as a **question to the author** under |
| 92 | +Infrastructure. Do not count it as confirmed, and do not request changes for |
| 93 | +it alone. Not being able to find a consumer is not proof that none exists. |
| 94 | + |
| 95 | +Confirmed waste is a must-fix Infrastructure finding and requires |
| 96 | +**Request Changes**. Write it up as follows: |
| 97 | +- cite the line; |
| 98 | +- say what is stored; |
| 99 | +- estimate the footprint (size per upload × matrix or job fan-out × run |
| 100 | + frequency × retention), stating your assumptions; |
| 101 | +- give the concrete fix: set `upload-artifact-to-s3: true` (plus the consumer |
| 102 | + change if one is needed), narrow the path, deduplicate, shorten retention, |
| 103 | + or remove the upload. |
| 104 | + |
| 105 | +If storage is necessary and bounded, do not mention this check in the review. |
0 commit comments