Skip to content

fix(metal): drop TSIM_DEVICE, bound the buffer pool, clearer errors - #296

Merged
rafaelha merged 3 commits into
mainfrom
fix/metal-usability
Nov 12, 2026
Merged

rafaelha merged 3 commits into
mainfrom
fix/metal-usability

Conversation

@rafaelha

Copy link
Copy Markdown
Collaborator

Usability fixes from a review of the Metal backend, plus fixes to the skills.

Changes

Tsim

  • TSIM_DEVICE removed. The device argument is the only way to choose the device. Before, any value other than cpu (such as cpuu, 0 or metal) was silently ignored.
  • Freed GPU output buffers are bounded. The pool keeps at most 1 GB (runtime.POOL_BYTES), and larger buffers are released at once. Before, a 1.28 GB boolean sample of surface_d7_r7 left 1.6 GB allocated after both the array and the sampler were gone. The bench's calls return at most about 256 MB, so they still reuse buffers.
  • Clearer device="metal" errors. A tiny rotation now reports its angle in the units the docs use, and every "cannot run" error names the fix:
    Cannot sample on Metal: a rotation's angle is 1.0e-05 π from a Clifford angle (a multiple of π/2), closer than the 6.4e-05 π that float32 amplitudes resolve; sample with device='cpu', or with device='auto', which falls back to the CPU.
    Before, it read: a rotation has a branch of weight 2.5e-10, below the 1e-08 that float32 amplitudes resolve.
  • The metal/__init__.py module docstring is rewrapped.

Dev harness

  • tsim-dev bench, bench --width and profile used TSIM_DEVICE=cpu to stay on the CPU, because tsim-bench's worker has no device option. They now start the worker through dev/tsim_dev/on_device.py, which passes device="cpu" to compile_sampler and compile_detector_sampler when Tsim has that argument (older Tsims sample on the CPU only). A --device option in tsim-bench's worker would make the shim unnecessary.

Skills and docs

  • simulating-circuits-with-tsim:
    • The memory-cap advice is split by device: an explicit batch_size on the CPU; on a GPU, leave it unset and ask for fewer shots per call.
    • The "Common errors" table has Cannot sample on Metal rows.
    • "List the stages down" now reads "Write the stages down", and the broken wrapping is fixed.
    • The description is 900 characters (was 994) and names the Metal error.
  • shipping-tsim-changes: a row requiring tsim-dev bench --device gpu --ref origin/main for changes to the GPU path.
  • benchmarking-tsim: device=cpu|gpu is listed among the --side keys. Mentions of TSIM_DEVICE are replaced here, in profiling-tsim, in docs/install.mdx and in the CHANGELOG entry.

Verification

  • uv run tsim-dev check: PASS for lint and tests (2783 passed).
  • New tests:
    • test_freed_buffers_beyond_the_pool_bytes_are_released: fails without the fix, since POOL_BYTES doesn't exist there.
    • test_device_cpu_keeps_the_samplers_on_the_cpu: checks that TSIM_DEVICE=cpu is now ignored.
    • The tiny-rotation test now matches the new message and checks that an angle near π/2 (R_Z(0.50001)) also stays on the CPU.
  • test_dev_cli checks that the CPU bench runs the worker through the launcher and the GPU bench does not.
  • An end-to-end run shows the device column as cpu by default and metal with --device gpu.

Benchmarks against origin/main (quick suite, Apple M4 Pro, 1 thread; the machine was partly busy, busy_cores 4.4 to 5.6):

Run Result
tsim-dev bench --ref origin/main (CPU, bit-packed) 8 circuits, all rates same (0.98 to 1.03x)
tsim-dev bench --device gpu --ref origin/main (bit-packed) 8 circuits, all rates same (0.98 to 1.02x)
tsim-dev bench --device gpu --bools --ref origin/main 8 circuits, all rates same (0.98 to 1.00x)

GPU, bit-packed:

