unsloth: prove each shipped feature actually runs, not just that it merged - #178
Conversation
…erged
pin_contract.py proves the merge did not lose a pin's code. That is a
different question from whether the feature works, and neither implies the
other. The Inkling banded-attention kernel merged against upstream's sparse
attention is thirteen hunks of CUDA template parameter threading where a
mistake gives wrong attention output, and every static check passes.
feature-checks.json says, per feature, what proves it. Keyed by FEATURE with
the pin as a binding, and not the other way round: when upstream absorbs a
feature the pin is deleted, and deleting the check with it would put the blind
spot back somewhere else. The feature is still in the release, it just arrives
through the base tag now.
The one thing this file has to get right is that a pass is positive evidence.
Both harnesses exit 0 having done nothing:
test-llama-archs -a diffusion-gemma excluded, prints SKIP, exits 0
test-backend-ops test -o TYPO matches nothing, exits 0
so every probe rejects skip markers and requires a non-zero count of cases it
ran. Without that the whole layer is decoration, and the tests are mostly
about this rather than about the happy path.
No runner in the prebuild pipeline has a GPU, and test-backend-ops compares a
backend against CPU, so with no accelerator it skips every backend and proves
nothing. Those probes are DEFERRED, named and counted, never quietly passed:
ok inkling: 1/2 device rows decoded and matched CPU;
projector registry intact over 194 assertions [1 needs a GPU]
all 6 features demonstrated on CPU, 6 check(s) need a GPU and were not run
With --gpu on a B200, against the fully merged tree:
ok glm5next: 2/3 device rows decoded and matched CPU; 144/144 cases
ok inkling: 2/3 device rows; 13/13 cases; projector registry over 194 assertions
ok iq1-narrow-grids: 13/13; 13/13; 13/13 cases matched the CPU reference
ok kimi-k3: 2/3 device rows decoded and matched CPU; registry intact
ok qwen4exp-mtp: 2/3 device rows decoded and matched CPU; 4/4 cases
all 6 features demonstrated on GPU
Every pin must be in `features` or in `unchecked`, and the lint fails
otherwise. `unchecked` is a recorded reason, not a hole: it is how
diffusion-gemma went five weeks with no coverage and no record of it. The
lint earned its keep immediately by catching that the manifest still named
#107 after the pin moved to #177.
The preflight runs the matrix after its compile gate, appending to the same
PROBLEMS accumulator that already feeds the alert.
|
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: 92f1412b32
ℹ️ 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".
| if ! python3 ../scripts/unsloth/feature_matrix.py \ | ||
| --build-dir "${RUNNER_TEMP}/gate" \ | ||
| --feature-checks ../scripts/unsloth/feature-checks.json \ | ||
| --report "${RUNNER_TEMP}/feature_matrix.json" ; then |
There was a problem hiding this comment.
When one of these probes fails, this preflight only appends to PROBLEMS and sends an alert; the release workflow .github/workflows/unsloth-prebuilt.yml still runs its existing llama-only compile gate and never invokes feature_matrix.py. Consequently, if nobody acts on the alert before the nightly starts, the same broken merged tree can still fan out and be published, so this does not actually prevent shipping a feature that the new check found broken.
Useful? React with 👍 / 👎.
| # A pin nobody decided about is the failure this whole file exists to stop. | ||
| # Being in `unchecked` with a reason is a fine answer; being in neither map | ||
| # is how DiffusionGemma went five weeks with no coverage and no record of it. | ||
| - name: Every pin is either checked or knowingly unchecked |
There was a problem hiding this comment.
Adding this test and manifest validation here does not make them run when their implementation or data changes: both the push.paths and pull_request.paths lists omit scripts/unsloth/feature_matrix.py and scripts/unsloth/feature-checks.json (the existing test_*.py glob only covers the test file). A manifest-only edit can therefore violate the pin ownership invariant without running this new step, and a runner-only edit can bypass its unit tests; include both new production files in both path filters.
Useful? React with 👍 / 👎.
…ffusion-gemma reason The manifest said save_models emits the diffusion-gemma fixture. It does not any more: the saver cannot write attention.sliding_window_pattern, which that arch reads as required, so the model is excluded there too. Both blockers are outside the fixture and both are now named. Also says, in the file someone reads when adding a feature, that the backend-op checks never run in CI and what to do instead: build on a GPU box, run feature_matrix.py --gpu, paste the output into the carry PR.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 834e3ff523
ℹ️ 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".
| "silently, so before accepting a carry PR that touches one, build it on a", | ||
| "GPU box and run:", | ||
| "", | ||
| " python3 scripts/unsloth/feature_matrix.py --build-dir build --gpu", |
There was a problem hiding this comment.
When a carry-PR author follows this documented command—the stated only place where backend-op checks run—argparse exits with status 2 before executing any probes because feature_matrix.py:133 declares --feature-checks as required. Add --feature-checks scripts/unsloth/feature-checks.json so the prescribed GPU validation can actually run.
Useful? React with 👍 / 👎.
…ten for The every-pin-is-declared lint compares feature-checks.json against pr-set.json on this branch, and master's set still lists the retired pins, so the check fails on pins this manifest was never meant to cover. Merging #174 in makes it compare against the real set; the diff goes away when #174 lands.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c752e6baf5
ℹ️ 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".
| rc, out = run(cmd, b, gpu) | ||
| if rc != 0: | ||
| raise Unproven(f"{' '.join(cmd[1:])} exited {rc}") | ||
| m = re.search(r"(\d+)/(\d+) tests passed", out) |
There was a problem hiding this comment.
On a build exposing multiple non-CPU devices or backends, test-backend-ops prints one tests passed summary per device, but re.search inspects only the first. Since test_backend treats an unsupported filter as a successful 0/0 run, a first device reporting 13/13 followed by another reporting 0/0 still exits successfully and this probe records a pass, leaving that second shipped backend untested. Parse all device summaries and require a nonzero passing count for each visible accelerator.
Useful? React with 👍 / 👎.
| for name, feat in sorted(doc["features"].items()): | ||
| if args.only and name != args.only: | ||
| continue |
There was a problem hiding this comment.
When --only contains a typo or a feature removed from the manifest, every iteration is skipped, failed remains zero, and the command exits successfully after claiming that zero features were demonstrated. This can falsely validate the exact manual single-feature run the option is intended to support; verify that the requested name exists before filtering.
Useful? React with 👍 / 👎.
The blocker was the fixture, not the arch: see the pin. It decodes and matches CPU on every device, so it moves out of `unchecked` and into `features` with an arch probe, which is the whole point of keeping that map honest rather than letting a gap sit unnamed.
|
DiffusionGemma moved from Seven features on the full thirteen-pin mix at Worth noting what the manifest did here. DiffusionGemma had been uncovered for five weeks with nothing recording it. Writing the gap down with a reason is what made it a question someone would look at, and the answer took an afternoon. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b41b099685
ℹ️ 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".
| else: | ||
| # Nothing was shown either way. Not a failure here, but it must not | ||
| # read as one of the ok lines. |
There was a problem hiding this comment.
When a manifest entry has "checks": [], this branch treats the absence of evidence like an intentional GPU deferral: failed is never incremented, the report remains successful, and the final summary claims every feature was demonstrated. The lint also counts that entry's owner as checked, so an accidentally empty feature definition can silently bypass the coverage invariant; distinguish zero configured checks from checks that were actually deferred and fail the former.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 221b7a7569
ℹ️ 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".
| "https://github.com/ggml-org/llama.cpp/pull/25731/commits/44eb88e9aba218b24c0f374f2ec1c4d7d7920877", | ||
| "https://github.com/unslothai/llama.cpp/pull/70/commits/edfd4c1a3b7a653303a85257ddac2a1f3ce39a2f", | ||
| "https://github.com/unslothai/llama.cpp/pull/91/commits/c86ed269986f2dced6325c5c58bda966a2e2ead1", | ||
| "https://github.com/unslothai/llama.cpp/pull/177/commits/4137a7d3669c3fe94431b68e2e04b8804e620760", |
There was a problem hiding this comment.
The newly added unslothai#177 pin is absent from both the manifest's owner values and unchecked, so the new “Every pin is either checked or knowingly unchecked” step always exits nonzero; the same mismatch affects unslothai#172, #171, and #173. Update the feature owners or add explicit unchecked entries so this commit can pass its own resolver-tests job.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 324f152254
ℹ️ 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".
|
|
||
| def probe_mtmd(check: dict, b: Path, gpu: bool) -> str: | ||
| """The projector registry is intact, including this projector's entry.""" | ||
| rc, out = run([str(b / "test-mtmd-impl"), "test_projector_registry"], b, gpu) |
There was a problem hiding this comment.
When a projector is removed or renamed—especially after its owning pin is repinned or deleted—the registry can remain internally consistent and test_projector_registry can still report nonzero assertions. Because this probe never uses check["projector"], the inkling and kimik3 checks then pass without proving that their requested projector still exists; pin_contract.py cannot close this gap once the pin disappears, and a regression inside a repinned PR regenerates its contract. Make this probe assert the named projector rather than only the registry as a whole.
Useful? React with 👍 / 👎.
1 participant
Stacked on #175. That PR proves the merge did not lose a pin's code; this one proves the features still work, which is a different question and neither implies the other.
The case that makes the difference concrete: the Inkling banded-attention kernel merged against upstream's sparse attention is thirteen hunks of CUDA template parameter threading. Get one wrong and you get wrong attention output, a clean merge, a clean
pin_contract.py, a cleanmerge_checks.pyand a clean compile.feature-checks.jsonSays per feature what proves it. Keyed by feature with the pin as a binding, and not the other way round: when upstream absorbs a feature the pin is deleted, and deleting the check with it would put the blind spot back somewhere else. The feature is still in the release, it just arrives through the base tag now. So an entry outlives its
owner.Every pin in
pr-set.jsonmust appear infeaturesor inunchecked, and the lint fails otherwise.uncheckedis a recorded reason, not a hole; it is exactly how DiffusionGemma went five weeks with no coverage and no record of it. The lint earned its keep on the first run by catching that the manifest still namedunslothai#107after the pin had moved to#177.The one thing this has to get right
A pass has to be positive evidence. Both harnesses exit 0 having done nothing:
So every probe rejects skip markers and requires a non-zero count of cases it actually ran. Without that this whole layer is decoration, and the tests are mostly about that rather than about the happy path: an all-SKIP arch run, an arch with no row at all, an op filter that matched nothing, an mtmd run with zero assertions. Fifteen checks, each fed the verbatim output shape of a real run.
No GPU in CI, said out loud
Every runner in the prebuild pipeline is
ubuntu-24.04/windows-2022/macos-*.test-backend-opscompares a backend against the CPU reference, so with no accelerator it skips every backend and printsSkipping CPU backend, having proved nothing.Calling that a pass would be a lie and calling it a failure would block every nightly. So those probes are deferred: named, counted, and never folded into the success line.
A feature whose every check needs a GPU prints
--, notok. A run that only ever prints success teaches the reader that green means covered.What it says on a GPU
Against the fully merged thirteen-pin tree at
b10775, CUDAsm_100, B200:That is the first time any of these has been demonstrated in the composed tree rather than one PR at a time.
Merge order
test-llama-archs.cppand share exactly one line,}.pr-set.jsonto be in the manifest, andmaster's pin set still lists the retired pins.