-
Notifications
You must be signed in to change notification settings - Fork 44
unsloth: prove each shipped feature actually runs, not just that it merged #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
92f1412
834e3ff
c752e6b
7750194
619b90c
b41b099
9af5835
221b7a7
324f152
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,13 +120,44 @@ jobs: | |
| scripts/unsloth/test_merge_checks.py \ | ||
| scripts/unsloth/test_sync_deletes.py \ | ||
| scripts/unsloth/test_carry_vintage.py \ | ||
| scripts/unsloth/test_pin_contract.py; do | ||
| scripts/unsloth/test_pin_contract.py \ | ||
| scripts/unsloth/test_feature_matrix.py; do | ||
| echo "::group::$t" | ||
| python3 "$t" || fail=1 | ||
| echo "::endgroup::" | ||
| done | ||
| exit "$fail" | ||
|
|
||
| # 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 | ||
|
Comment on lines
+131
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Adding this test and manifest validation here does not make them run when their implementation or data changes: both the Useful? React with 👍 / 👎. |
||
| run: | | ||
| set -euo pipefail | ||
| python3 - <<'PY' | ||
| import json, re, sys | ||
| pins = json.load(open("scripts/unsloth/pr-set.json"))["prs"] | ||
| doc = json.load(open("scripts/unsloth/feature-checks.json")) | ||
| owned = {f["owner"] for f in doc["features"].values() if f.get("owner")} | ||
| known = owned | set(doc.get("unchecked", {})) | ||
| fail = 0 | ||
| for entry in pins: | ||
| url = entry if isinstance(entry, str) else entry["url"] | ||
| m = re.match(r"https://github\.com/([^/]+)/llama\.cpp/pull/(\d+)/", url) | ||
| pin = f"{m.group(1)}#{m.group(2)}" | ||
| if pin not in known: | ||
| print(f"::error file=scripts/unsloth/feature-checks.json::{pin} is pinned " | ||
| "and appears in neither `features` nor `unchecked`; say which it is") | ||
| fail = 1 | ||
| for pin in sorted(owned & set(doc.get("unchecked", {}))): | ||
| print(f"::error file=scripts/unsloth/feature-checks.json::{pin} is in both " | ||
| "`features` and `unchecked`") | ||
| fail = 1 | ||
| print(f"{len(pins)} pin(s), {len(owned)} with a feature check, " | ||
| f"{len(doc.get('unchecked', {}))} knowingly unchecked") | ||
| sys.exit(fail) | ||
| PY | ||
|
|
||
| # An over-limit run: script makes the whole file uncompilable, and nothing else sees it: yaml, actionlint and GitHub's own parser all pass it. See check_workflow_scalars.py. | ||
| - name: Check no workflow string is near GitHub's size limit | ||
| run: python3 scripts/unsloth/check_workflow_scalars.py --root . | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| { | ||
| "_doc": [ | ||
| "The test that proves each shipped feature works. Read by feature_matrix.py.", | ||
| "", | ||
| "Keyed by FEATURE, with the pin that currently carries it, 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`, and `owner` becomes null rather than the entry", | ||
| "being removed.", | ||
| "", | ||
| "This is the half that cannot be derived. pin_contract.py reads a pin's own", | ||
| "diff and proves the merge kept it, which needs no upkeep but can only ever", | ||
| "prove the MERGE lost nothing -- a regression inside the pin regenerates a", | ||
| "smaller contract that passes. What a feature has to DO is a human sentence.", | ||
| "", | ||
| "Every pin in pr-set.json must appear in `features` or in `unchecked`. The", | ||
| "lint enforces that, so adding a pin forces a decision instead of a silence.", | ||
| "`unchecked` is a recorded reason, not a hole.", | ||
| "", | ||
| "kinds:", | ||
| " arch test-llama-archs -a <arch> builds a synthetic model of", | ||
| " the architecture, decodes 128", | ||
| " tokens on every device and", | ||
| " compares against CPU", | ||
| " backend-op test-backend-ops test -o <OP> runs the op against the CPU", | ||
| " reference implementation", | ||
| " mtmd test-mtmd-impl projector registry, no model", | ||
| "", | ||
| "A probe that exits 0 having run nothing is a failure, not a pass: both", | ||
| "harnesses do exactly that for an excluded arch or a misspelled op name.", | ||
| "feature_matrix.py rejects skip markers and requires a non-zero case count.", | ||
| "", | ||
| "No runner in the prebuild pipeline has a GPU, so the nightly runs this on", | ||
| "CPU and every backend-op check is DEFERRED there: named and counted, never", | ||
| "reported as passing. The kernels are exactly where a merge goes wrong", | ||
| "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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a carry-PR author follows this documented command—the stated only place where backend-op checks run— Useful? React with 👍 / 👎. |
||
| "", | ||
| "and paste the output into the PR. That is the only place those checks run." | ||
| ], | ||
| "schema": 1, | ||
| "features": { | ||
| "inkling": { | ||
| "owner": "ggml-org#25731", | ||
| "checks": [ | ||
| { "kind": "arch", "arch": "inkling" }, | ||
| { "kind": "backend-op", "op": "FLASH_ATTN_EXT_BANDED" }, | ||
| { "kind": "mtmd", "projector": "inkling" } | ||
| ] | ||
| }, | ||
| "glm5next": { | ||
| "owner": "ggml-org#27754", | ||
| "checks": [ | ||
| { "kind": "arch", "arch": "glm5next" }, | ||
| { "kind": "backend-op", "op": "LIGHTNING_INDEXER" } | ||
| ] | ||
| }, | ||
| "diffusion-gemma": { | ||
| "owner": "ggml-org#24423", | ||
| "checks": [ | ||
| { "kind": "arch", "arch": "diffusion-gemma" } | ||
| ] | ||
| }, | ||
| "kimi-k3": { | ||
| "owner": "unslothai#70", | ||
| "checks": [ | ||
| { "kind": "arch", "arch": "kimi-k3" }, | ||
| { "kind": "mtmd", "projector": "kimik3" } | ||
| ] | ||
| }, | ||
| "iq1-narrow-grids": { | ||
| "owner": "unslothai#61", | ||
| "checks": [ | ||
| { "kind": "backend-op", "op": "MUL_MAT", "params": "type_a=iq1_xs" }, | ||
| { "kind": "backend-op", "op": "MUL_MAT", "params": "type_a=iq1_xxs" }, | ||
| { "kind": "backend-op", "op": "MUL_MAT", "params": "type_a=iq1_xxxs" } | ||
| ] | ||
| }, | ||
| "qwen4exp-mtp": { | ||
| "owner": "unslothai#144", | ||
| "checks": [ | ||
| { "kind": "arch", "arch": "qwen4exp" }, | ||
| { "kind": "backend-op", "op": "TOPK_QSA" } | ||
| ] | ||
| }, | ||
| "projector-registry": { | ||
| "owner": "unslothai#176", | ||
| "checks": [ | ||
| { "kind": "mtmd", "projector": "*" } | ||
| ] | ||
| } | ||
| }, | ||
| "unchecked": { | ||
| "unslothai#95": "sampling penalties indexed by token id; behaviour is covered by test-sampling, and there is no feature surface of its own to probe", | ||
| "unslothai#137": "batched readahead for lazily read gather tables; a throughput change with no observable output difference", | ||
| "unslothai#149": "GGML_CUDA_ENABLE_UNIFIED_MEMORY=0 env parsing; needs a CUDA or HIP host, and no runner in the pipeline has one", | ||
| "unslothai#152": "per-run mmap of a context's tensors; a memory-layout change with no observable output difference", | ||
| "unslothai#157": "cudaMemcpyDefault in the ggml_cuda_cpy 2D fast path; needs a CUDA host", | ||
| "unslothai#158": "ROCm_Host compute buffer type on HIP integrated GPUs; needs a ROCm host" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| #!/usr/bin/env python3 | ||
| # SPDX-License-Identifier: AGPL-3.0-only | ||
| # Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. | ||
| """Run the test that proves each shipped feature works, against a built tree. | ||
|
|
||
| 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 one 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. | ||
|
|
||
| Keyed by FEATURE, not by pin. When upstream absorbs a feature and the pin is | ||
| deleted, removing the check with it would put the blind spot back in a | ||
| different place -- the feature is still in the release, it just arrives through | ||
| the base tag now. So the manifest binds a feature to its current pin and | ||
| survives that pin going away. | ||
|
|
||
| A PASS HAS TO BE 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 | ||
| actually ran. Without that this file is decoration. | ||
|
|
||
| CPU only under CUDA_VISIBLE_DEVICES="" is what CI can do, since no runner in | ||
| the prebuild pipeline has a GPU. Run it with the variable unset on a GPU box to | ||
| get the comparison that matters for kernels. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import argparse | ||
| import json | ||
| import re | ||
| import subprocess | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| # Output that means "this did not run" from a process that exited 0. | ||
| SKIP_RE = re.compile(r"\bSKIP\b|not supported|unsupported|no tests|0 tests", re.I) | ||
|
|
||
|
|
||
| class Unproven(Exception): | ||
| """The probe exited 0 without demonstrating anything.""" | ||
|
|
||
|
|
||
| class NeedsGPU(Exception): | ||
| """Nothing is wrong; this check cannot be answered on this machine. | ||
|
|
||
| test-backend-ops compares a backend against the CPU reference, so with no | ||
| accelerator present it has nothing to compare and prints "Skipping CPU | ||
| backend". Reporting that as a pass would be a lie and reporting it as a | ||
| failure would block every nightly, since no runner in the prebuild pipeline | ||
| has a GPU. It is counted and named instead. | ||
| """ | ||
|
|
||
|
|
||
| def bins(build_dir: Path) -> Path: | ||
| for c in (build_dir / "bin", build_dir): | ||
| if (c / "test-backend-ops").exists() or (c / "test-llama-archs").exists(): | ||
| return c | ||
| raise SystemExit(f"no test binaries under {build_dir}") | ||
|
|
||
|
|
||
| def run(cmd: list[str], cwd: Path, gpu: bool) -> tuple[int, str]: | ||
| env = None | ||
| if not gpu: | ||
| import os | ||
| env = dict(os.environ, CUDA_VISIBLE_DEVICES="", HIP_VISIBLE_DEVICES="") | ||
| r = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True, env=env) | ||
| return r.returncode, (r.stdout or "") + (r.stderr or "") | ||
|
|
||
|
|
||
| def probe_arch(check: dict, b: Path, gpu: bool) -> str: | ||
| """A synthetic model of this architecture decodes, and matches CPU.""" | ||
| arch = check["arch"] | ||
| rc, out = run([str(b / "test-llama-archs"), "-a", arch, "-s", "1234"], b, gpu) | ||
| if rc != 0: | ||
| raise Unproven(f"test-llama-archs -a {arch} exited {rc}") | ||
| # The arch's own rows, not the header and not another arch's. | ||
| rows = [ln for ln in out.splitlines() if ln.strip().startswith("|") and f"|{arch:>16}|" in ln | ||
| or (ln.strip().startswith("|") and ln.split("|")[1].strip() == arch)] | ||
| if not rows: | ||
| raise Unproven(f"test-llama-archs printed no row for {arch}; it is not in the harness") | ||
| ok = [r for r in rows if "OK" in r] | ||
| if not ok: | ||
| raise Unproven(f"every {arch} row was skipped, so nothing was decoded: {rows[0].strip()}") | ||
| return f"{len(ok)}/{len(rows)} device rows decoded and matched CPU" | ||
|
|
||
|
|
||
| def probe_backend_op(check: dict, b: Path, gpu: bool) -> str: | ||
| """The op exists in the backend and matches the CPU reference.""" | ||
| if not gpu: | ||
| raise NeedsGPU("test-backend-ops compares against CPU, so with no " | ||
| "accelerator it skips every backend and proves nothing") | ||
| cmd = [str(b / "test-backend-ops"), "test", "-o", check["op"]] | ||
| if check.get("params"): | ||
| cmd += ["-p", check["params"]] | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On a build exposing multiple non-CPU devices or backends, Useful? React with 👍 / 👎. |
||
| if not m: | ||
| raise Unproven(f"{check['op']} produced no test count; the filter matched nothing") | ||
| passed, total = int(m.group(1)), int(m.group(2)) | ||
| if total == 0: | ||
| raise Unproven(f"{check['op']} matched 0 cases; the op name is stale") | ||
| if passed != total: | ||
| raise Unproven(f"{check['op']}: {passed}/{total} passed") | ||
| return f"{passed}/{total} cases matched the CPU reference" | ||
|
|
||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a projector is removed or renamed—especially after its owning pin is repinned or deleted—the registry can remain internally consistent and Useful? React with 👍 / 👎. |
||
| if rc != 0: | ||
| raise Unproven(f"test-mtmd-impl exited {rc}") | ||
| m = re.search(r"assertions\s*:\s*(\d+)", out) | ||
| if not m or int(m.group(1)) == 0: | ||
| raise Unproven("test_projector_registry ran no assertions; the filter matched nothing") | ||
| # The registry test walks the whole enum, so it proves the table is sound. | ||
| # That the specific projector is IN the enum is pin_contract.py's job. | ||
| return f"projector registry intact over {m.group(1)} assertions" | ||
|
|
||
|
|
||
| PROBES = {"arch": probe_arch, "backend-op": probe_backend_op, "mtmd": probe_mtmd} | ||
|
|
||
|
|
||
| def main() -> int: | ||
| ap = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) | ||
| ap.add_argument("--build-dir", required=True) | ||
| ap.add_argument("--feature-checks", required=True) | ||
| ap.add_argument("--only", help="one feature id") | ||
| ap.add_argument("--gpu", action="store_true", | ||
| help="let the probes see the GPU; CI has none, so the default " | ||
| "hides it and the comparison is CPU-only") | ||
| ap.add_argument("--report") | ||
| args = ap.parse_args() | ||
|
|
||
| b = bins(Path(args.build_dir).resolve()) | ||
| doc = json.loads(Path(args.feature_checks).read_text()) | ||
| report: dict = {"gpu": args.gpu, "features": [], "ok": False, "deferred": 0} | ||
| failed = 0 | ||
| deferred = 0 | ||
|
|
||
| for name, feat in sorted(doc["features"].items()): | ||
| if args.only and name != args.only: | ||
| continue | ||
|
Comment on lines
+147
to
+149
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| entry = {"feature": name, "owner": feat.get("owner"), | ||
| "results": [], "problems": [], "deferred": []} | ||
| for check in feat["checks"]: | ||
| kind = check["kind"] | ||
| label = f"{kind}:{check.get('arch') or check.get('op') or check.get('projector')}" | ||
| try: | ||
| if kind not in PROBES: | ||
| raise Unproven(f"unknown check kind {kind!r}") | ||
| entry["results"].append({"check": label, "evidence": PROBES[kind](check, b, args.gpu)}) | ||
| except NeedsGPU as e: | ||
| entry["deferred"].append(f"{label}: {e}") | ||
| deferred += 1 | ||
| except Unproven as e: | ||
| entry["problems"].append(f"{label}: {e}") | ||
| except OSError as e: | ||
| entry["problems"].append(f"{label}: cannot run: {e}") | ||
| report["features"].append(entry) | ||
| if entry["problems"]: | ||
| failed += 1 | ||
| print(f"FAIL {name}", file=sys.stderr) | ||
| for p in entry["problems"]: | ||
| print(f" {p}", file=sys.stderr) | ||
| elif entry["results"]: | ||
| print(f"ok {name}: " + "; ".join(r["evidence"] for r in entry["results"]) | ||
| + (f" [{len(entry['deferred'])} needs a GPU]" if entry["deferred"] else "")) | ||
| else: | ||
| # Nothing was shown either way. Not a failure here, but it must not | ||
| # read as one of the ok lines. | ||
|
Comment on lines
+175
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a manifest entry has Useful? React with 👍 / 👎. |
||
| print(f"-- {name}: nothing provable without a GPU " | ||
| f"({len(entry['deferred'])} check(s) deferred)") | ||
|
|
||
| for pin, why in sorted(doc.get("unchecked", {}).items()): | ||
| print(f"note {pin} has no runtime check: {why}") | ||
|
|
||
| report["ok"] = failed == 0 | ||
| report["deferred"] = deferred | ||
| if args.report: | ||
| Path(args.report).write_text(json.dumps(report, indent=2)) | ||
| if failed: | ||
| print(f"\n{failed} feature(s) could not be shown to work", file=sys.stderr) | ||
| return 1 | ||
| # Say what was NOT proven in the same breath as what was. A run that only | ||
| # ever prints a success line teaches the reader that green means covered. | ||
| tail = f", {deferred} check(s) need a GPU and were not run" if deferred else "" | ||
| print(f"\nall {len(report['features'])} features demonstrated" | ||
| + (" on GPU" if args.gpu else " on CPU") + tail) | ||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When one of these probes fails, this preflight only appends to
PROBLEMSand sends an alert; the release workflow.github/workflows/unsloth-prebuilt.ymlstill runs its existingllama-only compile gate and never invokesfeature_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 👍 / 👎.