Repository navigation
Regenerate from numen-tech/mlx 2f0e44f9: re-land the counter fixes (gemma4-qat #181) - #6
Conversation
…l-explore#181) Bumps Source/Cmlx/mlx from aaefa500 to 2f0e44f9 (numen-tech/mlx PR #4, fork/181-counters-reland: prism-0.32.2-fixes + fork/86-affine-sym-0.32.2). That picks up the counter fixes prism was missing: 2e5d6ccd (skip teardown and CPU-only waits, count GPU throttle waits), 1fee32b6 (throttle waits only on GPU completion; skip eval error-recovery syncs), 9cb5069a (comments), and the PR #2 comment condensations after ff1deee5. Regenerated with tools/update-mlx.sh (LC_ALL=C, Xcode clang). Only include-framework headers (gpu/eval.h, metal/device.h, metal/metal.h, scheduler.h, the Metal.hpp stamp) and mlx-generated quantized.h/.cpp (comment-only) change. The Cmlx mlx_counters shim and MLX.GPUCounters are unchanged: the counters() API is identical, waits now also include the scheduler's GPU throttle waits. xcrun swift build: OK. xcodebuild MLXTests (incl. GPUCountersTests): 391 run, 2 skipped, 0 failed. The four commits unique to the old stranded pin 216f0f0 are patch-identical to debbd14/9b669be/ff81a8e/9cf0d59, already on prism via PR #4, so nothing is carried over. prism_readme: point the submodule rule at numen-tech/mlx#4.
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ee0ec6891
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Count only when a GPU task completed during the wait. | ||
| if (n_active_gpu_tasks_ < n_gpu_tasks_old) { | ||
| gpu_waits_.fetch_add(1, std::memory_order_relaxed); |
There was a problem hiding this comment.
When a sufficiently large GPU evaluation hits the scheduler's active-task throttle, this increment makes GPUCounters.waits include each blocking wait_for_one() call. However, the public documentation in Source/MLX/GPUCounters.swift and Source/Cmlx/include/mlx_counters.h still enumerates only event, synchronization, and CPU-fence waits. Performance experiments can therefore attribute these additional values to the wrong source; update both public contracts to describe scheduler throttle waits as well.
Useful? React with 👍 / 👎.
…(gemma4-qat ml-explore#181, Codex P2) counters().waits now includes Scheduler::wait_for_one() calls that ended on a GPU task completion. Update the Swift and C public docs, which listed only event, sync and CPU-fence waits.
…ero implied-bias factor gradient, gemma4-qat ml-explore#190)
1 participant
Depends on numen-tech/mlx#4. Merge that first. This PR bumps
Source/Cmlx/mlxto2f0e44f9, the head of numen-tech/mlx#4'sfork/181-counters-relandbranch.Why
prism(after #4 here) pinsSource/Cmlx/mlxataaefa500. That commit is missing the later counter fixes from numen-tech/mlx#3:Those fixes merged into
fork/86-affine-sym-0.32.2after that branch had already gone intoprism-0.32.2-fixes, so they never reached any branch this repo pins. numen-tech/mlx#4 merges them intoprism-0.32.2-fixes.What
Source/Cmlx/mlxfromaaefa500to2f0e44f9.LC_ALL=C CC=… CXX=… ./tools/update-mlx.sh. Nothing was hand-edited. The only changes are:gpu/eval.h,metal/device.h,metal/metal.h,scheduler.h, and the commit stamp inMetal.hpp;mlx-generatedquantized.handquantized.cpp(metal: affine_sym host+kernels, Precise transcendentals and tests on the 0.32.2 line (gemma4-qat #86) mlx#2's comment condensation afterff1deee5).mlx_countersshim andMLX.GPUCountersare unchanged.counters()has the same API.waitsnow also includes the scheduler's GPU throttle waits.prism_readme.md: the submodule rule now points at Re-land the #181 dispatch/commit/sync/wait counters onto prism-0.32.2-fixes mlx#4.The four commits unique to the old stranded gemma4-qat pin
216f0f09(f552b32, 1218d4c, b43df03, 216f0f0) are patch-identical to debbd14, 9b669be, ff81a8e and 9cf0d59, which are already onprismvia #4 (git range-diff: all=). Nothing needed carrying over.Tests
xcrun swift build: OK.xcodebuild test -scheme mlx-swift-Package -destination 'platform=macOS' -skipPackagePluginValidation -parallel-testing-enabled NO -only-testing:MLXTests: 391 run, 2 skipped, 0 failed. This includesGPUCountersTests(2/2).Merge order: numen-tech/mlx#4, then this PR, then the gemma4-qat re-land PR, which pins this head. Please use a merge commit, not squash or rebase, so the pinned SHA stays reachable from
prism.