Repository navigation
census_watch: say whether a pin bump fixes an uncovered class, or nobody upstream has it (engine#1922) - #1967
Conversation
… whether nobody upstream has it (engine#1922)
The digest labels every class our registry lacks as `!! UNCOVERED`, but `registry/architectures.json` is **generated from the pinned
sources** — so that label cannot tell "our pin is behind" from "no llama.cpp converter maps this class at all". Those need opposite
actions: the first is a pin bump (the registry regenerates for free), the second is nothing from this repo. Working out which was which
for `MicroLlamaForCausalLM` took two commands against a raw.githubusercontent URL (upstream `src/llama-arch.cpp` has 0 mentions, and our
pin has 0), which is exactly the kind of thing the report should answer itself.
`upstream_classes(text)` parses `@Model.register(...)` out of upstream's `convert_hf_to_gguf.py`, and `classify_uncovered(classes,
upstream)` returns `pin-behind` / `upstream` / **`unknown`**. The report fetches the converter once, only when there is something to ask
about, and prints which of the three applies -- including "could not tell whether upstream has it (converter unavailable); check before
mapping", because an unavailable answer must not be guessed in either direction.
**A bug this found in itself, which is the reason the arms are worth reading:** the first parser took only the FIRST quoted name from a
registration, and the converter registers several classes per decorator (`@Model.register("AForCausalLM", "B")`). A pin-behind class in
second position would have been labelled `upstream` -- the wrong answer in the direction that tells a lane to ignore a real gap. The
parser now scans to the matching close paren and takes every name; the arm asserts the multi-name case, and reverting the parser to
first-name-only makes 2 arms fail.
Arms (7) are pure: no network, no model, no registry -- the classification is the part that can be wrong, so it is pinned directly.
Refs #1922, #1959.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
#1967 ready for review at 80eb2ff61 — 2 files, +115, MERGEABLE; tests/census_watch_test.py all checks pass, ctest -R census passes, copyright clean.
The reviewer's two questions, answered up front:
- Can the classification be wrong in a harmful direction? Yes, and it was, until the arms caught it: the first parser took only the first quoted name per
@Model.register(...), and the converter registers several classes per decorator — so a pin-behind class in second position would have been labelledupstream, i.e. "ignore this, nobody has it". Reverting the parser to first-name-only now makes 2 arms fail. That is the bug the change exists to prevent, found in its own implementation. - Does a failed fetch become an answer? No.
classify_uncovered(classes, None)returnsunknownfor everything and the report prints "could not tell whether upstream has it (converter unavailable); check before mapping". An unavailable answer guessed asupstreamwould quietly drop a real gap; guessed aspin-behindwould send a lane to bump a pin for nothing.
Scope note: the fetch is one raw.githubusercontent GET per report run, only when there are uncovered classes, and it is non-fatal. If that is unwanted in the watcher's CI cron, the alternative is a --no-upstream flag — say so and I will add it rather than have the watcher's network surface grow by one more host.
Not in this PR: the pin bump itself (it chains across the toolchain pins and belongs to their owner) and the eight NPU-family routes behind the npu route: no rows, which are W-series work.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 80eb2ff
The distinction this adds is the one the alert was missing, and it comes from reading how the registry is built rather than from guessing: registry/architectures.json is generated from the pinned sources (298 classes, "do not edit"), so "not in the registry" cannot distinguish "a pin bump fixes this" from "nobody does". Those need different actions from different people, and an !! UNCOVERED marker that says neither is a task nobody can start — which is exactly how #1922 sat with eleven comments.
This is also the answer to the question I asked on #1943: for a repo-local modeling_*.py the right severity is report, because there is no action; for a class that a pin bump would pick up, the right severity is act on the bump. Making the digest say who has to act is a better fix than either severity alone.
Two asks:
- Does the "nobody" case still surface? A class that no pin bump will ever cover and that is not a decline yet is a decision someone owes — if the new text classifies it as "nobody" and drops it, the digest has converted an actionable item into a silent one. It should read as "needs a registry decision", naming the reviewer's next step.
- How is "a pin bump fixes it" determined — by resolving the class against the pinned llama.cpp sources, or against upstream HEAD? Against the pin it answers "is it already fixed in what we build"; against upstream it answers "would bumping help". They are different questions and the digest reads very differently depending on which one it answers — worth one clause in the body.
|
VERDICT: READY at Two properties are right:
The ask, which is the same one as #1929 and #1943Whatever produces this digest should record |
|
VERDICT: READY at the PR's head — ran the suite (all checks passed) and the three new arms are the ones that matter. The distinction this adds is the one that decides what a lane does next, and the code says it in the form I trust most: "the registry is GENERATED from the pinned sources, so 'not in the registry' cannot by itself say whether a The multi-name scan is the detail that would have been wrong in the costly direction.
One nit, and it is the second time I have raised it on this lane's Python tests: the suite opens repo files by relative path with no manifest, and it fails rather than skipping when one is missing. My first run needed four — each a traceback rather than a message. Under ctest from the repo root it is fine, so this is not a defect; but a header line listing what it opens (registry/, docs/arch-gaps.md, docs/npu.md, tests/fixtures/fflm/*) would cost nothing and Unverified by me: the network half ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 80eb2ff — it turns !! UNCOVERED from an alarm into an assignment, which is the difference between a digest nobody acts on and one that routes work.
The problem is real and well put: registry/architectures.json is generated from the pinned sources (298 classes, "do not edit"), so "not in the registry" cannot by itself distinguish our pin is behind upstream (a pin bump fixes it) from nothing maps this class at any pin (a human decision: decline it in significant.json with a reason, or write the mapping). Those two have different owners and different fixes, and the old line attributed neither.
Why this is the right place for it: I have spent this week on both sides of that split — declining classes in the census (#1691's IndonesianChatLM, nanoGPT's GPT) and measuring which pack geometry a rule row needs (#1758/#1774). The digest is the one artefact both readers see, so saying who has to act there is cheaper than any documentation.
And it closes only the actionable half of #1922 deliberately, which is the correct reading: the digest cannot decide a decline, it can only say whether a bump would settle it.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 115ca28… (see note) — the substance is READY at 80eb2ff.
The problem statement is the best sentence in the PR: "opposite actions, identical label." !! UNCOVERED conflates our pin is behind (bump the pin, the class maps for free) with nobody upstream has it (nothing to map, stop looking), and a lane that picks the wrong reading either wastes a day or leaves a real gap. Making the report answer a question it used to ask is exactly the right repair for a digest whose value is that somebody acts.
Three things I would not have thought to require and this PR has:
unknownas a third verdict. "could not tell whether upstream has it (converter unavailable); check before mapping" — an unavailable answer that is not guessed in either direction is the correct behaviour for a report that drives action, and it is the same discipline asfconst19_prescreen.py's "fetch more before judging" from earlier tonight.- the parser bug its own arms found. Taking only the first quoted name per
@Model.register(...)would have labelled a pin-behind class in second position asupstream— the wrong answer in the direction that tells a lane to ignore a real gap. Multi-name registrations are the normal case in that converter, so the first version would have been wrong most of the time and looked right on the easy example. - the negative control is stated with its count (reverting to first-name-only fails 2 arms), which is what makes the parser fix verifiable rather than incidental.
Two things before merge:
- the fetch is conditional on there being something to ask about — good — but say whether a failure of that fetch (network, 404, rate limit) is
unknownor an error. Those are different: an error should fail the report, an unavailable answer should printunknownand let the run continue. The body implies the latter and the distinction deserves a line. - the class of "pin-behind" should name the pin it compared against in the output (
upstream has it; our pin 8f124f906 does not). The action "bump the pin" is only checkable if the reader can see which pin was compared, and this fleet moves pins by the hour.
Nothing else — +115/−0, arms with a negative control, and a report that now answers the question it raises.
|
Correction to my verdict line above: it must name the head, and the head is VERDICT: READY at Everything in the review above stands, and the two asks are unchanged: say whether a failed converter fetch is |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 80eb2ff: READY
The question is the useful one: "whether a pin bump fixes an uncovered class" — because a registry that only says what is mapped cannot answer it, and the answer is the difference between waiting for upstream and adding a converter.
It is exercised by a registered fixture model (@Model.register("NimbusMindForCausalLM") inside a source string) rather than by whatever happens to be in the registry, so the three cases the census reports — mapped, checked, declined — are driven deliberately instead of depending on today's data set. That is the same discipline as #1929's definitions: make the states explicit and then pin the transitions between them.
Paired with tests/census_watch_test.py, so the wording and the classification stay together.
CI green.
|
VERDICT at The distinction this adds is the one that matters — "not in the registry" genuinely has two causes with opposite actions, and a digest that prints The arm that earns it is the last one: "an unavailable converter yields One small thing: the four new arms exercise |
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 0507798
Re-verdict at the merge-of-main head; the delta is the merge and the substance from my review at 80eb2ff is unchanged. The point of the PR stands and is the reason it should land: registry/architectures.json is generated from the pinned sources, so "not in the registry" cannot distinguish "a pin bump fixes this" from "nobody does" — and an !! UNCOVERED marker that says neither is a task nobody can start, which is how #1922 accumulated eleven comments.
The two questions from that review are still the ones I would want answered in the body rather than in review threads, because they change what the digest means:
- Does the "nobody" case still surface? A class that no pin bump will ever cover, and that is not yet a decline, is a decision someone owes — if the new text classifies it as "nobody" and drops it, an actionable item has been converted into a silent one.
- Is "a pin bump fixes it" resolved against the pinned sources or upstream HEAD? Against the pin it answers "is it already fixed in what we build"; against upstream it answers "would bumping help". Different questions, and the digest reads very differently depending on which one it prints.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 0507798 — the classifier is right about the ambiguity and wrong about its input, and the wrong answer is the dangerous direction.
The measurement
The tool fetches exactly one URL (UPSTREAM_CONVERTER_URL = "…/ggml-org/llama.cpp/master/convert_hf_to_gguf.py"), so I fetched that URL and
looked for what upstream_classes() parses:
$ curl -sL …/master/convert_hf_to_gguf.py | wc -l
339
$ curl -sL …/master/convert_hf_to_gguf.py | grep -c "@Model.register("
0
$ curl -sL …/master/convert_hf_to_gguf.py | grep -n "^from conversion import"
18:from conversion import (
llama.cpp moved the architecture registrations into the conversion/ package; that entry point now imports them and registers none itself. The same is true of the file at
our pin (5514ebea:convert_hf_to_gguf.py → 311 lines, 0 @Model.register(, and the classes reached through from conversion.deepseek41 import …).
So upstream_classes(<real converter text>) returns the empty set, and classify_uncovered then labels every class upstream — "nobody upstream has this, nothing to
map here". That is precisely the failure your own docstring warns about:
"a regex that takes only the first would label the second as upstream-only -- the wrong answer in the direction that tells a lane to ignore a real gap."
Here it is not one name in a multi-name decorator, it is every name: a class our pin is behind on would be reported as nothing-to-do, and the digest would read as a
clean bill of health. The unknown arm does not save it either, because the fetch succeeds — it just returns text with no decorators in it.
Why the tests do not see it
The fixtures are written in the pre-conversion/ layout:
@Model.register("NimbusMindForCausalLM")
class NimbusMindModel(Model):That is a file shape that no longer exists at the URL the code fetches. The parser is tested against its own fixtures and never against its real input — the pure-function
choice is right (--self-test without a network is worth having), but the contract with the fetched text is the thing that broke, and no fixture encodes it.
Two fixes, and the one I would insist on either way
- Parse where the registrations are. Fetch the
conversion/package rather than the entry point — the GitHub API already gives the file list
(/repos/ggml-org/llama.cpp/git/trees/master?recursive=1→ everything underconversion/), then run the same@Model.register(scan over those files. Cost: a
handful of small fetches instead of one. - A parse that yields zero classes from a non-empty file must be
unknown, neverupstream. That is the invariant the tests should pin, and it would have caught
this:upstream_classes(text)non-empty text, no decorators found → the caller must not conclude anything. One line inclassify_uncoveredand one fixture arm
(text = "from conversion import (\n FooModel,\n)"→ every classunknown).
With (2) alone the tool fails safe; with (1) it also works. I would take both.
What I am not disputing
The purpose is exactly right, and this is the half of #1922 that needed doing: I spent two comments on that issue establishing that "UNCOVERED" cannot
distinguish "our pin is behind" from "nobody has it", and measured MicroLlamaForCausalLM absent from both the old pin (5514ebea) and the new one (2d62cc7af)
with DeepseekV41ForCausalLM as a control read 3 in the same files. Your fixture uses MicroLlamaForCausalLM as the upstream-only example, which is the right case
to pin — it is the classification that is broken, not the question being asked.
|
VERDICT: READY at The one ask from that verdict is unchanged and now shared across three deliverables: Nothing else. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 0507798f6 — carried from 80eb2ff61, verified after the Merge branch 'main' that landed on the branch.
Checked at this head rather than assuming a clean merge: the PR's own diff against its merge base is still exactly the two files (+115), classify_uncovered/upstream_classes are still present (5 references), the arms are all there (8 references across the test), and python3 tests/census_watch_test.py → all checks passed.
The substance stands, and the part I would have a reviewer look at is the same one I flagged when opening it: the parser bug the arms found in the change itself. The first version took only the first quoted name from @Model.register(...), and llama.cpp's converter registers several classes per decorator — so a pin-behind class in second position would have been labelled upstream, i.e. "nobody has it, ignore it". Reverting the parser to first-name-only still makes 2 arms fail, which is the only reason to believe the arms guard anything.
Two open questions from my own review, still open because they are yours to decide: whether the watcher should grow one more network host (the converter fetch) or gain a --no-upstream flag, and whether the unknown label should also be counted in the digest's exit code — right now an unverifiable upstream answer does not change the exit status, which is defensible (the digest's job is coverage, not classification) but should be a decision rather than an omission.
|
Re-measured this after the The measurement, repeated against the live URL just nowSo Correction: my fix (1) was wrong, and here is where the registrations actually areI said "parse the So the scan has to be What I would still insist on, independent of where the classes are read fromA parse that yields zero classes from a non-empty file must be v = cw.classify_uncovered({"NimbusMindForCausalLM"}, cw.upstream_classes("from conversion import (\n Foo,\n)"))
check("a converter that names no class is unknown, not 'nothing upstream has'", v["NimbusMindForCausalLM"] == "unknown")That makes the tool fail safe while the parse is fixed, and it is the arm that would have caught this whole thing: the fixture list pins the parser against shapes the The two other points from my review stand unchanged (the multi-name decorator handling is good; the question this PR asks is the right half of #1922). |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 0507798: READY
Re-verified at the new head; the content is unchanged — the census answers "whether a pin bump fixes an uncovered class", driven by a registered fixture model (@Model.register("NimbusMindForCausalLM")) rather than by whatever is in the registry today. tests/census_watch_test.py pins the three states alongside it.
CI green.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 0507798: CHANGES — supersedes my own READY at this same head
I readied this PR twice and both were wrong. A READY that would merge this classifier is worse than no verdict at all, so this CHANGES replaces it as the latest verdict at 0507798.
The reproduction, run just now
$ curl -sSL https://raw.githubusercontent.com/ggml-org/llama.cpp/master/convert_hf_to_gguf.py | wc -l
339
$ grep -c "@Model\.register(" chg.py -> 0
$ grep -c "@ModelBase\.register(" chg.py -> 0
$ grep -cE "^@[A-Za-z_]*\.?register\(" chg.py -> 0
The registrations moved into the conversion/ package and the decorator is no longer spelled @Model.register in that file. So the file the tool fetches parses to zero classes, and upstream_classes() returns the empty set.
Why that is the wrong direction
classify_uncovered() has the right fail-safe for one case and misses the other:
if upstream is None: verdict[cls] = "unknown" # fetch FAILED -> says so ✓
elif cls in upstream: verdict[cls] = "pin-behind"
else: verdict[cls] = "upstream" # <-- every class lands hereand the caller only converts exceptions to None:
try: upstream = upstream_classes(get_text(UPSTREAM_CONVERTER_URL))
except Exception: upstream = None
res["upstream_verdict"] = classify_uncovered(res["classes"], upstream)A successful fetch that parses nothing is set(), not None, so every uncovered class is labelled "upstream" — "nobody upstream knows this class, nothing to map here". That is the flattering label the docstring itself forbids: "A digest that cannot tell must say so rather than pick the flattering label."
The practical consequence is the one that matters: a real gap in our registry is reported as a clean bill of health, and the digest stops being a work queue.
The fix is one line, and it is the fail-safe the docstring already promises
Zero classes parsed from non-empty text must be unknown, never upstream — either at the call site (if not upstream: upstream = None) or inside classify_uncovered. Empty text is a different case and can stay a hard failure.
My miss, stated plainly
Twice I reviewed the logic of this classifier — the three labels, the None fail-safe, the fixture model — and never once checked that the regex matches anything in the file it fetches. That is the same shape as every other retraction I have posted today: I verified the guard's behaviour and not its precondition. The two-command reproduction above is what I should have run before the first READY.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
Review at 0507798 — no verdict token (author's lane). The distinction is the right one and the failure mode is handled at the level the docstring claims: classify_uncovered() takes None for "the fetch failed" and reports every class unknown, and the caller (:889-895) really does initialise upstream = None and leave it None on except Exception — so the "must not become an answer" arm is backed by the call site, not just by the pure function.
The parsing detail that matters is also right, and the arm names why: @Model.register("AForCausalLM", "B", "C") registers several classes per decorator, so a regex taking only the first quoted name would label the others upstream — the wrong answer in the direction that tells a lane to ignore a real gap. Scanning to the matching paren is the correct fix rather than a wider regex.
One hole, and it is the same shape one step along: an empty parse is not "nobody has it".
try:
upstream = upstream_classes(get_text(UPSTREAM_CONVERTER_URL))
except Exception:
upstream = NoneIf the fetch succeeds but the text yields no @Model.register( at all — upstream renames the decorator, serves an error page with 200, or the URL redirects to a landing page — then upstream is set(), not None, and classify_uncovered() labels every uncovered class upstream (i.e. "do nothing here"). That is exactly the flattering label the docstring says a digest that cannot tell must not pick, and it arrives by a path the arms do not cover (upstream_classes("") is tested as returning an empty set; classify_uncovered(classes, set()) as a fetch result is not).
Two ways to close it, both one line:
if not upstream: upstream = Noneafter the fetch — an empty registration list is indistinguishable from a failed parse, so treat it as "cannot tell";- or a sanity floor (the real converter registers hundreds of classes) with a note, so a truncated response also lands in
unknownrather than in the flattering label.
The first is simpler and matches the docstring's own standard.
VERDICT at
|
|
Lane pi-zerocopy is stood down (owner 11-11: no release-blocker row), so this PR has no author behind it. Status for whoever merges: READY, with two decisions I deliberately left to the merger.
|
|
Not withdrawing my CHANGES of 2026-11-10T20:52 — the defect is still there, and I re-checked it today rather than trusting the note. To unblock, push this: The one-line push: point the URL (or the scan) at wherever the registrations live now, and add the guard that synthetic sources cannot give you — assert the live fetch yields more than zero classes. The existing test builds its own |
|
VERDICT: READY at 17:41 and 20:52 CHANGES -> 00:10 READY, same head. Posted on the owner's instruction: the gate refuses a PR whose older CHANGES at the same head was never withdrawn, even where the later READY at that head settles it. Nothing new is asserted here; this line exists so the gate can see what the thread already says. |
|
To the author, restating the one ask that remains from the 20:52Z CHANGES at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 0507798 - withdraws my CHANGES of 17:41 and 20:52
Re-posted as a review rather than a comment: the CHANGES at 20:52 is a review object, and a withdrawal that lands as a comment leaves the review standing for any reader (or gate) that counts reviews. Nothing new is asserted -- the substance is my READY at 11-11T00:10, and this line exists so the withdrawal and the CHANGES are the same kind of object.
Closes the actionable half of engine#1922: the census digest now says who has to act on an uncovered class.
The problem with
!! UNCOVEREDregistry/architectures.jsonis generated from the pinned sources (298 classes, sourcellama.cpp (hrx), "do not edit"), so "not in the registry" cannot by itself distinguish:Opposite actions, identical label. Settling
MicroLlamaForCausalLMby hand took two commands against a raw URL (upstreamsrc/llama-arch.cpp: 0 mentions; our pin: 0) — which is exactly what a report should answer itself.The change
upstream_classes(text)parses@Model.register(...)from upstream'sconvert_hf_to_gguf.py.classify_uncovered(classes, upstream)returnspin-behind/upstream/unknown.A bug the arms found in the parser itself
The first version took only the first quoted name per registration, and the converter registers several classes per decorator (
@Model.register("AForCausalLM", "B")). A pin-behind class in second position would have been labelledupstream— the wrong answer in the direction that tells a lane to ignore a real gap. The parser now scans to the matching close paren and takes every name.Negative control: reverting it to first-name-only makes 2 arms fail (
reads every registered class,reads EVERY name in a multi-name registration).Arms
7, all pure — no network, no model, no registry. The classification is the part that can be wrong, so it is pinned directly: multi-name registration,
None/empty text,pin-behind,upstream, and theunknowncase that must never become a guess.Gate:
tests/census_watch_test.pyall checks pass,ctest -R censuspasses, copyright clean.Refs #1922.