-
Notifications
You must be signed in to change notification settings - Fork 1
ci: lint gates (clippy, ruff, thaw-lint) + contributor rails #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <!-- Keep the diff scoped to one concern. See CONTRIBUTING.md. --> | ||
|
|
||
| ## What & why | ||
|
|
||
| <!-- One or two sentences: what changes, and the problem it solves. --> | ||
|
|
||
| ## How it was verified | ||
|
|
||
| <!-- Commands you ran, or the pod/receipt for a GPU/perf change. --> | ||
|
|
||
| ## Checklist | ||
|
|
||
| - [ ] `python tools/thaw_lint.py` is clean | ||
| - [ ] `ruff check --select F,E9` passes on the Python I touched | ||
| - [ ] `cargo clippy --workspace --exclude thaw-py --all-targets -- -D warnings` is clean (if Rust changed) | ||
| - [ ] Tests pass (`pytest tests/ -q` and/or `cargo test`), and new behavior has a test | ||
| - [ ] Diff is scoped — no unrelated files | ||
| - [ ] **Perf claims ship their receipt** (`site/receipts/*.json` from the run), state what was actually measured, and use a re-validated number — no retracted multipliers, no local paths in receipts |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,69 @@ permissions: | |
| contents: read | ||
|
|
||
| jobs: | ||
| lint: | ||
| name: lint (clippy, ruff, thaw-lint) | ||
| runs-on: ubuntu-22.04 | ||
| timeout-minutes: 15 | ||
| steps: | ||
| # fetch-depth: 0 so the changed-files ruff step can diff against the | ||
| # PR base (or the previous commit on a push). | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' | ||
| cache: pip | ||
|
|
||
| - name: Install ruff | ||
| run: pip install ruff | ||
|
|
||
|
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check for unpinned ruff installs across workflows/docs/scripts
rg -n "pip install .*ruff(\\s|$)" .github/workflows CONTRIBUTING.md .github/pull_request_template.mdRepository: thaw-ai/thaw Length of output: 210 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== .github/workflows/test.yml (around install step) ==\n'
cat -n .github/workflows/test.yml | sed -n '1,120p'
printf '\n== ruff references in repo ==\n'
rg -n --hidden --glob '!.git' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '\bruff\b|ruff==' .Repository: thaw-ai/thaw Length of output: 6580 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== .github/workflows/test.yml (around install step) ==\n'
cat -n .github/workflows/test.yml | sed -n '1,120p'
printf '\n== ruff references in repo ==\n'
rg -n --hidden --glob '!.git' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '\bruff\b|ruff==' .Repository: thaw-ai/thaw Length of output: 6580 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n== .github/workflows/test.yml (around install step) ==\n'
cat -n .github/workflows/test.yml | sed -n '1,120p'
printf '\n== ruff references in repo ==\n'
rg -n --hidden --glob '!.git' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '\bruff\b|ruff==' .Repository: thaw-ai/thaw Length of output: 6580 Pin Ruff in 🤖 Prompt for AI Agents |
||
| # ruff runs on CHANGED Python only. The legacy tree has known debt | ||
| # (tracked in the ratchet plan in CONTRIBUTING.md); gating the whole | ||
| # tree on it would block every PR. Scoping to the diff means new and | ||
| # touched code must be clean without forcing a one-shot tree cleanup | ||
| # that would collide with in-flight PRs. --select F,E9 = real bugs | ||
| # (undefined names, unused imports, syntax), not style. | ||
| - name: ruff (real-bug rules) on changed Python | ||
| run: | | ||
| set -euo pipefail | ||
| BASE="${{ github.event.pull_request.base.sha }}" | ||
| if [ -z "$BASE" ]; then BASE="${{ github.event.before }}"; fi | ||
| if ! git rev-parse --verify "$BASE" >/dev/null 2>&1; then | ||
| BASE="$(git rev-parse HEAD^ 2>/dev/null || true)" | ||
| fi | ||
| if [ -n "$BASE" ]; then | ||
| FILES="$(git diff --name-only --diff-filter=ACMR "$BASE" HEAD -- '*.py' || true)" | ||
| else | ||
| FILES="$(git ls-files '*.py')" | ||
| fi | ||
| if [ -z "$FILES" ]; then | ||
| echo "No changed Python files — skipping ruff." | ||
| exit 0 | ||
| fi | ||
| echo "Linting changed Python files:" | ||
| echo "$FILES" | ||
| echo "$FILES" | xargs ruff check --select F,E9 | ||
|
|
||
| # thaw-lint runs on the WHOLE tree: every rule is a regression guard on | ||
| # something currently clean (or a non-blocking WARN), so it stays green. | ||
| - name: thaw-lint (project footguns) | ||
| run: python tools/thaw_lint.py | ||
|
|
||
| - uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| components: clippy | ||
|
|
||
| - uses: Swatinem/rust-cache@v2 | ||
|
|
||
| # clippy runs tree-wide with -D warnings: the workspace is clean today, | ||
| # so this is a regression guard. thaw-py is excluded (it needs the | ||
| # PyO3/CUDA surface that only builds in the wheel workflow). | ||
| - name: cargo clippy (deny warnings) | ||
| run: cargo clippy --workspace --exclude thaw-py --all-targets -- -D warnings | ||
|
|
||
| rust: | ||
| name: cargo test (${{ matrix.os }}) | ||
| runs-on: ${{ matrix.os }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,126 @@ | ||||||||||
| # Contributing to thaw | ||||||||||
|
|
||||||||||
| thaw is a small team moving fast. These are the rails that keep PRs flowing | ||||||||||
| without regressions or hours lost to footguns we've already paid for once. | ||||||||||
|
|
||||||||||
| ## The loop | ||||||||||
|
|
||||||||||
| 1. Branch off `main` (`git checkout -b area/short-description`). | ||||||||||
| 2. Make the change. Keep the diff scoped — one concern per PR. | ||||||||||
| 3. Run the checks below locally. | ||||||||||
| 4. Open a PR. CI runs `lint`, `cargo test`, and `pytest`. Fill in the template. | ||||||||||
| 5. Get a review. Address blocking + high items; land it. | ||||||||||
|
|
||||||||||
| Never push to `main` directly, and never tag a release without a GPU | ||||||||||
| validation pass — see [Release](#release). | ||||||||||
|
|
||||||||||
| ## Checks before you push | ||||||||||
|
|
||||||||||
| ```bash | ||||||||||
| # Project footgun linter (whole tree, fast, no deps) | ||||||||||
| python tools/thaw_lint.py | ||||||||||
|
|
||||||||||
| # Real-bug lint on the Python you touched | ||||||||||
| ruff check --select F,E9 <your changed .py files> | ||||||||||
|
|
||||||||||
| # Rust: format + lint + tests | ||||||||||
| cargo fmt --all | ||||||||||
| cargo clippy --workspace --exclude thaw-py --all-targets -- -D warnings | ||||||||||
| cargo test --workspace --exclude thaw-py | ||||||||||
|
|
||||||||||
| # Python tests (torch/vllm/sglang are mocked — no GPU needed) | ||||||||||
| pytest tests/ -q | ||||||||||
| ``` | ||||||||||
|
|
||||||||||
| ### What CI gates | ||||||||||
|
|
||||||||||
| | Gate | Scope | Why scoped this way | | ||||||||||
| |------|-------|---------------------| | ||||||||||
| | `cargo clippy -D warnings` | whole workspace (minus `thaw-py`) | clean today — a regression guard | | ||||||||||
| | `python tools/thaw_lint.py` | whole tree | every ERROR rule is currently green | | ||||||||||
| | `ruff check --select F,E9` | **changed Python only** | the legacy tree has known debt; see the ratchet below | | ||||||||||
|
|
||||||||||
| **The ruff ratchet.** We do not gate the whole tree on ruff yet — there are | ||||||||||
| ~36 pre-existing real-bug findings (mostly unused imports) that a one-shot | ||||||||||
| cleanup would land right on top of in-flight PRs. So ruff runs only on the | ||||||||||
| files your PR changes: new and touched code must be clean, legacy debt does | ||||||||||
| not block you. Once the open PRs land, a dedicated `ruff --fix` sweep will | ||||||||||
| clear the backlog and the gate widens to the whole tree. If you're already in | ||||||||||
| a legacy file, fixing its findings while you're there is welcome. | ||||||||||
|
|
||||||||||
| `cargo fmt --check` is **not** gated yet (a full `cargo fmt` touches ~16 files | ||||||||||
| and would conflict with open Rust PRs). Run `cargo fmt --all` before pushing | ||||||||||
| anyway; we'll turn the gate on once the tree is normalized. | ||||||||||
|
|
||||||||||
| ## thaw-lint — the project-specific rules | ||||||||||
|
|
||||||||||
| `tools/thaw_lint.py` encodes the mistakes that have actually cost us time. | ||||||||||
| `ERROR` fails CI; `WARN` just surfaces. Run `python tools/thaw_lint.py --list` | ||||||||||
| to see them all. Current ERROR rules: | ||||||||||
|
|
||||||||||
| - **`import thaw_native`** → the `thaw-native` wheel installs the module | ||||||||||
| `thaw`. Write `import thaw`. (This one cost real fresh-pod debugging hours.) | ||||||||||
| - **Personal `@icloud.com` email** in tracked files → the public contact is | ||||||||||
| `nils@thaw.sh`. | ||||||||||
| - **`matteso1/thaw` URLs** → the repo lives at `thaw-ai/thaw`; the old one is | ||||||||||
| archived and 404s clone scripts. | ||||||||||
|
Comment on lines
+65
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This exact line is tripping CI ( Suggested fix-- **`matteso1/thaw` URLs** → the repo lives at `thaw-ai/thaw`; the old one is
+- **Archived repo URLs** (old owner/repo slug) → the repo lives at `thaw-ai/thaw`; the old one is
archived and 404s clone scripts.📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: test / 0_lint (clippy, ruff, thaw-lint).txt[error] 65-65: thaw-lint (no-archived-repo-url): Point at github.com/thaw-ai/thaw — matteso1/thaw is archived. 'matteso1/thaw' URLs should be updated to 'thaw-ai/thaw'. 🪛 GitHub Actions: test / lint (clippy, ruff, thaw-lint)[error] 65-65: thaw-lint failed ([no-archived-repo-url]). Point at github.com/thaw-ai/thaw — matteso1/thaw is archived. 🤖 Prompt for AI AgentsSource: Pipeline failures |
||||||||||
| - **Leftover `breakpoint()` / `pdb.set_trace()`** in shipped Python. | ||||||||||
| - **Retracted speedup multipliers** (`17.2x`, `12.6x`, `9.7x`, `5.9x`) in the | ||||||||||
| README or `site/` — see [receipt hygiene](#receipt--benchmark-hygiene). | ||||||||||
| - **Hard-setting `VLLM_ALLOW_INSECURE_SERIALIZATION`** → use | ||||||||||
| `os.environ.setdefault` so a user can override a security-sensitive flag. | ||||||||||
|
|
||||||||||
| Sure a line is a false positive? Add `# thaw-lint: allow` to it. New footgun | ||||||||||
| worth guarding? Add a `Rule` to `tools/thaw_lint.py` with a test — keep ERROR | ||||||||||
| rules ones the tree is currently clean on, so they're regression guards, not a | ||||||||||
| backlog. | ||||||||||
|
|
||||||||||
| ## Receipt & benchmark hygiene | ||||||||||
|
|
||||||||||
| Performance is the pitch, so the bar on numbers is high. | ||||||||||
|
|
||||||||||
| - **Only claim re-validated numbers.** Default to the most conservative | ||||||||||
| defensible figure, not the most flattering one. | ||||||||||
| - **A perf claim ships with its receipt** — a JSON under `site/receipts/` from | ||||||||||
| the run that produced it, in the same PR. Prose-only numbers rot. | ||||||||||
| - **Say what the benchmark actually measured.** If it injects synthetic | ||||||||||
| latency, it measured latency-hiding, not bandwidth — don't let the README | ||||||||||
| round that up to a throughput claim. | ||||||||||
| - **Scrub local paths** (`C:\Users\...`, `/home/you/...`) out of committed | ||||||||||
| receipts. | ||||||||||
| - Retired numbers live in `docs/ARCHITECTURE.md` so they don't resurface. | ||||||||||
|
|
||||||||||
| ## GPU vs. no-GPU | ||||||||||
|
|
||||||||||
| A lot of thaw runs without a GPU — the whole inspect/diff/log/rewind surface, | ||||||||||
| the format readers, the Rust core against `MockCuda`. Those are unit-tested in | ||||||||||
| CI. The GPU paths (real freeze/restore/fork, TP, KV cache) are validated on a | ||||||||||
| pod per release; `tests/gpu/` and `benchmarks/` hold those. Don't put a | ||||||||||
| GPU-requiring test in the default `pytest` path — mock the engine | ||||||||||
| (`tests/conftest.py` shows how) or gate it under `tests/gpu/`. | ||||||||||
|
|
||||||||||
| ## Ownership boundary (format vs. transport) | ||||||||||
|
|
||||||||||
| To keep two people out of each other's way, work splits at the file format | ||||||||||
| line: | ||||||||||
|
|
||||||||||
| - **Above the line — format, semantics, the verb surface.** The `.thaw` / | ||||||||||
| `.thawkv` schema, `handle.json`, cross-engine restore, `verify`, lineage, | ||||||||||
| the fork API contract. Changes here define *what the bytes mean*. | ||||||||||
| - **Below the line — byte transport & throughput.** Restore DMA, freeze | ||||||||||
| pipelining, multi-NVMe/TP write paths, parallel cloud fetch. Changes here | ||||||||||
| move *the same bytes faster*. | ||||||||||
|
|
||||||||||
| `python/thaw_common/cloud.py` is the one file both sides touch — partition it: | ||||||||||
| manifest parse/resolve above the line, the transfer executor below it, with a | ||||||||||
| typed boundary between. When a change spans the line (e.g. shard-at-freeze), | ||||||||||
| freeze the on-disk schema + golden fixtures **first**, then build transport | ||||||||||
| against bytes-on-disk rather than against in-flight code. | ||||||||||
|
|
||||||||||
| ## Release | ||||||||||
|
|
||||||||||
| The `thaw-native` version string lives in **two** independent places that must | ||||||||||
| match: `Cargo.toml` `[workspace.package]` and **`crates/thaw-py/pyproject.toml`** | ||||||||||
| (the authoritative file maturin reads — the other crate `Cargo.toml`s inherit | ||||||||||
| via `version.workspace = true`). Desyncing them has failed a release before. | ||||||||||
| Bump both, validate on a GPU pod, then tag. | ||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |||||
| # Usage on a fresh RunPod/Lambda pod: | ||||||
| # export HF_TOKEN=hf_xxxxx | ||||||
| # export GITHUB_PAT=ghp_xxxxx | ||||||
| # curl -sSL https://raw.githubusercontent.com/matteso1/thaw/main/demos/run_quick_test.sh | bash -s -- --setup | ||||||
| # curl -sSL https://raw.githubusercontent.com/thaw-ai/thaw/main/demos/run_quick_test.sh | bash -s -- --setup | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win thaw-lint flags this curl-pipe-bash bootstrap line. The new 🔧 Add inline suppression-# curl -sSL https://raw.githubusercontent.com/thaw-ai/thaw/main/demos/run_quick_test.sh | bash -s -- --setup
+# curl -sSL https://raw.githubusercontent.com/thaw-ai/thaw/main/demos/run_quick_test.sh | bash -s -- --setup # thaw-lint: allow📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: test / 0_lint (clippy, ruff, thaw-lint).txt[warning] 13-13: thaw-lint: [no-curl-pipe-bash] Prefer 🪛 GitHub Actions: test / lint (clippy, ruff, thaw-lint)[warning] 13-13: thaw-lint warning ([no-curl-pipe-bash]): Prefer 🤖 Prompt for AI AgentsSource: Pipeline failures |
||||||
|
|
||||||
| set -euo pipefail | ||||||
|
|
||||||
|
|
@@ -37,7 +37,7 @@ if $SETUP; then | |||||
| # Clone repo | ||||||
| if [ ! -d /workspace/thaw ]; then | ||||||
| if [ -n "${GITHUB_PAT:-}" ]; then | ||||||
| git clone "https://${GITHUB_PAT}@github.com/matteso1/thaw.git" /workspace/thaw | ||||||
| git clone "https://${GITHUB_PAT}@github.com/thaw-ai/thaw.git" /workspace/thaw | ||||||
| else | ||||||
| git clone https://github.com/thaw-ai/thaw.git /workspace/thaw | ||||||
| fi | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
Repository: thaw-ai/thaw
Length of output: 1642
Pin all workflow actions to commit SHAs.
actions/checkout@v4,actions/setup-python@v5,dtolnay/rust-toolchain@stable, andSwatinem/rust-cache@v2are all still floating refs in this workflow; the same pattern also appears at lines 64-68, 90-94, and 111-113.🧰 Tools
🪛 zizmor (1.26.1)
[warning] 20-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Source: Linters/SAST tools