The DataLoader PR was functionally done — every review comment addressed, tests green — and my mentor's verdict was that it still read as boilerplate-y. He pointed me at ponytail, a review skill whose only job is to hunt over-engineering, and asked whether we'd end up with something leaner. The result: #698 went from 1361 added lines to 1045 — 23% gone — with the source file down from 608 lines to 494. Deleting was the easy half. The half worth writing down is how you convince yourself, and a reviewer, that a deletion this size broke nothing.
What got cut, what got refused
What survived verification was mostly repetition: the Parquet schema validation written
out twice, verbatim; a normalizer returning a 3-tuple whose second and third elements
were one-line derivations of the first; 53 tests without a single
parametrize in a repo whose sibling suites lean on it; six tests hand-rolling
a warnings filter the repo already applies globally. My favorite cut answered a review
question by deletion: Rob had asked whether torch.DataLoader has anything
like the loader's two progress counters. It does not, and nothing here read them either
— and with the counters gone, the loader's second iteration path had no reason to
exist, so the whole accounting layer folded into __iter__. Coverage went
up, because the sanity check it carried now runs on every pass.
The other half of the job is saying no. Error handling stayed: this is out-of-core data loading, and the boundary checks are the feature, not ceremony. The license headers stayed, because the maintainer had asked for them two days earlier. And one test that looked perfectly redundant — zero unique line coverage — stayed because mutation testing showed it was the only test that catches a real regression class in the scalar-sample path. Coverage can tell you a test is redundant on lines; only breaking the code on purpose tells you whether it is redundant on behavior.
Three proofs, none of them "trust me"
First, a differential test: load the old and new modules side by side and feed them
identical sources — ten configurations, three shuffle seeds, the error paths too.
Every batch came out byte-identical. Second, replay: run the old 53-test suite
against the new code — 46 passed, and the 7 failures were precisely the tests of
the deliberately deleted counter API, nothing else. Third, mutation testing: break each
surviving guard by hand — disable the size check, accept a negative
total_size, skip the per-shard schema validation — and confirm the
slimmed suite still fails loudly. Six mutants, six kills.
One end-to-end check did scare me: streamed ADVI in a hand-rolled stepping harness
wandered off the posterior. A control experiment cleared the deletions — the
pre-trim code and even a no-loader arm wandered identically, and through the supported
pm.fit path the trimmed loader recovers the true posterior, landing within
Monte-Carlo noise of pm.Minibatch. The artifact lives in my harness, not
the PR. When an end-to-end test fails after a refactor, the first question is not
"what did I break" but "which component actually differs between the passing and the
failing arms."
Then I pointed the tool at my own drafts
The monitor and streaming Pathfinder from weeks 6–8 are about to go out as draft
PRs, and Rob asked to see them early, with a warning attached: maybe some of that
complexity doesn't make sense. So they got the same review before he sees them —
and reviewing unshipped code turns out to hunt a different animal: not repetition,
but speculation. A diagnostics recorder only its own tests read; a reset()
whose docstring imagined a caller that exists nowhere; an arming flag a 12-case probe
showed was provably dead. The monitor went from 268 lines to 149 with its calibration
untouched.
It also repeated week 3's naming lesson back to me. StreamingConvergenceMonitor
is now CheckLossConvergence, a proper sibling of PyMC's
CheckParametersConvergence; my PatienceStopping is deleted
outright, because the entire ecosystem calls that idea EarlyStopping and
inventing a third name helps nobody. Two deletions I vetoed: the line-search failure
counter stays paired with the curvature-violation rate, so a stuck optimizer cannot
report a clean 0%; and a steepest-descent fallback keeps its guard even though it lost
its counter. Both draft descriptions now end with questions rather than claims —
the complexity decisions are cheaper made together than defended after the fact.
Links. DataLoader PR #698 · ponytail · The CUSUM monitor (week 7) · The streaming Pathfinder (week 8) · GSoC project page · Mentors @fonnesbeck · @zaxtax