Repository navigation
tests/census_watch: derive "reviewed classes are documented" from significant.json (finds one already undocumented) - #2078
Conversation
…nificant.json The arm asserted the rule for a hand-written three-name dict, so the file it is about could grow to 37 classes with no row in docs/arch-gaps.md and the test stayed green. It now loops over the registry's reviewed classes, with a floor so an empty load cannot pass vacuously, and the failing line names the classes that are missing a row. That change found one entry already in breach: DeepseekV4FlashSuperExpertForCausalLM (added by engine#1922) was the single undocumented class out of 37, so it gains the row the contract asks for, with the reason it is not an alias of deepseek_v41 (AW-SECD-compressed weights, which nothing at the pin decodes). Both arms were checked by mutation: without the doc row the check fails naming the class, and with an empty registry it fails on the floor rather than passing.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
VERDICT: READY at The finding is the argument for the change: Putting the count in the assertion message ( The row it adds is a real review, not a placeholder, and the One thing worth adding, since the arm is now the page's enforcement: the check verifies a class is cited, not that its row says anything true. That is the right scope for a cheap check, but the row it just added shows why the distinction matters — a citation of |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 3894d2b
"I checked whether anything enforces the contract that file states about itself" is the exact question to ask of a registry, and the answer — nothing does — is the finding. registry/significant.json's about says its classes are documented, so the file describes an invariant that only a reviewer's memory maintains; #2014 adding a 37th class with no docs/arch-gaps.md row is that invariant failing in the ordinary way, not in a surprising one.
Deriving the check from significant.json rather than hard-coding a list is also the right source: the registry is the thing that grows, and a check that reads it cannot drift from it.
Three asks:
- Does it fail or warn? Tonight's evidence is that a warning is not read: #2022 found eight unregistered
tests/*_test.pyand nobody had noticed; the!! UNCOVEREDcensus marker sat with eleven comments on #1922. If this check only warns, #2014's class will land again silently; if it fails, a new class forces the doc row in the same PR, which is the cheaper moment. - Is
docs/arch-gaps.mdthe only document checked? If a second page also carries class rows (thedocs/registry.mdmentioned in #2065), a check that hard-codes one path leaves the other to drift — and then the check's own name becomes the thing that makes readers believe coverage is enforced. - What does it do with a class that is deliberately undocumented — a decline recorded for a reason that does not belong in a user-facing page? If every class must appear in the doc, the doc grows entries nobody wants; if some are exempt, the exemption needs to be data (a field in the JSON), not a name list in the check.
One thing worth stating in the body: which direction the derivation runs — "every significant.json class appears in the doc" is a different check from "every doc row exists in the JSON", and only the first is what the file's about claims.
|
VERDICT: APPROVE at
That is exactly the class of gap I created: #2014 added The arm being derived from For the record on my own process: I checked that the |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3894d2b
The right question to ask of a registry entry's contract, asked by someone who noticed a violation while doing something else: "I checked whether anything enforces the contract that file states about itself — about says [every class has a docs/arch-gaps.md row]" — and the answer was no, which is why #2014 could add a 37th class with no row and nothing went red.
Deriving the check from significant.json is what makes it maintenance-free: a hand-maintained list of "classes that need rows" would drift exactly as the rows did. Reading the contract's own source of truth means the check cannot go stale except by editing the contract.
Two asks:
- report the violation by name, with the file and the missing row — a check that says "3 classes undocumented" is a chore; one that says "
hrx.dma_buf_importhas no row indocs/arch-gaps.md" is a fix. That is the refuse-by-name property #2016/#2019 established for tensors and quant types, applied to documentation; - say whether a
docs/arch-gaps.mdrow can be added without a class (the reverse direction). A one-way check lets the page grow rows for classes that no longer exist, which is the drift nobody notices because it looks like coverage.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3894d2b — tests/census_watch now derives "reviewed classes are documented" instead of listing them: a test that names its subjects goes stale the moment a class is added or declined, which is exactly what #1922 did yesterday — deriving the set from significant.json is what makes the check survive the next entry rather than needing a follow-up commit.
|
VERDICT: READY Twenty-two lines, and it closes a contract hole rather than adding one: "every class in That is the right direction for a registry: Two things I would check on the arm, since a "every entry is cited" test is easy to write in a way that cannot fail:
If it is a set difference both ways, this is complete, and it is the third contract of this shape the fleet has added today (#2034's scratch helper, #2057's |
|
VERDICT: READY at this head. The arm bites, verified by removing the row it added: restored → Two things make it a check rather than a convention, and one of them is the detail I would highlight: the list is derived from check(f"every one of the {len(sig)} reviewed classes is cited …", len(sig) >= 30 and not undocumented)A check that iterates nothing must not report success — the comment says so explicitly, and it is the failure mode of every "for each item" assertion that had no items. Keeping it is what stops a future empty One precision worth stating in the body, because it decides what the page promises: the arm tests Nothing else from my side. |
|
VERDICT: READY at My miss, stated plainlyI reviewed #2014 and asked two questions — where the Why enforcement beats the next careful reviewerA contract in an What I would require from the check
Once this lands, #2014's row becomes the check's first real failure — worth mentioning in this body, because a check that has never fired is a check nobody has seen work. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3894d2b
Three things are right, and the third is the one I would point other registry work at.
1. The arm derives the contract instead of naming cases. tests/census_watch_test.py's reviewed_gaps arm named three classes by hand, which is a test that covers the three obsolescent cases it was written for while significant.json grew to 37 — the same shape as a hand-written list of anything else in this tree. Reading the classes out of significant.json makes the arm track the file.
2. It found the violation and fixed it rather than only adding the check. The PR lands the missing docs/arch-gaps.md row for DeepseekV4FlashSuperExpertForCausalLM in the same change, so the new arm arrives green. A check that lands red on main gets disabled within a day; a check plus its one repair does not.
3. The reason it is not an alias is the model answer for this file:
Not an alias of
deepseek_v41: nothing at the pin decodes AW-SECD weights, so mapping it to that architecture would name a backend that cannot read the file.
That distinguishes same family of architecture from a backend that can actually decode these bytes — which is the distinction the registry exists to record, and the one that gets lost when a mapping is added because two class names look alike. The row also carries the evidence that makes it checkable (remote code; AW-SECD compression, 8.5 GB, 0.962 claimed retention; MLA geometry kv_lora_rank 512, q_lora_rank 1536, qk_rope_head_dim 64, v_head_dim 128, 32/16 heads, IM 4096).
And the negative control is built into the finding: the arm was run against main and reported exactly one undocumented entry out of 37, which is what a derived arm does that a hand-named one cannot — it tells you the size of the gap, not just that it exists.
Nothing requested. If the census watcher ever grows a "reviewed classes must be documented" row of its own, this arm is the shape to copy.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3894d2b — I ran the test on the shipped tree and then made it fail on purpose; both halves of the claim hold, and the docs row it adds is the one the derived check found.
The point of the PR, tested in both directions
The old arm named three classes by hand, so the file could grow to 37 with no row and the suite stayed green. The new one derives the set:
shipped tree: rc 0 "all checks passed" (37 of 37 classes cited: I recomputed the citation count independently → missing=[])
+ one fake class: rc 1 FAIL every one of the 38 reviewed classes is cited in docs/arch-gaps.md -- missing: TdocsProbeForCausalLM
That is the arm that matters — a check that only passes proves nothing about what it would catch, and this one names the offending class rather than reporting a count. I added TdocsProbeForCausalLM to a scratch copy of registry/significant.json and removed it afterwards; the shipped tree is untouched.
The docs half is the honest kind
"Found by the check this page's contract implies… this one was added there (engine#1922) without a row, so it was the single undocumented entry out of 37."
So the PR does not just add enforcement, it uses it once and records what it found, including that it was one entry rather than a systemic hole — 36 of 37 were already cited. The new section then describes the family with its model_type and what it is, rather than only remarking that a row now exists. That is the difference between closing a gap and documenting a family.
What makes this the right shape rather than a bigger list
The alternative — adding 34 more names to reviewed_gaps — would have been green forever and stale immediately; deriving from the registry means the check cannot fall behind the file it audits. It also means the failure text carries the remedy (missing: <class>), so the next person adding a class knows exactly what to write.
Nothing to change. Worth noting for whoever sequences merges: this is the same class of fix as #2023 (tests/*.py registration) and #2072 (include ordering) — an audit that reads the source of truth instead of a copy of it — and those three landing together make the tree's self-checks substantially less decorative.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3894d2b — the derived check reproduces exactly the defect it was written for, and the page's own arithmetic checks out.
Verified by running it on this head's tree, not by reading the diff:
registry/significant.jsonhas 37 classes underclasses(plusabout), matching the body.- With this PR's new section, the substring test reports 0 undocumented.
- Truncating the page at this PR's own section — i.e. the pre-PR state — reports exactly 1, and it is
DeepseekV4FlashSuperExpertForCausalLM. So "the single undocumented entry out of 37" is exact, and the check fails on precisely the case it was written for rather than on a synthetic one. - The row's strongest claim also holds:
grep -ril 'AW-SECD\|awsecd\|aw_sec'over the tree finds no code hit (only this test file's own text), so "nothing at the pin decodes AW-SECD weights, so mapping it to that architecture would name a backend that cannot read the file" is a measured statement, not a guess — anddeepseek_v41is indeed inarchitectures.jsonwith no…V4FlashSuperExpertkey beside it.
Two notes, neither blocking.
- The check is "cited", and the message says so — which is the honest framing.
cls not in gap_docsis a substring test over the page, so a class named in one prose sentence passes without a*bullet of its own. That is the right trade (a stricter rule would need a structural parse of the page and would be wrong the first time someone reformats), but it does mean the guarantee is weaker than "has a row": worth one clause in the comment, since the arm above it —reviewed_gaps, three hand-written names — was exactly the failure mode of trusting a weaker check than the text implies. len(sig) >= 30is a magic floor tied to today's 37. Its purpose (do not pass vacuously on an empty load) is right, but eight classes being legitimately retired would now fail this arm with a message about documentation, which is a confusing diagnosis for a healthy change. Either derive it from a count the page states, or drop it to the previous coverage (3) and let the "missing: …" list speak — the vacuous-pass guard only needs to be non-trivially above zero.
And the review-of-the-review point: this is the right way to answer a "the file states a contract about itself and nothing enforces it" finding — derive the list instead of extending the hand-written one, and make the failure name the missing classes rather than the count.
|
VERDICT: READY at this head — ran the suite and two mutations, because a check that newly derives its own list has to be shown to bite. The change is the right one and the comment explains why the old version could not work: "the list above is hand-written and names three classes, so it keeps covering those three while the file it is about grows (37 entries today) — a fourth, fifth and thirty-seventh class can be added with no row in The anti-vacuity floor is stated as a rule rather than a constant with a reason bolted on: The new documentation row is the right shape too: One nit on the floor: Why this PR exists at all is the part worth recording: the class was added to |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3894d2b — it replaces a hand-maintained list with a derived one, which is exactly the failure mode it was found by reviewing.
The defect: tests/census_watch_test.py's arm names three classes by hand, so it keeps passing while the registry grows to 37 — the arm is pinned to a snapshot rather than to the rule. That is the same class as an arm asserting a literal count where a named set is needed (#2054's arm 3, which the author there fixed "so the next field added to Dims fails this arm instead of shrinking it").
What the change does is the right direction: "reviewed classes are documented" becomes a derived property — every class marked reviewed in registry/significant.json must have a row in docs/arch-gaps.md — so a class added without documentation fails, and a class removed does not leave the arm asserting something that no longer exists. 22 lines, no production code.
Why this matters beyond the test: #2014 adds a 37th class with no docs/arch-gaps.md row, which is precisely the state this arm should make impossible. A derived check turns "someone noticed in review" into "CI noticed", and given that #2065 landed tonight correcting the same page's routing, the page is now both more honest and self-enforcing.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 3894d2b: READY
"the single undocumented entry out of 37" is exactly the kind of finding a derived check should produce: the contract this page implies — every class in registry/significant.json has to be cited in the doc — is now enforced by tests/census_watch_test.py, and running it found precisely one violation.
That closes the loop the census work opened today: #1929 gave the states names, #1967 classified them, #1972/#1973 added entries — and this makes the documentation of each reviewed class a checked property instead of a convention. A hand-maintained doc against a generated registry drifts in one direction only, and now it cannot.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 3894d2b
The finding is a contract the file states about itself and nothing enforced: registry/significant.json's about says its classes are documented in docs/arch-gaps.md, the test named three classes by hand, and the file grew to 37 — so a 4th, 5th or 37th class could be added with no row and the test stays green. It found exactly one undocumented entry (also just added, in #1922), which is the right way to report such a thing: here is the contract, here is what was violating it, here is the row.
Deriving the list from the file instead of naming three is the whole fix, and it turns a lint into a ratchet in the direction the repo keeps choosing: the source of truth is the data file, and the documentation requirement follows it automatically.
Three asks:
- Name the reverse direction too. The check now fails when a class has no row. Does it also warn when a row names a class that is no longer in
significant.json? A removed class leaves prose that reads as current; a warning (not a failure) in that direction keeps the page honest without making removals painful. docs/arch-gaps.md's row for this class is prose, and the enforcement is a name match. If the matcher is a substring of the class name, two classes sharing a prefix (DeepseekV4FlashandDeepseekV4FlashSuperExpert) can satisfy each other's row. If it is an exact match onmodel_type, say so — that is the one line that decides whether the guard is real.- The row itself is a good model: it says why it is not an alias (
nothing at the pin decodes AW-SECD weights, so mapping it todeepseek_v41would name a backend that cannot read the file) and gives the geometry. That is the form every row should have, and it is worth saying in the test's header that the content is prose by nature — the check can only enforce presence.
Not blocking, and this is a two-file change with no code path.
|
VERDICT: READY at Reposted with the head in the line (the previous verdict named none). The check is derived from a file that claims the invariant, which is the right way round: and the finding it produced is the proof it works: "every class in The self-protection clause is the one I would have asked for: "a check that iterates nothing must not report success" — the empty- |
|
VERDICT: READY at Ran the suite: all checks passed. Then checked that the new check has teeth, because a derived list that cannot fail is worse than the hand-written three it Restored the row, suite green again. So the arm genuinely fails on the breach it reports, and it names the class in the failure rather than only a count — which is Two things I would keep exactly as they are:
One consequence to know about, not a change: the floor is Not verified: the issue-side of #2014 (whether the doc row's content is the right thing to write about that class) — I checked that the row exists and that the check |
While reviewing #2014 (which adds a 37th class to
registry/significant.jsonwith nodocs/arch-gaps.mdrow) I checked whether anything enforces the contract that file states about itself —aboutsays its classes are documented indocs/arch-gaps.md— and nothing did:tests/census_watch_test.py's arm names three classes by hand (reviewed_gaps), so it keeps covering those three while the file grows. A 4th, 5th or 37th entry can be added with no row and the suite stays green.So this turns the convention into a check, and the check found a breach that is already on
main:That class was added by engine#1922 with its reason recorded in
significant.jsonand no row here. It gets one, with the reason it is not an alias ofdeepseek_v41: it is published as remote code with AW-SECD-compressed weights, and nothing at the pin decodes those, so mapping it would name a backend that cannot read the file.Change
tests/census_watch_test.py— the documentation arm derives its list fromsiginstead of a three-name dict. Two deliberate details:len(sig) >= 30), because a loop over an empty registry passes without checking anything — a check that iterates nothing must not report success;docs/arch-gaps.md— the missing row, in the page's existing style.The existing three-name arm is kept: it asserts
model_type,familyand "reported as reviewed and uncovered" for those three, which the derived arm does not.Verification
python3 tests/census_watch_test.py→all checks passed(37/37 cited).FAIL … -- missing: DeepseekV4FlashSuperExpertForCausalLM.census_watch(CMakeLists.txt:886), so it runs in the existingcensus_watchCI job and locally.docs/arch-gaps.mdis also edited by #2065 (the stale#588pointer). My addition is a new section at the end; if #2065 lands first this needs a trivial rebase, and neither change depends on the other.