Conversation
| .target( | ||
| name: "MLXNN", | ||
| dependencies: ["MLX", "MLXRandom", "MLXFast"], | ||
| dependencies: ["MLX"], |
There was a problem hiding this comment.
These dependencies are obsolete, clean up.
| /// | ||
| /// ### See Also | ||
| /// - ``depends(inputs:dependencies:)`` | ||
| public func depends(input: MLXArray, dependencies: [MLXArray]) -> MLXArray { |
There was a problem hiding this comment.
Noticed this was missing
Source/MLX/Ops.swift
Outdated
| /// - bits: Number of bits used to represent each element of `x` and `w` | ||
| /// - mode: The quantization mode. Default is `.affine` | ||
| /// - stream: Stream or device to evaluate on | ||
| public func qqMatmul( |
|
Note: cmake builds will fail until mlx-c is tagged and we pick up the tag. |
| /// - w: weight matrix. If quantized, it is packed in unsigned integers. | ||
| /// - scales: The scales to use per `groupSize` elements of `w` if `w` is quantized | ||
| /// - groupSize: Number of elements in `x` and `w` that share a scale | ||
| /// - bits: Number of bits used to represent each element of `x` and `w` | ||
| /// - mode: The quantization mode. Default is `.affine` |
There was a problem hiding this comment.
The default here should be "nvfp4" (affine doesn't make sense as it's not supported in this op).
There was a problem hiding this comment.
Ah, I missed the new quantization modes, glad this showed up!
| @@ -215,6 +215,7 @@ Note: the `-` and `/` operators are not able to be linked here. | |||
| - ``addMM(_:_:_:alpha:beta:stream:)`` | |||
| - ``quantizedMatmul(_:_:scales:biases:transpose:groupSize:bits:mode:stream:)`` | |||
| - ``gatherQuantizedMatmul(_:_:scales:biases:lhsIndices:rhsIndices:transpose:groupSize:bits:mode:sortedIndices:stream:)`` | |||
| - ``qqMatmul(_:_:scales:groupSize:bits:mode:stream:)`` | |||
There was a problem hiding this comment.
This is a bit of a nit, but the naming here is inconsistent with itself (and inconsistent with MLX core which is also inconsistent with itself 😓)
- In some cases
Quantizedis expanded in some it's justq(should it beQ?) - In some cases we have
MMand in othersMatmul
Unclear why it's different from MLX core in the conventions and the pattern is also somewhat random.
There was a problem hiding this comment.
We don't have to fix this now but I want to flag it as I could see it becoming messier as we add more ops.
There was a problem hiding this comment.
Per offline discussion we will do:
- matmul
- gatherMM
- blockMaskedMM
- addMM
- quantizedMM
- gatherQuantizedMM
- quantizedQuantizedMM
awni
left a comment
There was a problem hiding this comment.
Looks awesome, minor comments. O/w good to go!
| @@ -1006,6 +1006,11 @@ public protocol UnaryLayer: Module { | |||
| func callAsFunction(_ x: MLXArray) -> MLXArray | |||
| } | |||
|
|
|||
| /// A `Layer` (``Module`` subclass) that can be evaluated with an array and offset. | |||
There was a problem hiding this comment.
@davidkoski this declaration is from an earlier version and has made its way back.
There was a problem hiding this comment.
Sorry, must have messed it up in a rebase. Let me get a PR out for it.
Proposed changes
mlx v0.30.1 which includes the NAX JIT work.
This still requires a tag on mlx-c and updating the CMakeLists.txt to reflect that.
Checklist
Put an
xin the boxes that apply.pre-commit run --all-filesto format my code / installed pre-commit prior to committing changes