circuit                                        k device  shots/s   kept  compile   shots/s vs ref (min, max)      compile vs ref
inject_cultivate_d3_p0.001                     4  metal    1.97G  0.687     0.18   1.00x [0.99, 1.02] same    4.39x faster
inject_cultivate_d5_p0.001                     6  metal     747M  0.144     0.35   1.02x [0.99, 1.08] same    0.99x same
end2end_d3_dsurface15_p0.001                   4  metal     172M  0.539     2.11   0.99x [0.97, 1.00] same    0.99x same
hook_inject_T_d15_pd5_p0.001                   1  metal    60.4M  0.667     2.43   0.98x [0.97, 1.00] same    1.00x same
distillation_color_d5_Z                        3  metal    2.45G 0.0798     0.19   1.00x [1.00, 1.00] same    1.20x same
coherent_d3_r3                                 4  metal    1.16G      -     0.18   1.00x [1.00, 1.00] same    0.81x same
surface_d7_r7                                  0  metal     754M      -     0.28   1.00x [1.00, 1.00] same    1.18x same
qv_q10                                        10  metal    11.8M      -     0.37   1.02x [0.99, 1.04] same    1.12x same

The two "faster" compile verdicts (4.39x here, 1.95x on coherent_d3_r3 with booleans) are noise. Without --thorough the processes compile at the same time, and this change doesn't touch the compile path.

Not verified:

  • No --thorough or --suite full runs, since this change doesn't aim at speed.
  • tsim-dev profile fails on macOS because py-spy's --native isn't supported there. That happens without this change too.

🤖 Generated with Claude Code

- Remove the TSIM_DEVICE environment variable; the `device` argument
  chooses where a sampler samples. tsim-dev's bench, width and profile
  modes now pass device="cpu" to the samplers through a small launcher
  around tsim-bench's worker (dev/tsim_dev/on_device.py).
- Keep at most 1 GB of freed Metal output buffers for reuse; larger
  buffers are released at once, so a big boolean sample no longer pins
  its memory for the life of the process.
- The device="metal" error for a tiny rotation names its angle and the
  threshold in units of π, and every "cannot run" error says to use
  device="cpu" or "auto".
- Skills: GPU-aware memory advice, Metal rows in the errors table,
  wrapping fixes and a shorter description in the user skill; a GPU
  bench row in shipping-tsim-changes; the device side key in
  benchmarking-tsim.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-12T21:38:10.975889Z 658640c PR opened
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 658640c9e4

ℹ️ 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".

Comment thread dev/tsim_dev/bench.py
done = subprocess.run(
wrapped,
env=side_env(side, threads, device),
env=common.child_env(threads),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Clear legacy TSIM_DEVICE for GPU workers

When the invoking shell still exports TSIM_DEVICE=cpu, --device gpu --ref ... now passes that variable through child_env to both workers. The changed checkout ignores it, but a reference such as this commit's parent still honors it, so the benchmark compares the new GPU path against the reference CPU path and can report a misleading performance change. Preserve the previous GPU-side sanitization by removing TSIM_DEVICE from worker environments.

Useful? React with 👍 / 👎.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Nov 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

☂️ Code Coverage

current status: ✅

Overall Coverage

Statements Covered Coverage Threshold Status
6277 5203 83% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/tsim/circuit.py 95% 🟢
src/tsim/sampler.py 95% 🟢
src/tsim/statevector/metal/_init_.py 34% 🟢
src/tsim/statevector/metal/runtime.py 40% 🟢
TOTAL 66% 🟢

updated for commit: 468ff85 by action🐍

@rafaelha
rafaelha merged commit f9ef8a4 into main Nov 12, 2026
14 checks passed
@rafaelha
rafaelha deleted the fix/metal-usability branch November 12, 2026 22:36
rafaelha added a commit that referenced this pull request Nov 12, 2026
TSIM_DEVICE is gone, as on main; the rotation-angle message moves into the
shared gpu.tape.unsupported, which CUDA uses too; CUDA's CHANGELOG entry goes
under Unreleased.

Co-Authored-By: Claude Opus 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.

1 participant