unsloth: prove the merged tree still has the features, and that it compiles - #175
Conversation
…mpiles
The nightly proves the pins merged. It has never proved they are in the
release, and the gap has cost three outages:
ggml-org#28133 upstream squashed it, so the pinned commit stopped being an
ancestor and the merge re-applied work the base already had.
It happened to conflict, which is the only reason anybody
noticed. Merging quietly would have shipped nothing.
ggml-org#27754 merged with zero conflicts and did not compile. Upstream had
added n_kv_max to build_attn_mha; the pin's new
build_attn_sparse still called the old signature. The
release died in the CUDA leg after the 38-job fan-out.
08-27 a resolution that kept the wrong side and compiled fine.
Three additions, cheapest first.
pin_contract.py derives from each pin's own diff what it puts in the tree,
then checks the merged tree still has it: every LLM_ARCH_/GGML_OP_/
PROJECTOR_TYPE_/... name, per FILE and not per tree, because the failure being
looked for is exactly LLM_ARCH_INKLING surviving in llama-arch.h while its
dispatch arm is gone; every file the pin adds; and every non-comment code line
it adds, which catches a hunk eaten without touching a symbol. Nothing to
maintain: the expectation comes out of the commit, so a repin regenerates it.
It also reports, never fatally, a pin whose added lines the base tag already
has. That is pr-set.json's retirement rule mechanised, and it needs to be text
rather than ancestry because upstream squashes. The separation is not close:
measured at b10775 the pin upstream had absorbed scored 99% and the highest
live pin scored 33%.
Not fatal, deliberately: upstream landing a feature overnight must not stop
that night's release.
A CPU compile gate in resolve, before the fan-out. `llama` target only, which
is where llama-graph.cpp lives, and CPU only: 59 seconds cold at -j4 with no
ccache, against 20 to 60 minutes for CUDA.
The preflight gets both, seven hours ahead of the nightly on a throwaway
runner, appending to the PROBLEMS accumulator that already feeds the alert.
resolve now copies all of scripts/unsloth/ to $RUNNER_TEMP in one line instead
of one cp per script. That block is within a few hundred characters of the
21000 cap, where going over silently disables the entire workflow, and this
leaves it 121 characters smaller than before while making every future check
free. The pin list reaches the checker through env, not ${{ }}: it carries PR
titles, which are third-party text.
What this cannot do, so nobody reads more into a pass than is there: the
contract is re-derived from the pin, so it only proves the MERGE lost nothing.
A regression inside the pin regenerates a smaller contract that passes, and
proving a feature runs needs a build and a test.
|
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: 7828fbc0cf
ℹ️ 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/pin_contract.py --root . --base "$BASE" \ | ||
| --pr-set ../scripts/unsloth/pr-set.json --report "${RUNNER_TEMP}/pin_contract.json" ; then |
There was a problem hiding this comment.
When the set contains a closed optional pin and at least one other pin merges, the loop above skips the optional pin, but this invocation reloads the entire pr-set.json. pin_contract.py consequently checks code that was deliberately not merged and reports it missing, turning an otherwise successful preflight into a failure and triggering the repin automation. Pass an effective pin list here, as the nightly does with --prs-json.
Useful? React with 👍 / 👎.
| stripped = text.strip() | ||
| if len(stripped) >= MIN_LINE and not TRIVIAL_RE.match(stripped): | ||
| lines[cur].append(stripped) |
There was a problem hiding this comment.
When a pin adds a substantial code line with a trailing comment and a later valid resolution changes only that comment, the contract still contains the original comment because the line is appended before COMMENT_RE is applied. With the default threshold of 1.0, this rejects a merged tree whose executable code is unchanged, despite the stated policy that comments are not part of the contract.
Useful? React with 👍 / 👎.
| text = target.read_text(errors="replace") | ||
| lost = [w for w in wanted if w not in text] |
There was a problem hiding this comment.
When a bad merge resolution comments out a pin-added statement rather than deleting it, the raw file still contains the wanted string, so w in text counts the disabled statement as surviving. Such a tree can compile while silently losing the feature, which is the exact failure this gate is intended to catch; compare against comment-stripped executable text instead.
Useful? React with 👍 / 👎.
The base tag moved from b10775 to b10786 while this was open, and five pins needed work to survive it. Four of the five merge without a single conflict and produce a tree that does not compile, which is the failure mode the compile gate in #175 exists for; three of those four are the same upstream change. - #172 inkling, #173 glm5next, #177 diffusion-gemma: n_ff_exp became a per-layer array behind an accessor. Reading the old scalar field is a compile error, and inside a create_tensor dimension list the member function quietly decays to a pointer-to-member instead. #172 and #173 also override preprocess(), which the mtmd base classes made const. - #152 per-run buffers: b10786 added a load-ordering pass that reads a llama_buf_map entry as one buffer, and this pin made an entry a run of buffers. - #144 qwen4exp MTP: the only one that conflicts, in both places it touches, over the same n_ff_exp change. Verified on b10786: all 13 pins merge (11 clean, 2 additive), the CPU llama target builds, and the CUDA build plus the feature matrix are in the PR comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c21aa4324c
ℹ️ 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".
| -DLLAMA_BUILD_EXAMPLES=OFF -DLLAMA_BUILD_TOOLS=OFF -DLLAMA_CURL=OFF > /dev/null \ | ||
| || ! cmake --build "${RUNNER_TEMP}/gate" --target llama -j "$(nproc)" ; then |
There was a problem hiding this comment.
When a pin changes tools/mtmd and only that target fails to compile, this preflight remains green because it disables tools and builds only llama, while the compile gate in .github/workflows/unsloth-prebuilt.yml explicitly builds both llama and mtmd. This defeats the earlier preflight for the exact class of mtmd failure described in the nightly workflow, so the problem is discovered only when the nightly starts. CMake's local help confirms the semantics: --target <tgt>... builds the named targets instead of the defaults.
Useful? React with 👍 / 👎.
| for path, wanted in sorted(contract["lines"].items()): | ||
| target = root / path | ||
| if not target.is_file(): | ||
| continue # already reported, or a deletion |
There was a problem hiding this comment.
When a bad resolution deletes a pre-existing file that the pin modified, and that file contains no identifier from SYMBOL_FAMILIES, this branch skips all of its line-survival checks. Only files classified as newly added are reported above, so the comment's “already reported” assumption is false for modified files and the pin can pass despite losing every line it contributed to that file.
Useful? React with 👍 / 👎.
| if total and hit / total >= args.redundant_at: | ||
| note = (f"the base tag already has {hit}/{total} ({hit / total:.0%}) of the " | ||
| "lines this pin adds; upstream has taken this work and the entry " | ||
| "should be deleted from pr-set.json") |
There was a problem hiding this comment.
When upstream merged the pinned SHA without squashing and a later base contains it as an ancestor, merge-base(pin, base) is the pin itself, so the derived diff has zero lines and total is zero. This condition then suppresses the retirement notice while check() has an empty contract and passes, leaving exactly the stale, no-op pin this checker says it should identify; handle ancestry or an empty contract before requiring a nonzero line total.
Useful? React with 👍 / 👎.
1 participant
The nightly proves the pins merged. It has never proved they are in the release, and that gap has cost three outages:
ggml-org#28133ggml-org#27754n_kv_maxtobuild_attn_mha; the pin's newbuild_attn_sparsestill called the old signature. The release died in the CUDA leg after the 38-job fan-out.merge_checks.pyexists because of this one.merge_checks.pyasserts the absence of two known-bad shapes. Nothing anywhere asserts the presence of anything, and nothing compiles the merged tree before thirty-eight build jobs start.scripts/unsloth/pin_contract.pyDerives from each pin's own diff what it puts in the tree, then checks the merged tree still has it. Nothing to maintain: the expectation comes out of the commit, so a repin regenerates it.
LLM_ARCH_/GGML_OP_/PROJECTOR_TYPE_/GGML_TYPE_/LLAMA_FTYPE_/LLM_KV_/LLM_TENSOR_/LLM_TYPE_name the pin introduces, per file, not per tree. That distinction is the whole point:LLM_ARCH_INKLINGsurviving inllama-arch.hwhile its dispatch arm is gone fromllama-model.cppis exactly the failure being looked for, and a tree-wide grep passes it.Comments are stripped before anything is read off a line.
unslothai#70has a comment namingGGML_OP_SSM_SCANto explain why it does not use it; holding comment wording as a contract fails the moment upstream rewords it.On redundancy
A pin whose added lines the base tag already has is work upstream has taken. That is the retirement rule already written in
pr-set.json's_doc, mechanised, and it has to compare text rather than ancestry because upstream almost always squashes, so no ancestry test will ever say the work landed.Measured on the real set at
b10775, the separation is not close:Threshold 0.95. Not fatal, deliberately: upstream landing a feature overnight must not stop that night's release.
The compile gate
CPU only,
llamatarget only. That is wheresrc/llama-graph.cpplives, which is the translation unit that failed on 09-03. Measured on the mergedb10775tree, cold, ccache disabled, at-j4to match a hosted runner's core count:Against twenty to sixty minutes for a CUDA build. It runs in
resolvebefore the fan-out, and again in the preflight seven hours earlier.Workflow plumbing
resolvenow copies all ofscripts/unsloth/to$RUNNER_TEMP/usin one line instead of onecpper script. That block is within a few hundred characters of GitHub's 21000-character per-string cap, where going over silently disables the entire workflow -- no jobs, no annotations, no error. This leaves it 121 characters smaller than before while making every future check free:The pin list reaches the checker through
env:, never${{ }}inline. It carries PR titles, which are third-party text, and${{ }}pastes them into the shell source before bash sees it.The preflight gets both checks appended to the
PROBLEMSaccumulator that already feedsprebuilt-alert, so findings land in the run summary with no new plumbing.Testing
scripts/unsloth/test_pin_contract.py, added to the lint workflow's resolver-tests loop. Seventeen checks, each building a real repository with a real base tag, a real pin branch and a real merge, then damaging the merged tree the way a bad resolution damages it.Beyond the unit tests, three regressions reproduced against the real
b10775mix:A retired pin is spotted. Running against
master's pin set, which still listsggml-org#28133:A dropped dispatch arm is caught. Deleting the
PROJECTOR_TYPE_KIMIK3builder arm fromtools/mtmd/clip.cpp:Note that one was caught by line survival, not by the symbol check, because
PROJECTOR_TYPE_KIMIK3still appears elsewhere in the same file. That is why both checks exist.The 09-03 failure is caught in a minute. Reverting the
n_kv_maxfix in the merged tree:And on the intact tree, all twelve pins pass:
All six resolver test suites pass, and
check_workflow_scalars.pyis clean.What this does not do
The contract is re-derived from the pin, so it only ever proves the merge lost nothing. A regression inside the pin itself regenerates a smaller contract that passes. Proving a feature actually runs needs a build and a test, and that is the next change: a feature-keyed manifest plus
test-llama-archs/test-backend-opsprobes. Two of the features we ship, DiffusionGemma and Inkling, have no runtime test at all today.