unsloth: compare add/add conflicts on content, not on the braces around it - #170
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.
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: e4169d0296
ℹ️ 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".
| # Both sides added byte-identical text; one copy is the resolution. | ||
| return list(ours) | ||
| shared = set(nonblank(ours)) & set(nonblank(theirs)) | ||
| shared = identifying(ours) & identifying(theirs) |
There was a problem hiding this comment.
When both branches add the same keyed initializer but format its opener differently—for example, ours splits { and "foo", across lines while theirs uses { "foo", and each supplies a different handler—the only exact shared line can be },. identifying() removes that line, so this intersection is empty and the resolver stages both entries; the result compiles but silently duplicates the key or registration. The previous check refused this conflict because of the shared closing line, so structural punctuation should be normalized within content lines rather than discarded before deciding that the additions are independent.
Useful? React with 👍 / 👎.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71d6b39f65
ℹ️ 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".
| return list(ours) | ||
| shared = set(nonblank(ours)) & set(nonblank(theirs)) | ||
| ours_arms, theirs_arms = case_arms(ours), case_arms(theirs) | ||
| if ours_arms and theirs_arms and ours_arms.isdisjoint(theirs_arms): |
There was a problem hiding this comment.
When both branches add the same label with harmless formatting differences, such as case FOO: versus case FOO :, case_arms() stores the complete stripped lines, so these sets appear disjoint and this fast path stages both arms while bypassing duplicate detection. The resulting switch contains two equivalent case values and fails to compile; compare normalized label expressions rather than raw label lines before taking this path.
Useful? React with 👍 / 👎.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 184488ad34
ℹ️ 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".
| return list(ours) | ||
| shared = set(nonblank(ours)) & set(nonblank(theirs)) | ||
| ours_arms, theirs_arms = case_arms(ours), case_arms(theirs) | ||
| if ours_arms and theirs_arms and ours_arms.isdisjoint(theirs_arms): |
There was a problem hiding this comment.
When both additions are fallthrough arms with distinct labels but a shared body line, this fast path concatenates them without checking whether either arm terminates. For example, merging case A: prepare(); and case B: prepare(); immediately before an existing case produces case B: prepare(); case A: prepare();; selecting B now executes prepare() twice, whereas each branch executed it once. Restrict this exception to body-free fallthrough labels or independently terminated arms; otherwise the resolver silently changes runtime behavior.
Useful? React with 👍 / 👎.
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.
1 participant
additive_merge.pyrefuses a conflict when both sides add a line the other side also adds. The reasoning is sound: one construct added twice would be duplicated by a union, and unioning it produces code that does not compile. The implementation compares every non-blank line, which is where it goes wrong on the shape it exists for.Two changes, one to each half of that check.
1. Braces are not evidence
Two independent
casearms in the sameswitchshare their scaffolding by construction. A case arm is a label, a body, and} break;, and the last part is the same whatever the arm does. So the check fires on the braces and refuses a pure add/add.That is what stopped the 09-02 nightly on its last pin:
One side added a
PROJECTOR_TYPE_KIMIK3arm, the other aPROJECTOR_TYPE_DEEPSEEK4Vone, with no line of actual content in common:So the sides are compared on their identifying lines. A line made of braces, brackets, parens, semicolons and commas, around at most one bare block-terminating keyword, is scaffolding and carries no identity:
Deliberately narrow.
break;,},} break;,});are scaffolding.return true;is not, and neither is anything naming a type, a constant or a function.One case is now refused that was not before: if everything one side added is scaffolding, discounting it leaves nothing to tell the two additions apart, so unioning braces onto braces would be a guess.
2. Two case arms may share a body line
Discounting the braces is not enough on its own. Once upstream landed DEEPSEEK4V, both arms set the same
rope_theta, and the check refuses on that coincidence instead: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 that branch and is still refused. A duplicated label would not compile.
Tests
scripts/unsloth/test_additive_merge.pybuilds every case as a real git conflict, so the markers are exactly what git produces. Added:clip.cppshape, two arms sharing{and} break;and nothing else, which must resolve and keep both bodies exactly oncerope_thetabody line, which must still resolveThe pre-existing "one change made twice" case used
break;as its shared line, which is now scaffolding, and thencase FOO:/case BAR:, which the label rule now resolves. It was rewritten around two differently-named helper functions sharing a body line, so it still tests what it was written to test, and there is an assertion that the refusal message names the content line and not the braces.On the preflight check
Dry-run the pin mergesfails here, and it is not this change. It runs against the pin file, which is broken onmasterand is fixed in #174; this branch carriesmaster's copy verbatim. #174 was deliberately verified with the resolver frommasterrather than this one, so the two are independent: the nightly does not need this to go green, and this does not need the nightly to be green.