Skip to content

census: record the three vision/audio pack classes as reviewed, not aliases (#2171) - #2229

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
pi-i679/2171-reviewed-classes
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
pi-i679/2171-reviewed-classes

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

What

registry/significant.json gains the three architecture classes the 2026-11-11 census reported as registry: none, each with the model_type taken from the pack's own config.json and a family note that says why it is not an alias:

class model_type source
MiniCPMV4_7ForConditionalGeneration minicpmv4_7 openbmb/MiniCPM-V-4.7-1B/config.json
MiniCPMOcrForConditionalGeneration minicpm_ocr FastFlowLM/MiniCPM-OCR-1.0-NPU2/config.json
WhisperForConditionalGeneration whisper openai/whisper-large-v3-turbo/config.json

That is the mechanism tools/census_watch.py:564 already documents for Lfm3ForCausalLM and DFlash2DraftModel: a class the registry has reviewed and declined is listed, not alerted (engine#1451), so the daily issue stops reopening on packs whose family this engine does not serve. This applies the same rule to three more classes instead of inventing a new one.

Verification that they are not served, at the pin: grep -rilE 'minicpmv|minicpmocr|whisperfor' over src/ in the llama.cpp tree at the registry's pinned source (2b7960bae) finds nothing, and registry/architectures.json (generated) has no entry for any of them. All three packs ship a second artefact beside model.q4nx -- vision_weight.q4nx for the two MiniCPM packs, an audio encoder for Whisper -- so they are multimodal packs whose non-text tower this engine has no route for, which is the honest reason to record rather than a mapping.

--check-gaps stays meaningful: tools/registry_build.py --check-gaps fails when a class in this file becomes mapped, so if the pin later gains a minicpmv or whisper class, CI will demand a mapped_ok reason instead of silently keeping a stale "declined".

What I deliberately did not touch

  • OpensciForCausalLM (REMOTE-CODE) is not added. It is reported through a different route and it is a safety signal -- a repo that needs trust_remote_code to run. Recording it as "reviewed, not an alias" would suppress exactly the alert that should keep firing.
  • The nine UNVERIFIABLE gated repos are a credentials problem, not a registry gap.
  • registry/architectures.json is generated from the pinned sources ("do not edit" in its own about), so no mapping is added there; a hand-added entry would be overwritten by the next registry_build.py run and would claim support that does not exist.
  • The many npu route: no lines are engine#588's scope, not this file's.

Diff is additions only -- 15 lines, 0 removed, no reordering of the existing 38 classes.

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2171 - Partially compliant

Compliant requirements:

  • The three architecture classes are added to registry/significant.json with correct model_type and family notes
  • The family notes explain why these are not aliases and provide context on multimodal packs and backend limitations
  • The PR description confirms that the pinned source does not register these classes, supporting the decision to mark them as reviewed but not aliases

Non-compliant requirements:

  • The PR does not update docs/arch-gaps.md as required

Requires further human verification:

  • Confirmation that docs/arch-gaps.md has been updated with the new classes (this is a requirement but not visible in the diff)
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Documentation Update

The PR adds entries to registry/significant.json to prevent alerts for three new architecture classes, but does not update docs/arch-gaps.md as required by the ticket. This documentation update is necessary to maintain consistency and completeness of the architecture gap tracking.

"MiniCPMOcrForConditionalGeneration": {
 "model_type": "minicpm_ocr",
 "family": "MiniCPM-OCR 1.0 vision-language OCR pack (FastFlowLM/MiniCPM-OCR-1.0-NPU2: model.q4nx beside vision_weight.q4nx); the pinned llama.cpp registers no minicpm class at all, and the OCR read is a vision tower this engine has no route for",
 "watch": "report"
},
"MiniCPMV4_7ForConditionalGeneration": {
 "model_type": "minicpmv4_7",
 "family": "MiniCPM-V 4.7 vision-language pack (MiniCPM-V-4.7-1B and -35B-A3B NPU2, each shipping vision_weight.q4nx beside model.q4nx); nothing at the pin registers a minicpmv class, so this is a vision tower plus a text decoder this engine does not serve, not an alias",
 "watch": "report"
},
"WhisperForConditionalGeneration": {
 "model_type": "whisper",
 "family": "Whisper ASR encoder-decoder (Whisper-V3-Turbo-NPU2, base openai/whisper-large-v3-turbo); whisper at the pin is a separate library (whisper.cpp), not a class the engine's text route registers, and the q4nx pack is an audio encoder this backend never loads",
 "watch": "report"
}

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Closing: wrong file, and the gap it targets is already answered.

I read registry/architectures.json (generated, "do not edit"), then found registry/significant.json and added the three classes there. The test suite says why that is wrong:

FAIL engine#2171 before: the 2026-11-11 rows alert with no answers (the rule is unchanged)

tests/census_watch_test.py:1023 pins exactly the behaviour I changed -- with no answers recorded, all 31 FLM rows must still alert. significant.json is the model census record ("reviewed as NOT an alias of a supported family"); the mechanism for these pack rows is registry/flm_answers.json, and it turns out to already contain all three:

MiniCPMV4_7ForConditionalGeneration  -> not served: a vision-language pack whose class the pin does not register; ... engine#1788
MiniCPMOcrForConditionalGeneration   -> not served: an OCR/vision pack whose class the pin does not register; ...
WhisperForConditionalGeneration      -> present

with the doc rows the file's own instructions require ("add an entry only with a row for it in docs/npu.md"). They were added by 853f8c9c (2026-11-11 18:04Z, "census: an answered FastFlowLM gap stays answered"), which is after the census output quoted in #2171 -- so those registry: none lines are stale text, not an open gap.

Nothing here is worth a change, and a same-class entry in significant.json would additionally have made the next pin bump's --check-gaps report a class as reviewed twice, in two files with two different meanings. My mistake was picking the second-best file without reading the test that pins the contract; the test was right.

I keep the pieces of the investigation that are reusable: nothing at the pin registers minicpmv/minicpm_ocr/whisper, and --check-gaps only fails when a reviewed class becomes mapped, which is the guard that keeps whatever ends up in flm_answers.json honest.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 4751c0e — a census entry is only worth what reads it, and there are four readers to name.

The entries are the right shape

"watch": "report" with a family string that says what the pack is (MiniCPM-OCR 1.0 vision-language OCR pack, model.q4nx beside vision_w…) — that is a row a future reader can act on, and putting three vision/audio classes in as reviewed rather than as unknowns is the correct census move: the classes are not gaps, they are decisions, and the entry records which.

The one ask: say which reader enforces it

registry/significant.json has 27 watch entries and four things read the registry — tests/census_watch_test.py (6 refs), tests/registry_gaps_test.py (3), tools/census.py (3), tools/census_watch.py (7) — and this PR does not change any of them. So the entries are data that the tools will pick up, which is good, but nothing in the diff shows them being checked: a new pack class arriving without an entry still passes.

One of these makes the PR much stronger:

  • "registry_gaps_test now fails on a pack class in this shape with no entry" — the row becomes enforced;
  • or "these three were the last unentered classes; census_watch --report lists none" — the row becomes a closure with a measurement;
  • or "the entries are read at census time only; nothing fails" — honest, and the PR is documentation, which is worth saying.

Why this class of PR matters more than its size

The class of defect this page keeps recording is a reviewed decision that looks like an oversight because nobody wrote it down — the FLM families (#2207, #1787), the Medgemma gemma3 arrival that read as "no trace", the two-liner headers. Fifteen lines that say "this is a decision, not a gap" are what stop the next person re-deriving the decision from scratch, and the entries are specific enough (model_type + family + watch) to survive a rename.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CHANGES at 4751c0e — fresh configure rc 0, but the PR's own suite fails 5 checks on this head:

FAIL every one of the 41 reviewed classes is cited in docs/arch-gaps.md -- missing: WhisperForConditionalGeneration
FAIL flm first run: an unmapped family alerts (exit 1)
FAIL flm report: the unknown family is marked !! NEW FLM
FAIL flm --full: an unmapped repo in the listing still alerts

One is a missing citation, three are the fixture problem. The ledger now answers WhisperForConditionalGeneration (plus the two other vision/audio classes), and docs/arch-gaps.md does not cite it — the test's rule is that every reviewed class appears there, so the answer has no row a reader can find.

The other three are the more interesting failure: the fixtures that assert "an unmapped family alerts" and "the unknown family is marked !! NEW FLM" are built on FastFlowLM/MiniCPM-V-4.7-1B-NPU2, and that family is one the PR now answers. So answering a class silently removed the suite's example of an unanswered one — the alert path is no longer exercised anywhere. That is exactly the risk of a ledger that grows: the negative case has to move to a family the ledger does not cover, or the check that "an unmapped family alerts" becomes a check that never fires.

What I would do before merging:

  1. Cite the three new classes in docs/arch-gaps.md — the test names the one it wants (WhisperForConditionalGeneration), and the other two presumably land in the same edit.
  2. Re-point the three negative fixtures at a family the ledger does not answer. The clean way is to have the fixture build its live_archs from a synthetic repo whose architecture is not in registry/flm_answers.json (the test already passes answers=NO_ANSWERS in one place, which suggests the mechanism exists — it is the repo listing that still names an answered one).
  3. Add an assertion that the ledger cannot swallow the negative case: e.g. the "unmapped" fixture fails loudly if its chosen architecture later gains an answer, so the next ledger addition has to update the fixture instead of quietly turning a test into a no-op.

Scope of my check: fresh configure rc 0 and the full census_watch_test.py run on this head (5 failures, listed above), plus the readings of flm_answers.json and the fixture lines that name MiniCPM-V-4.7-1B-NPU2. No network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant