mtmd: test that every projector is registered and uniquely named - #176
mtmd: test that every projector is registered and uniquely named#176danielhanchen wants to merge 2 commits into
Conversation
Nothing checks the projector registry, and it needs no model file to check.
Two ways it breaks, both of which compile and neither of which is visible
until someone loads a model:
* a projector's enum value survives an edit while its PROJECTOR_TYPE_NAMES
entry is lost. clip_projector_type_from_string then returns
PROJECTOR_TYPE_UNKNOWN for it, and every model using that tower fails to
load with no indication of why.
* two projectors end up sharing a name, which routes one model to the
other's graph. Deleting one line to demonstrate:
{ PROJECTOR_TYPE_GLM4V, "kimik25"}
ASSERT EQUAL FAILED : "kimik25" resolves to its own projector
expected: 46
actual : 43
So: walk the enum, and for each value assert it has a name, that the name
resolves back through the same lookup a model load goes through, and that no
name is claimed twice. Also assert an unregistered string still lands on
UNKNOWN, since clip reads that string straight out of the GGUF and a typo must
not become a wrong graph.
PROJECTOR_TYPE_MLP_NORM is exempt and listed as such: clip assigns it from the
tensor shapes for Yi-type llava and never reads it from a file, so it is
unnameable by construction rather than by omission. It is the only one, and
the comment says what adding a second would mean.
185 assertions, no I/O.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ad7512711
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A projector whose enum value survives a merge while its PROJECTOR_TYPE_NAMES | ||
| // entry is lost still compiles, and every model using it then loads as | ||
| // PROJECTOR_TYPE_UNKNOWN. Two projectors sharing a name compiles too, and | ||
| // silently routes one model to the other's graph. |
There was a problem hiding this comment.
This explanation hard-wraps sentences across four source lines and, together with the adjacent comment blocks, adds extensive narration for straightforward assertions. The repository explicitly requires concise comments, avoids excessive commentary, and prohibits splitting sentences across lines; condense this to one or two simple lines without mid-sentence wrapping.
AGENTS.md reference: AGENTS.md:L74-L81
Useful? React with 👍 / 👎.
1 participant
Nothing checks the projector registry, and it needs no model file to check. That matters for the nightly: every vision and audio tower we pin (
kimik3,inkling,glm5next) is registered by hand in the same two places, and a merge that loses one of those lines compiles.What breaks without it
Two shapes, both of which compile, neither visible until someone loads a model.
A projector's enum value survives while its
PROJECTOR_TYPE_NAMESentry is lost.clip_projector_type_from_stringthen returnsPROJECTOR_TYPE_UNKNOWNfor it, and every model using that tower fails to load with no indication why. Deleting thePROJECTOR_TYPE_KIMIK25line:Two projectors end up sharing a name, which silently routes one model to the other's graph. Repointing
PROJECTOR_TYPE_GLM4Vat"kimik25":The test
Walks the enum, and per value asserts it has a name, that the name resolves back through the same lookup a model load goes through rather than by reading the table backwards, and that no name is claimed twice. Plus one assertion that an unregistered string still lands on
UNKNOWN, since clip reads that string straight out of the GGUF and a typo must not become a wrong graph.185 assertions, no I/O, runs in the existing
test-mtmd-implregistry.The one exemption
PROJECTOR_TYPE_MLP_NORMhas no name and correctly so: clip assigns it from the tensor shapes for Yi-type llava (clip.cpp, "this is a hack to support Yi-type llava") and never reads it from a file, so it is unnameable by construction rather than by omission.It is listed in an
internal_onlyset with that reason rather than the assertion being weakened, because the two cases look identical from the outside and only one of them is fine. The comment says what adding a second entry would mean: a projector no model can ever select.Found by running the test, incidentally, which is the first thing it did.
Notes
Generic, no Unsloth specifics, and worth sending to
ggml-orgas-is. It sits onbase/upstream-67a17c17c(b10775verbatim) so it can be pinned in the meantime, which is how the nightly gets it before upstream takes it.