Repository navigation
Conversation
ArupKantiDas
left a comment
There was a problem hiding this comment.
I tested this on an M5 Pro (applegpu_g17s, so is_nax_available() is true) against a clean build of main 4ca8d5d5a. The gated delta code is the same on this PR's base 9e2e8e2 and on 4ca8d5d5a. CI never takes this path. This PR's macOS jobs ran on "Apple M2 Pro (Virtual)", and the self-hosted runners from #4795 report "Apple M4" (gen 16), so is_nax_available() is false on both.
| Check (M5 Pro) | main 4ca8d5d5a |
this PR ac923d1 |
|---|---|---|
test_fast_gated_delta.py, TF32 off (conftest) |
2 failed (sequential, simdgroup) |
all pass |
same, MLX_ENABLE_TF32=1 |
2 failed | 2 failed |
#4614 repro, float32, dg rel. L2 vs float64 (16/48, strong decay), TF32 off |
8.8e+02 | 2.2e-07 |
| same, TF32 on (the default) | 8.8e+02 | 8.8e+02 |
| Full pytest suite | 3 failed, 969 passed (the 2 above plus test_conv test_input_dilation_grad, which isn't on this PR's base yet) |
0 failed, 969 passed |
With TF32 off the change does what it says. float32 now takes the sequential VJP, and every input gradient in the #4614 repro is within 4e-7 of the float64 reference (16/48 and 16/16, strong and weak decay).
Some notes:
-
MLX_ENABLE_TF32defaults to 1 (utils.h), so with default settings the #4614 repro gives the samedgerror as before. bf16 inputs on the chunked VJP get it too:dgrel. L2 of 8.5e+02, against 3.2e-03 withGATED_DELTA_CHUNK_VJP=1. So the largedgerror comes from the chunkeddgpath under strong decay, and TF32 rounding is only part of it (#4788 has a plausible explanation for why). Should #4614 stay open, or get a follow-up issue, for that part? -
The two failures on main aren't a TF32 problem.
config::getcaches the first value it reads from the environment (config.cpp), and pytest runs unittest methods in alphabetical order. That meanstest_gated_delta_naxsetsGATED_DELTA_CHUNK=16first, and thentest_gated_delta_sequentialandtest_gated_delta_simdgroupalso run the NAX kernel, against a 1e-4 tolerance. Each passes on its own, and the file passes on main withtest_gated_delta_naxdeselected. On CI the cached 16 turns into 8 (no NAX), so iftest_gated_delta_sequentialruns aftertest_gated_delta_naxin the same xdist worker, it's really testing the simdgroup kernel. -
With this PR and conftest's TF32 off,
test_gated_delta_naxand thechunk=16subtests oftest_gated_delta_vjpdon't reach the NAX kernels on M5 either: 16 falls back to 8 in the forward and to 1 in the VJP. The CI runners have no NAX, so right now nothing in the suite runs the NAX gated delta kernels. One way to fix both this and point 2 is to usemx.config.scoped_update(GATED_DELTA_CHUNK=...)in the three forward tests, astest_gated_delta_vjpalready does, and addMLX_ENABLE_TF32=1for the NAX cases. I tried that locally. The file passes on main and on this PR with conftest's TF32 off, and the NAX cases still hit the NAX kernels.
I used Claude Code to build both versions, run the tests and repro above.
|
Thanks for looking into this PR. On the precision, at the moment we can only try to guarantee that when you use float32 and On the tests, I did some refactoring in #4783 that uses |
Close #4614