Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20827
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 Unrelated FailureAs of commit b1f3acb with merge base 5f6d1aa ( NEW FAILURE - The following job has failed:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@claude review this code |
This PR needs a
|
|
Claude finished @metascroy's task in 4m 22s —— View job Code Review: MLX Q5K/Q6K upgradesI read through the repack paths, native kernels, the Correctness — looks solid
Minor issues1. Wasted export-time unpack on the Q6_K fallback. When a Q6_K weight can't merge to 2. Stale group size in the Q5_K native docstring. 3. Terminology drift across the One question
TestsCoverage is good — native/fused × Q4_K/Q5_K/Q6_K for both linear and embedding, ragged shapes, dynamic-M IfNode branches, group-merge (
I did not run the MLX kernel tests (they require Metal/macOS and aren't runnable on this Linux CI checkout); the review is static. Review authored with Claude. |
|
@claude review this code |
|
Claude finished @metascroy's task in 3m 16s —— View job Code Review: MLX Q5K/Q6K upgradesI read the repack paths, the native linear/embedding lowerings, Correctness — verified
Minor issues1. Q6_K computes biases twice, discards one. 2. Confirmed: shared Q6_K weight emits a duplicate init multiply. For a weight referenced by both a linear and an embedding, 3. Wasted full unpack on the Q6_K fallback. 4. Docstring / terminology drift.
TestsStrong coverage: native/fused × Q4_K/Q5_K/Q6_K for linear and embedding, dynamic-M I did not run the MLX kernel tests — they need Metal/macOS and aren't runnable on this Linux checkout. This review is static. Review authored with Claude. |
|
@claude review this code |
|
Claude finished @metascroy's task in 1m 48s —— View job Code Review: MLX Q5K/Q6K upgrades (pass 3)I re-read the repack paths, the native linear/embedding lowerings, Correctness — verified
Minor issues1. Stale comment in 2. Q6_K computes biases twice, discards one. 3. Shared Q6_K weight → duplicate init multiply. For a weight referenced by both a linear and an embedding, 4. Wasted full unpack on the Q6_K fallback. TestsStrong coverage: native/fused × Q4_K/Q5_K/Q6_K for linear and embedding, dynamic-M I did not run the MLX kernel tests — they need Metal/macOS and aren't runnable on this Linux checkout. This review is static. Review authored with Claude. |
This PR adds an MLX-native repack lowering path for GGUF Q5_K and Q6_K weights, mirroring the existing Q4_K path: at export time the raw GGUF blob is unpacked and repacked into MLX affine qparams (bits=5 / bits=6) consumed by MLX's native QuantizedMatmul/quantized-gather kernels, gated by ET_MLX_EMIT_DIRECT_GGUF (default = native, falling back to the fused Metal kernels otherwise). It also introduces a lossless group-size upgrade in to_intx_unpacked_to_int8_tensor (_max_uniform_group_size) that merges adjacent sub-blocks with identical scale/min into the largest MLX-supported group size (32→64→128) when bit-exact, which the repack paths request via max_group_size=128; because Q6_K's native group size (16) isn't MLX-compatible, its native path only activates when this merge reaches ≥32 and otherwise transparently falls back to fused kernels. Q6_K's symmetric zero-point lets biases be computed in the init chain via emit_quantized_biases instead of being serialized, and a FakeTensor guard keeps the data-dependent merge from tripping the partitioner's support check. Coverage includes new unit tests for the merge logic in test_gguf.py and native/fused + group-merged op tests across Q4_K/Q5_K/Q6_K in test_linear.py and test_embedding.py.