unsloth: repin DiffusionGemma and Inkling to their conflict-fixed heads - #230
Merged
Merged
Conversation
Both pins have been stale since upstream ggml-org#29042 landed on 09-18. That commit taught the model saver to write the SWA pattern and removed 15 architectures from llama_model_saver_supports_arch(), which is the same block both pins add their architecture to. Upstream deleting text a pin edits is not a pure add/add, so additive_merge.py refuses it and the resolve step fails. Every nightly from 09-19 on has died there. Both PR branches now carry a merge of current upstream master with the conflicts resolved, so the pins move to those heads: ggml-org#24423 12e0a96 -> 3dac51d ggml-org#25731 946fc11 -> 3870aa4 The resolutions keep upstream's deletion and keep each architecture excluded from the saver for the reason that is still true: add_kv_from_model does not write diffusion.canvas_length or the inkling.* hparams, so a saved model of either cannot be loaded back. The old comments blamed the SWA pattern, which upstream has now fixed. Verified: the full pin set merges onto b11078 with no refusal (ggml-org#24423 clean, ggml-org#25731 additive), pin_contract.py reports all 13 pins intact, and both architectures build and pass test-llama-archs on CUDA and CPU.
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The nightly prebuild has failed at Resolve tag every run since 09-19. Both pins below stopped merging onto the base tag, and one refusal aborts the whole merge, so nothing is built.
What broke
Upstream ggml-org#29042 (landed 09-18) taught the model saver to write the SWA pattern and removed 15 architectures from
llama_model_saver_supports_arch(). That is the same block both pins add their own architecture to. Upstream deleting text that a pin edits is not a pure add/add, soadditive_merge.pyrefuses it, exactly as designed:ggml-org#25731had two further collisions: it and upstream both claimed vocab pre-type id 59, and upstream rewrote the sparseindicesindex math in the same two lines where the pin added its banded-bias pointer.What changed here
Both PR branches now carry a merge of current upstream master with those conflicts resolved, so the pins move to the new heads:
12e0a963dac51d946fc113870aa4The resolutions keep upstream's deletion and keep each architecture excluded from the saver for the reason that is still live:
add_kv_from_modeldoes not writediffusion.canvas_lengthor theinkling.*hparams, so a saved model of either cannot be loaded back. The old comments blamed the SWA pattern, which upstream has now fixed. Inkling's vocab pre-type moves to the next free id, which is safe because the id is internal and the GGUF carriestokenizer.ggml.preas a string.Verification
b11078with no refusal: DiffusionGemma ggml-org/llama.cpp#24423 clean, Add TML Inkling architecture ggml-org/llama.cpp#25731 additive, everything after unchanged.pin_contract.pyreports all 13 pins intact in the merged tree.test-llama-archs:diffusion-gemmaNMSE vs CPU 3.66e-07 on B200 and 0.00e+00 on CPU,inkling1.98e-07 and 0.00e+00.diffusiongemma-26B-A4B-it-Q4_K_Mat 200.6 tok/s throughllama-diffusion-cli, andInkling-Small-UD-IQ1_Sat about 70 tok/s throughllama-cliwith flash attention on, matching its output with flash attention off.