Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions design/IDEAS.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,15 @@ Each is one file plus a `@register` decorator — the registry was built for thi
promised, but every rule independently recomputes `dotted_name(node.func)` and
`final_attr(node.func)` on the same nodes. Caching those on the dispatcher, keyed by node
identity, should flatten most of the per-rule cost without touching any rule.
- **Run the documented pre-commit config, the way `action-smoke.yml` runs the documented
Action.** The `rev:` pins in the README and `docs/ci.md` had drifted to `v0.1.0` and `v0.2.0`
and nobody noticed, which is the third instance of one pattern: instructions we publish and
never execute. The first was `uses: highwaterlabs/torch-preflight@v0` resolving to nothing;
the second was `action.yml` missing the Node 20 pin bump that every workflow file got. A job
that installs pre-commit, runs the hook exactly as documented against a known-bad file and
asserts a non-zero exit would close the whole class rather than the instance. The pins would
still need bumping at release, but a stale one would fail loudly instead of silently.

- **A scoped-fact helper.** Five separate rules have now hit a bug where a file-level fact
leaked across functions — `prov.models`, `prov.criteria`, `uses_distributed`, TG008's "does
this file train", and TG001's deferred-backward exemption. Each was fixed the same way:
Expand Down
49 changes: 47 additions & 2 deletions design/TODO.md
Original file line number Diff line number Diff line change
Expand Up @@ -457,8 +457,16 @@ Per RFC [0001](rfcs/0001-vram-estimator.md). No new **required** dependencies.
volume — tutorials keep it simple deliberately. Note `litgpt` came back **137 files, 0
errors**, so careful code does scan clean.

- [ ] **File the tensor-parallelism PR.** Three one-line `zero_grad()` additions to
`pytorch/examples/distributed/tensor_parallelism/`.
- [x] **Filed the tensor-parallelism PR** — [pytorch/examples#1424](https://github.com/pytorch/examples/pull/1424).
Three one-line `zero_grad()` additions, placed after `optimizer.step()` to match the
sibling `distributed/FSDP2/example.py` rather than the `mnist` convention of putting it
before the forward. Re-verified against live `main` first, since the clone was a day old.
Deliberately left out a second finding in the same directory: `sequence_parallel_example.py`
has no `manual_seed` where its two siblings do, but its own comment says "input can be
different across all ranks", so it reads as intentional — and bundling it would have given
a reviewer something to argue about. One finding, one PR. The tool is credited in a single
line at the bottom rather than the top.
**The single upstream-reportable result from 318 findings across 1,615 files.**
- [x] **Decided and implemented RFC 0003** — [what severity means, and what should fail a build](rfcs/0003-severity-and-ci-gating.md).
Written because the TG001 split stopped the classic `losses.append(loss)` from failing
CI by default. The measurement says the split is right: on a 4-layer transformer the
Expand Down Expand Up @@ -502,6 +510,43 @@ Per RFC [0001](rfcs/0001-vram-estimator.md). No new **required** dependencies.
TG013 went 57 -> 14 -> 8 on this codebase, and the pipelining case above would have
been a rejected PR in `pytorch/pytorch` arguing against a design we had not understood.

- [x] **0.4.0 released**, and the Marketplace listing republished from it. The changelog leads
with a breaking-change block rather than burying it under "Changed", because the TG001
downgrade and TG004 becoming a note both mean a previously-red build goes green. Verified
from the published wheel rather than the working tree: fresh venv, 11 packages, no torch,
and the classic `losses.append(loss)` exiting 0 by default and 1 under `--fail-on warning`.
Also caught while bumping: the documented pre-commit `rev:` pins had drifted to `v0.1.0`
in `docs/ci.md` and `v0.2.0` in the README. Same class as the `@v0` tag that did not
resolve — instructions we publish but never execute. A test that runs the documented
pre-commit config the way `action-smoke.yml` runs the documented Action would close the
whole class; filed as an idea rather than done.

- [x] **The standard Accelerate evaluation loop no longer reports a TG001 error**
([#46](https://github.com/highwaterlabs/torch-preflight/pull/46)). Two causes:
`accelerator.backward(loss)` seeded `accelerator` itself as a live tensor, because the
collector seeds whatever `.backward()` is called on and Accelerate inverts that shape;
and a single `main()` binding `outputs` in both a training and an evaluation loop shared
one key, so the detached binding inherited grad-ness from its sibling. Bindings now carry
their enclosing loop identities. Python has function scope rather than block scope, so
this says "detached *within this loop*" rather than shadowing the name outright.

**The part worth remembering is the bug I put in on the way.** Detachment first
propagated whenever a binding was not *provably* grad-bearing — absence of proof treated
as evidence. That silenced `loss = compute_loss(...)` followed by `losses.append(loss)`,
and silenced it even when `loss.backward()` was called on the name.

Neither check caught it. All 437 tests passed, because every TG001 fixture assigns from
something resolvable like `criterion(model(batch), y)`. And the seven-repo scan reported
**zero new findings**, which is structurally blind here: a true positive that stops firing
is indistinguishable from a false positive that got fixed. I had reported 24 removals;
**14 were findings being silenced**, 8 of them genuine. The honest figure is 10.
*A wild scan is evidence about false positives only.* False negatives need fixtures that
deliberately exceed what the analysis can resolve, and there are now two.

- [x] **Merged branches deleted a third time**, and `v0` moved unattended again. Three releases
in, both are reflexes rather than decisions — the repo setting for auto-deleting head
branches would remove one of them permanently.

## Cross-cutting

- [x] Name and org settled: package `torch-preflight`, org `highwaterlabs`, deliberately
Expand Down