Carry ggml-org#25731 (TML Inkling) merged onto b10775 - #172
Carry ggml-org#25731 (TML Inkling) merged onto b10775#172danielhanchen wants to merge 23 commits into
Conversation
Hybrid attention model: 55 sliding-window plus 11 global layers, banded content-dependent relative position bias instead of RoPE, per-layer short convolution state, fine-grained MoE (256 experts top-6 plus 2 shared), attention log-scaling past 128K, 1M context. Includes the GGML_OP_FLASH_ATTN_EXT_BANDED operator (CPU and CUDA, fused into the MMA flash attention kernel with an fp16 accumulator overflow guard), HF to GGUF conversion, chat template with typed content block parsing (interleaved thinking, narration and tool calls), mmproj vision and audio support, and backend op tests at production shapes.
# Conflicts: # ggml/src/ggml-cuda/mmq.cuh # src/llama-model-saver.cpp # src/llama-vocab.h
Upstream 910196f renamed common_chat_params::thinking_end_tag to thinking_end_tags and changed it from std::string to a vector, so data.thinking_end_tag = END_MESSAGE no longer compiles.
# Conflicts: # src/llama-arch.cpp
# Conflicts: # ggml/include/ggml-rpc.h # ggml/src/ggml-backend-meta.cpp # ggml/src/ggml-cuda/ggml-cuda.cu # src/llama-model-saver.cpp # tests/test-llama-archs.cpp
# Conflicts: # src/llama-model.cpp
# Conflicts: # ggml/src/ggml-rpc/ggml-rpc.cpp
# Conflicts: # ggml/src/ggml-backend.cpp # ggml/src/ggml-cuda/fattn-mma-f16.cuh # tests/CMakeLists.txt
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: 5b836b6f1d
ℹ️ 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".
| test_cases.emplace_back(new test_flash_attn_ext_banded(128, 8, 1, 512, 8192, 1024, 1, GGML_TYPE_F16, GGML_TYPE_F32)); | ||
| test_cases.emplace_back(new test_flash_attn_ext_banded(128, 8, 1, 512, 16384, 1024, 1, GGML_TYPE_F16, GGML_TYPE_F32)); | ||
| test_cases.emplace_back(new test_flash_attn_ext_banded(128, 8, 1, 512, 16403, 1024, 1, GGML_TYPE_F16, GGML_TYPE_F32)); | ||
| test_cases.emplace_back(new test_flash_attn_ext_banded(128, 8, 1, 512, 16896, 1024, 1, GGML_TYPE_F16, GGML_TYPE_F32)); | ||
| test_cases.emplace_back(new test_flash_attn_ext_banded(128, 8, 1, 512, 17024, 1024, 1, GGML_TYPE_F16, GGML_TYPE_F32)); |
There was a problem hiding this comment.
Every normal test-backend-ops run now executes these stress-sized cases for each applicable backend and its reference; the 32768-token case alone reports about 68.7 billion FLOPs. In the inspected Release CPU build, even the 8192-token case took approximately 20 seconds, so the remaining near-duplicate 16K-32K cases can add several minutes and risk the 900-second CI timeout on slower runners. Keep representative small coverage here and move the boundary stress cases to an opt-in suite.
AGENTS.md reference: AGENTS.md:L87-L87
Useful? React with 👍 / 👎.
| add_test(NAME test-flash-attn-bias COMMAND test-flash-attn-bias) | ||
|
|
||
| add_executable(test-flash-attn-generic-hash test-flash-attn-generic-hash.cpp) | ||
| target_link_libraries(test-flash-attn-generic-hash PRIVATE ggml) | ||
| add_test(NAME test-flash-attn-generic-hash COMMAND test-flash-attn-generic-hash) |
There was a problem hiding this comment.
The inspected CI entry points invoke CTest with -L main, but these raw add_test calls assign no label, unlike the repository's llama_test helper. Consequently neither new regression test runs in the standard CI suite; register them through the existing helper or explicitly assign the main label.
AGENTS.md reference: AGENTS.md:L87-L87
Useful? React with 👍 / 👎.
| uint64_t fnv = UINT64_C(1469598103934665603); | ||
| const uint8_t * bytes = reinterpret_cast<const uint8_t *>(result.data()); | ||
| for (size_t i = 0; i < result.size()*sizeof(result[0]); ++i) { | ||
| fnv ^= bytes[i]; | ||
| fnv *= UINT64_C(1099511628211); | ||
| } |
There was a problem hiding this comment.
When this executable is run without an output path, as configured by CTest, it only prints the computed hash and then returns success. Any numerical regression that still completes therefore passes the test, so this adds test-suite cost without validating the advertised generic flash-attention behavior; compare against an expected hash or a reference result and fail on mismatch.
AGENTS.md reference: AGENTS.md:L87-L87
Useful? React with 👍 / 👎.
|
Full That is every op, not just flash attention, so the two add/add resolutions in |
The arch was enumerated and then skipped:
if (arch == LLM_ARCH_INKLING) {
return false; // TODO fixture params for the arch-specific hparams
}
so nothing has ever built an Inkling model, and the row it printed said SKIP
rather than saying nothing, which is the worst of both.
load_arch_hparams reads d_rel, rel_extent, rel_extent_swa, shortconv_kernel,
dense_block_count and logit_scale_denom unconditionally and then GGML_ASSERTs
four of them non-zero, so all six have to be in the fixture; the three optional
log-scaling and unpadded-vocab keys go in with them so the fixture describes a
whole model rather than the minimum that loads.
MoE-mandatory, not MoE-implemented: expert_feed_forward_length,
expert_shared_count and expert_weights_scale are read unconditionally too, so
there is no expert-free config of this arch to build. Dense and MoE are a
per-layer split decided by dense_block_count, not a model-level choice.
Now runs a real forward pass, and on CUDA it is a real comparison:
| Model arch.| Device|Config| NMSE vs. CPU|Roundtrip|
| inkling| NVIDIA B200| MoE| OK (1.82e-11)| SKIP|
| inkling|Xeon 8559C | MoE| OK (0.00e+00)| SKIP|
Roundtrip stays SKIP because llama_model_saver_supports_arch already lists
INKLING as unsupported; that is this PR's own decision and is untouched here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cd60b7852
ℹ️ 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".
| self.gguf_writer.add_float32(f"{arch}.logit_scale_denom", hp["logits_mup_width_multiplier"]) | ||
| self.gguf_writer.add_uint32(f"{arch}.log_scaling_n_floor", int(hp.get("log_scaling_n_floor") or 0)) | ||
| self.gguf_writer.add_float32(f"{arch}.log_scaling_alpha", hp.get("log_scaling_alpha", 0.0)) | ||
| self.gguf_writer.add_uint32(f"{arch}.unpadded_vocab_size", hp["unpadded_vocab_size"]) |
There was a problem hiding this comment.
When an Inkling config omits unpadded_vocab_size, get_vocab_base() explicitly falls back to vocab_size, but this direct lookup later raises KeyError and aborts conversion. Use the same fallback here (and in the following log statement) so checkpoints without this optional field can be converted consistently.
Useful? React with 👍 / 👎.
The merge is conflict-free and the tree does not compile, in two ways.
n_ff_exp became a per-layer array behind an accessor:
error: invalid use of non-static member function
'uint32_t llama_hparams::n_ff_exp(uint32_t) const'
Read into n_ff_exp_arr and take the value through the accessor, the same
way the other MoE archs do since that change. Inkling's expert FF length
is uniform across layers, so the layer-0 value is every layer's value.
The image and audio preprocessor base classes made preprocess() const:
error: 'mtmd_image_preprocessor_inkling::preprocess(...)'
marked 'override', but does not override
Both Inkling overrides only read the cache; the writes are in
initialize(), where the base class already puts them, so const is
accurate rather than a cast.
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.
|
Refreshed onto What the move required is in the merge commit message. Verified with the whole 13-pin set replayed on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f639c770a0
ℹ️ 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".
| model_arch = gguf.MODEL_ARCH.INKLING | ||
| undo_permute = False | ||
|
|
||
| _SKIP_PREFIXES = ("model.visual.", "model.audio.", "model.mtp.") |
There was a problem hiding this comment.
Checkpoints that store the combined towers as visual.* or audio.* cannot be converted as text: the mmproj converter explicitly accepts those bare prefixes, but this filter skips only model.visual.* and model.audio.*, so the text converter passes the tower tensors to map_tensor_name() and aborts with Can not map tensor. Include the bare prefixes here as well.
Useful? React with 👍 / 👎.
| mtmd_inkling_image_preproc_out mtmd_image_preprocess_inkling( | ||
| const clip_image_u8 & img, | ||
| resize_algo algo) { |
There was a problem hiding this comment.
A repo-wide search finds no caller of this newly declared helper outside its declaration and definition; the runtime instead uses mtmd_image_preprocessor_inkling::preprocess() below. The two implementations already disagree on resize selection and BF16 rounding, so retaining this second preprocessing path adds substantial dead code and an ambiguous API that can produce different embeddings if used later; keep only the runtime implementation or make it reuse this helper.
AGENTS.md reference: AGENTS.md:L80-L80
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.
|
Retired. Everything this carry held beyond a replay of ggml-org#25731 is now in the upstream PR itself:
Verified on the merged pin set against b10795: The pin in |
2 participants
Carries ggml-org#25731 (TML Inkling) onto
b10775so the nightly has something current to pin.The pin in
scripts/unsloth/pr-set.jsonis44eb88e9a, which forked fromdaef7b687and last moved on 08-31. It stopped merging onto the base tag:The head repository is
ggml-org, so the repin bot correctly leaves it alone. This is our own carry branch instead, base branchbase/upstream-67a17c17c, which isb10775verbatim.The collision
Upstream added sparse attention to the MMA flash attention kernel in the same weeks ggml-org#25731 added banded relative bias to it. Both thread an extra parameter through
flash_attn_ext_f16_iterandflash_attn_ext_f16_process_tile, and they land in the same 13 places:const int32_t * indicesparameter, ause_sparsetemplate argument, and splitsKV_max_ptrintoKV_max/sparse_indicesconst float * rel_f,rel_extent,head_q0andne11, and derivesbanded_biasfrommax_bias < 0.0fEvery one of the 13 is a union of two disjoint parameter additions. The parameter order is not a choice:
flash_attn_ext_f16_process_tile's own declaration merged without a conflict, asmask_h, indices, sinks_f, rel_f, so upstream's addition goes first and every call site follows it.The one hunk that is not purely mechanical is
sinks_f:ggml-org#25731 overloads the
sinkspointer to carry the relative bias table whenmax_biasis negative, so its narrowersinks_fhas to win; upstream's version would feed the bias table to the sink path.indicesreadssparse_indices, which nothing else touches, so the three locals are independent.ggml/src/ggml-backend.cppandtests/CMakeLists.txtalso conflicted. Both are pure add/add and were resolved byscripts/unsloth/additive_merge.py: aGGML_OP_FLASH_ATTN_EXT_BANDEDcase next to upstream'sGGML_OP_MUL_MATone, and the two new test targets next to upstream's metallib test block.Testing
Built CUDA,
sm_100, B200, CUDA 13.1.That covers each feature against the CPU reference: the 2948 cases exercise upstream's sparse and view paths, the 13 banded cases and
test-flash-attn-biasexercise ggml-org#25731's. What it does not cover is the two features active in the same kernel launch, because no test builds a banded op with a sparse index list; that combination does not exist in either branch's test set and nothing in the merge creates it, sincebanded_biasanduse_sparsecome from different inputs.git diff b10775..HEADis 64 files, +4080/-124, the ggml-org#25731 delta.