Skip to content

Fix attention mask for float16 - #496

Merged
davidkoski merged 3 commits into
ml-explore:mainfrom
louen:val/attnmask
Oct 4, 2026
Merged

davidkoski merged 3 commits into
ml-explore:mainfrom
louen:val/attnmask

Conversation

@louen

@louen louen commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

This PR fixes a bug in MultiHeadAttention.createAdditiveCausalMask which occurs with float16

for dtype in [DType.float32, .bfloat16, .float16] {
    let mask = MultiHeadAttention.createAdditiveCausalMask(2, dtype: dtype)
    print(dtype, mask.asType(.float32).asArray(Float.self))
}

which will yield invalid values in float16

float32 [-0.0, -1e+09, -0.0, -0.0]
bfloat16 [-0.0, -9.9824435e+08, -0.0, -0.0]
float16 [nan, -inf, nan, nan]

due to the fact that -1e9 is not representable in float16, yielding infinities at the masked location and nan (0 * inf) in unmasked locations.

This doesn't occur in python

for dtype in [mx.float32, mx.bfloat16, mx.float16]:
    mask = nn.MultiHeadAttention.create_additive_causal_mask(2,dtype)
    print(dtype, mask.astype(mx.float32).tolist())

which correctly uses the min representable value for all types

mlx.core.float32 [[-0.0, -3.4028234663852886e+38], [-0.0, -0.0]]
mlx.core.bfloat16 [[-0.0, -3.3895313892515355e+38], [-0.0, -0.0]]
mlx.core.float16 [[-0.0, -65504.0], [-0.0, -0.0]]

.

The fix is simply to use dtype.finfo.min where available in the function.

This PR adds a unit tests that fails before the fix and succeeds after.

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works

@louen louen changed the title Fix attention max for float16 Fix attention mask for float16 Oct 4, 2026

@davidkoski davidkoski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Just waiting on CI, thanks!

@davidkoski
davidkoski merged commit b4f7684 into ml-explore:main Oct 4, 2026
7 checks passed
eejd pushed a commit to eejd/mlx-swift that referenced this pull request Oct 18, 2026
…lx-swift

Real merge of upstream/main (b4f7684, "Fix attention mask for float16 (ml-explore#496)")
instead of a snapshot copy, so upstream history is preserved and future syncs
only have to deal with new commits.

- Re-vendor mlx v0.32.2 (1f8e74e) and mlx-c ebc88f1 as plain directories and
  re-apply the SharpAI patch series on top (SSD expert streaming, TurboKV,
  fast loaders, mlx-c fast.* wrappers, quieter Metal compile logging).
- Drop the old lazy per-thread Metal command encoder patch: upstream's Swift
  StreamPool creates streams with mlx_stream_new_thread_unsafe, so they already
  work across threads, and a second encoder for the same stream index would
  silently break ordering.
- mlx-c: forward the Swift stream in mlx_fast_streamed_gather_mm (it used only
  the device, which crashes when the graph is evaluated on another thread) and
  make the prefetch cache's try_take check the stored size before copying.
- Regenerate Source/Cmlx/mlx-generated with tools/update-mlx.sh; keep
  moe_stream.metal.
- Swift: keep the fork's MLXFast bindings next to upstream's crossEntropy and
  the compile scoped-default-device fix.
- Package.swift stays on swift-tools-version 5.12 (upstream's 6.3;(experimentalCGen)
  is only needed for the Linux CUDA plugin, which this fork does not ship).
- Restore upstream fixes that an earlier sync (877d992) had silently reverted:
  Module.update dictionary keys (ml-explore#356), ellipsis indexing copy (ml-explore#358/ml-explore#363),
  init order (ml-explore#367), WiredMemory cancellation guard (ml-explore#369), release.yml (ml-explore#471).
- Add SSDStreamingTests (streamedGatherMM, preadInto, preadIntoOffset, and
  evaluation on a different thread).
- Document the layout and the merge procedure in UPSTREAM_MERGE_PLAN.md.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants