Repository navigation
pr-set: pin the Metal NVFP4 kernels (#330) - #331
Merged
Merged
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. |
# Conflicts: # scripts/unsloth/feature-checks.json # scripts/unsloth/pr-set.json
1 of 5 tasks
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.
Pins #330 by @Lyxot (metal: add NVFP4 mul_mat, mul_mat_id and get_rows kernels) at
3adf93fa5, so the macOS prebuilt runs NVFP4 weights on the GPU instead of silently on the CPU (unslothai/unsloth#15409).Upstream state
Upstream Metal has no NVFP4 kernels.
ggml_metal_supports_mul_mat_op()rejects an NVFP4src0forMUL_MATandMUL_MAT_ID, andGET_ROWSrejects it too, on every tag from b11922 to b11933 and on currentmaster. The loader then places those weights in CPU buffers while still reporting every layer as offloaded. MiniMax-M3.1 UD-Q4_K_XL and UD-Q8_K_XL have all 171 routed-expert tensors in NVFP4, so on a Mac the experts run on the CPU. As #330 notes, ggml-org#20456 adds Metal NVFP4 upstream, but it is not merged and is written against the old single-file shader layout, so it no longer applies.#330 adds the kernels for every path the three ops can take:
mul_mv,mul_mv_ext,mul_mv_id, the few-rowmul_mv_mmakernels,mul_mm,mul_mm_idandget_rows, and drops the NVFP4 rejections insupports_op. Thanks to @Lyxot, who also tested it on an M3 Max.Changes
scripts/unsloth/pr-set.json: appendunslothai/llama.cpp#330 @ 3adf93fa5ae019c153c8c1ace3a698189b7db41dafter vulkan: copy a misaligned top_k input to an aligned scratch instead of aborting #328.scripts/unsloth/feature-checks.json:unslothai#330underunchecked. The behaviour is covered by test-backend-ops MUL_MAT, MUL_MAT_ID and GET_ROWS with-p nvfp4, which need a Metal device, and no runner in the pipeline has one. A backend-op check would pass on CUDA, which already has NVFP4 upstream, so it cannot tell the pin apart.#330 is based on
base/upstream-d854d98ba, an ancestor of every tag simmed below, and only touchesggml/src/ggml-metal. No other pin touches that directory. The pin is dropped once upstream merges ggml-org#20456 or an equivalent.Evidence
Pin set = master + this pin (25 pins).
audit: 24/25 clean, #330 clean, feature-checks coverage ok. The one FIX row is #320 (pin != PR head, stale base), which is the same on master.The full set does not merge on these tags with or without this pin: ggml-org#24423, ggml-org#25731, #241 and #298 are refused on all three, and #251 as well on b11933. On b11925 and b11933 the refused list and the failing contract rows are the same as for master's pin set alone. Those pins need their own repins before any nightly can go green. This PR does not change that.
Because of that, the CPU checks ran on b11925 plus this pin alone:
-b CPU -p nvfp4MUL_MAT / MUL_MAT_ID / GET_ROWSThe Linux CPU build does not compile
ggml/src/ggml-metal, so this mainly shows that the pin breaks nothing else. The Metal evidence is from GitHub macOS arm64 runners (macos-15 and macos-26, Apple Paravirtual GPU), #330's head against its own based854d98ba. Both arms were built from the same source, with a CI-only switch that turns on simdgroup ops on the virtual GPU. The model is a tiny random-weight Qwen3-MoE with Q8_0 weights and NVFP4 routed experts, the same layout as UD-Q4_K_XL.test-backend-ops -b MTL0 -p nvfp4MUL_MATtype_b=f16, same rule as MXFP4)CPU_MappedMTL0_MappedThe KLD and top-token numbers match an independent port of the same kernels to 4 decimals on the same model, and the speed is within runner noise of it. The UE4M3 scale decode (
ue4m3_to_fp32inkernels/common.h) agrees with the CPUggml_ue4m3_to_fp32on all 256 byte values, including 0x7F (decodes to 0) and scale bytes with bit 7 set (ignored on both sides).Not run here: a CUDA build of the merged tree and the GPU feature checks (no local GPU in this session; the pin touches no CUDA source), bare-metal Apple Silicon (covered by the author's M3 Max run), and the ROCm, Vulkan, Windows and ARM legs, which only the nightly builds.