unsloth: unbreak the pin set on b10775 - #174
Conversation
…nd it
additive_merge.py refuses a conflict when both sides add a line the other
also adds, on the grounds that one construct added twice would be duplicated
by a union. Two independent case arms in the same switch always share their
scaffolding, so that check fires on `{` and `} break;` and refuses exactly
the add/add it exists to resolve.
That is what stopped the 09-02 nightly on its last pin:
refused tools/mtmd/clip.cpp: both sides add the same line(s),
so this is one change made twice: {, } break;
where one side added a PROJECTOR_TYPE_KIMIK3 arm and the other a
PROJECTOR_TYPE_DEEPSEEK4V one, with no line of actual content in common.
Compare the sides on their identifying lines instead: braces, brackets,
parens, semicolons and commas around at most one bare block-terminating
keyword are scaffolding and carry no identity. `break;` is scaffolding,
`return true;` is not, and anything naming a type, constant or function is
not. Two arms that really are the same change still share their case label
and their body, so a genuine duplicate is still refused.
A side whose addition is nothing but scaffolding now refuses too: with the
scaffolding discounted there is no content left to tell the two additions
apart, so unioning would be a guess.
Discounting the braces is not enough on its own. Once upstream landed DEEPSEEK4V, the KIMIK3 arm and the DEEPSEEK4V arm of the same switch both set `hparams.rope_theta = 10000.0f;`, so the shared-line check refuses on a coincidence: conflict tools/mtmd/clip.cpp: both sides add the same line(s), so this is one change made twice: hparams.rope_theta = 10000.0f; Two arms of one switch labelled differently are two constructs, whatever lines their bodies have in common, so when both sides add case arms and no label appears on both sides, the union is the resolution. The labels are the proof, and they are also what keeps the duplicate check working: the same change made twice keeps its label, so it never reaches this branch and is still refused. A duplicated label would not compile.
Five pins stopped merging. Two of them had been overtaken by upstream, one was carrying work the base tag now has, one is not ours to fix, and one is obsolete. ggml-org#25731 -> #172 carried onto b10775; the head repo is ggml-org, so the repin bot cannot touch it #70 refreshed onto b10775 #91 -> #171 merged into fork master, so it cannot be repinned; re-carried onto b10775 #154 dropped, upstream deleted the function ggml-org#28133 dropped, b10775 contains it The whole set now merges onto b10775 with eleven clean merges and one additive one, and the merged tree passes merge_checks.py.
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. |
|
Every op, so the pins that add their own ( |
repin.py writes its new pins back into scripts/unsloth/pr-set.json, and running it here to reproduce the failing conflict left that file modified. It has nothing to do with this change, and the pins it wrote point at local merge commits that were never pushed, so the lint correctly rejected them. The pin changes live in their own PR.
|
Pushed three more pin changes, all of which came out of making the shipped features testable.
Replayed onto
That is the case #170 fixes, arrived at from a completely different direction than the one that motivated it. This PR now depends on #170, where it did not before. Then on the merged tree, CUDA That is the first time any of these has been shown to work in the composed tree rather than one PR at a time. DiffusionGemma is the one exception and is recorded as such rather than quietly missing: the fixture is in place and the model builds, but the backend comparison aborts inside the arch's own graph input on a null |
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.
|
The base tag moved to Five pins needed work to survive the move. Four of the five merge without a single conflict and produce a tree that does not compile, which is the case the compile gate in #175 exists for, and three of those four are the same upstream change.
The Verified on b10786, with the exact pins listed hereAll 13 merge: 11 clean, 2 additive. The two additive ones need #170, since #172 and #177 both add a fixture block to That is
CUDA The tree I built and ran that on is byte-identical to the tree these pins produce, so the run belongs to this pin set and not to a scratch copy of it. |
…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.
The exclusion blamed the arch's graph input for a null self_kq_mask buffer. It was the fixture: a scalar sliding-window pattern makes every layer sliding, so the plain mask is built and used by nothing and never gets allocated. With a per-layer pattern the arch decodes and matches CPU on all three devices.
The preflight on this branch reports that #172 does not merge onto b10786, and it is right: #172 and #177 both add a fixture block to tests/test-llama-archs.cpp and share exactly one line, `}`, which master's additive_merge.py reads as the same change made twice. #170 fixes that and lands first. Merging it here so the preflight on this branch tests the combination that will actually exist on master, rather than a state nobody will ever run.
![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: b454748037
ℹ️ 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".
| ident = [ln for ln in nonblank(lines) if not STRUCTURAL.match(ln)] | ||
| if not ident or not CASE_LABEL.match(ident[0]): | ||
| return None | ||
| return {ln for ln in ident if CASE_LABEL.match(ln)} |
There was a problem hiding this comment.
When both branches add the same one-line arm with different bodies (for example, case FOO: return 1; versus case FOO: return 2;) or merely format the label differently (case FOO: versus case FOO :), this set contains the entire lines, so the sets appear disjoint. resolve_region then unions them and stages a duplicate case FOO instead of refusing the conflicting implementations; the same problem occurs with differently implemented one-line default: arms. Extract and normalize only the label portion before applying the disjointness shortcut.
Useful? React with 👍 / 👎.
1 participant
The nightly has been failing since 09-02, and by
b10775five of the fourteen pins were broken. Run 33691388002 stopped at the first of them:resolvestops at the first failure, so the other four were invisible. Each one is a different way for a pin to rot, and each needed a different fix.unslothai#70SSM_A_NOSCANchange as ggml-org#28121, so the tip commit re-applied it and every merge was an edit/edit conflictb10775, took upstream's line,883f2c9baunslothai#91NGRID_*defines now sit on a line upstream has edited95e08993aggml-org#25731fattn-mma-f16.cuh; the head repo isggml-org, so the repin bot cannot touch it5b836b6f1ggml-org#27754n_kv_maxtobuild_attn_mhaand this PR's newbuild_attn_sparsestill calls the old signatureaed95865bunslothai#154for_each_token_in, the function it optimisesggml-org#28133b10775contains it; upstream squashed, so the pinned commit is not an ancestor and the merge re-applies code the base already hasggml-org#27754is the one worth reading twice. It is the failure mode the pr-set doc warns about in a form nothing in the pipeline looks for: the merge is clean,additive_merge.pyis never invoked,merge_checks.pypasses, and the release only dies at compile time in the CUDA job, forty minutes and thirty-nine jobs later.Verification
Replayed the whole set onto
b10775exactly the way theresolvestep does, with the resolver frommasterrather than the one in #170, so this stands on its own:Eleven clean, one additive. Then on the merged tree:
CUDA
sm_100, B200, CUDA 13.1. The pin lint passes with no errors; the only notes are the two below.Two things left alone deliberately
unslothai#107is closed, and the resolve log reads that as "upstream declined it". It did not: ggml-org#24423 (DiffusionGemma) is still open, and #107 is our own carry branch, which is closed rather than merged because that is what carry branches are for. The pin merges clean and ships work upstream has not rejected, so it stays. The warning text is what is wrong, not the pin.unslothai#144has moved one commit past its pin (b76199698, "qwen4exp mtp: trim comments"). It is comment-only as far as I can see, but repinning past a reviewed commit is the one thing the pin file exists to prevent, so that is a call for whoever reviews #144, not for this PR.