Repository navigation
pr-set: repin ggml-org#25731 (Inkling) to e1b89cfa0 - #310
Closed
danielhanchen wants to merge 1 commit into
Closed
danielhanchen wants to merge 1 commit into
danielhanchen wants to merge 1 commit into
Conversation
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. |
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
Member
Author
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
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.
Overview
Repins ggml-org#25731 (Inkling) from 534c321 to e1b89cf. 534c321 does not merge on any upstream base from b11760 on, so the nightly fails at the merge step on every tag it can pick now (the preflight on #306 says "Tonight's nightly will fail on base b11761 unless pr-set.json is repinned").
The prebuilt is upstream's aged
b####tag plus the pins inscripts/unsloth/pr-set.json, merged in order (git diff3, thenadditive_merge.pyfor pure additions only). We do not carry a fork: once upstream takes a pin, the pin is deleted.feature-checks.jsonis unchanged (no pin is added, removed or folded).What broke it
Upstream ggml-org#30836 (b684498, first in b11760, "HIP/CUDA: Fix reduction in mmf getting tile widths not devisiable by 64") changes
ggml/src/ggml-cuda/ggml-cuda.cufrom theWARP_SIZEconstant to a per-devicewarp_sizeinggml_cuda_mul_mat_id_needs_sync()andggml_cuda_mul_mat_id(). These are the same lines where ggml-org#25731 adds itsf32_pedanticgate in front of bothggml_cuda_should_use_mmf(...)calls. The base side of that hunk is not empty, soadditive_merge.pyrefuses it and the merge aborts. On master's pr-set at b11785, ggml-org#25731 is REFUSED. It listsparsers.h,sources.cmake,ggml-cuda.cu,llama-arch.*,llama-model-saver.cpp,test-llama-archs.cppandmtmd-audio.hbecause the whole merge aborts, but onlyggml-cuda.cuis a real conflict.Fix (in ggml-org#25731's own branch)
e1b89cf = 534c321 +
git merge --no-ff b11767. This is oneFix merge conflictscommit, with nothing rebased or force-pushed. b11767 was the aged base when the merge was made, so the merge brings no upstream lines onto any base the nightly can still pick.ggml-cuda.cu: upstream'sconst int warp_size = ...lines and itswarp_sizecall arguments are kept verbatim. Only the pin'sf32_pedanticdeclarations and the!f32_pedantic &&prefix on bothggml_cuda_should_use_mmfcalls are re-applied.common/parsers/parsers.h,common/parsers/sources.cmakeandtools/mtmd/mtmd-audio.hare add/add only (upstream's harmony-xml / muse-spark entries next to the pin's inkling entries). Both sides are kept.ggml/src/ggml-cudafor other old call shapes upstream changed (git diff b11767 -- ggml/src/ggml-cuda | grep '^+'). Every remainingWARP_SIZEuse is in the pin's own new kernel code, where the constant is still valid upstream.diff(b11767, e1b89cfa0)is 70 files, +4119/-126, against +4120/-126 for 534c321. The one-line difference is aggml_cuda_should_use_mmfcall that is now on one line, as it is upstream.masteragain (git merge-treeof e1b89cf against master 0a1ef30 is clean).Checks
Merge simulation
llama_pinset.py simwith master's pr-set plus this repin, on a full-history clone:verify --cuda (sm_100, local B200)
A standalone CPU build of e1b89cf also passes:
test-llama-archs140/140 (inkling OK),test-mtmd-implandtest-chat.Inkling numeric gate
The gate uses
unsloth/Inkling-Small-GGUFUD-IQ1_S, wikitext-2-c 512 --chunks 20, B200, with A = 534c321 as the reference. Error bars as printed by llama-perplexity: PPL about ±2.1, KLD ±0.0098, top-1 ±0.53.Under the gate's rule (KLD <= 1e-6, top-1 100%), B and C fail. The whole delta is one upstream kernel change, ggml-org#29720 (623c163, "CUDA: HIP: rms_norm: add variants that load float4 at a time and store x in registers", first in b11691). The conflict resolution adds none of it. Bisected by merging upstream commits into 534c321, each measured against A:
So everything from b11609 to b11767 apart from ggml-org#29720 (including ggml-org#30836 and this merge's conflict resolution) changes nothing in the numbers. ggml-org#29720 changes the RMS norm reduction order. On CUDA it passes
test-backend-ops -o RMS_NORM53/53 and-o RMS_NORM_MUL_ADD37/37. This 1-bit MoE model (sigmoid routing, top 6 of 256 experts, 42 layers) amplifies last-bit changes: turning off CUDA fusion in A's own build (GGML_CUDA_DISABLE_FUSION=1) moves it by a similar amount (KLD 0.208155, top-1 81.569%, PPL 40.885748).ggml-org#29720 has been in every base the nightly has picked since b11691, including the current
b11734-mixrelease. Any base the nightly can pick now carries it, whatever ggml-org#25731 is pinned to.audit --pr-set
After pushing e1b89cf to ggml-org#25731's branch, the ggml-org#25731 row is
ok(base=master, +4119/-126, 70 files). TheFIXrows (#70, #95, #157, #264, #266, #298) show on master's own pr-set too and are not touched here.Interaction with open pin-set PRs
ggml/src/ggml-cuda/fattn-mma-f16.cuhandtests/test-backend-ops.cpp, because it collides with Add TML Inkling architecture ggml-org/llama.cpp#25731's lines in those files. It is refused the same way with the old Inkling pin (b11759, 534c321, where Inkling still merges), so the cause is CUDA: dequantize q4_0/q8_0 K/V tiles in the FA MMA kernel for small multi-sequence batches (pin branch on b11642) #303 against Add TML Inkling architecture ggml-org/llama.cpp#25731, not this repin. It needs a fix in CUDA: dequantize q4_0/q8_0 K/V tiles in the FA MMA kernel for small multi-sequence batches (pin branch on b11642) #303's branch.Not run
unsloth-pin-preflight.yml) on this branch.