Skip to content

fix(hardware): detect Apple Silicon under Rosetta 2 in GPU probe (BE-3435) - #568

Open
mattmillerai wants to merge 4 commits into
mainfrom
matt/be-3435-rosetta-gpu-probe
Open

fix(hardware): detect Apple Silicon under Rosetta 2 in GPU probe (BE-3435)#568
mattmillerai wants to merge 4 commits into
mainfrom
matt/be-3435-rosetta-gpu-probe

Conversation

@mattmillerai

Copy link
Copy Markdown
Collaborator

ELI-5

If you run an Intel-flavored Python on a modern Apple Mac (via Apple's Rosetta 2 translator), the machine lies and says it's an old Intel chip. Our hardware probe believed the lie and reported "no GPU" — even though the Mac has a perfectly capable Apple GPU. This teaches the probe to ask macOS one extra question ("am I being translated?") so it correctly recognizes the Apple chip underneath and reports its unified-memory GPU.

What & why

comfy_cli/hardware.py:_detect_gpu keyed Apple Silicon unified-memory detection on machine == 'arm64'. An x86_64 Python running under Rosetta 2 on an Apple Silicon Mac reports platform.machine() == 'x86_64', so the unified-memory block was skipped and the machine fell through to the NVIDIA/AMD probes (which find nothing) — a capable GPU reported as gpu: null.

Fix: a small _is_apple_silicon(machine) helper keeps the native arm64 fast path and, when machine != 'arm64', falls back to sysctl -n sysctl.proc_translated (via the existing _run helper). A value of '1' means the process is translated, which only happens on Apple Silicon, so the apple/unified block is used.

Notes for the reviewer

  • Chesterton's Fence — this deliberately broadens an existing check. The original machine == 'arm64' guard was introduced (intentionally minimal) in the base PR feat: add cross-platform hardware block to comfy env --json (BE-3399) #562 (BE-3399); this ticket (BE-3435) is the scoped follow-up to broaden it. The old and new conditions differ only in the newly-matched state — machine != 'arm64' and proc_translated == '1', i.e. exactly Apple Silicon under Rosetta. On a genuine Intel Mac the sysctl.proc_translated OID is absent, _run returns None, None == '1' is False, so Intel behavior is unchanged (the existing test_intel_mac_has_no_apple_gpu still passes).
  • Never-raise contract preserved: _is_apple_silicon only calls _run (which never raises and is timeout-bounded) plus a string compare, and it runs inside _detect_gpu's existing try/except.
  • Under Rosetta, arch is still honestly reported as x86_64 (that's what the process is); only the gpu block is corrected. The model comes from machdep.cpu.brand_string, which returns the real Apple chip name even under translation.
  • New unit test mocks _run to return '1' for the sysctl.proc_translated key and asserts the unified block.

Stacking

Opened stacked on matt/be-3399-hardware-block (#562)hardware.py does not exist on main yet; it lands with #562. GitHub will retarget this PR to main when #562 merges; the diff here is just the net Rosetta change.

Testing

  • pytest tests/comfy_cli/test_hardware.py → 20 passed
  • ruff check + ruff format --check on the touched files → clean

mattmillerai and others added 3 commits July 17, 2026 17:40
Add comfy_cli/hardware.py with a single detect_hardware() entry point that
reports OS, CPU, RAM, and a GPU sub-block (vendor/model/VRAM/unified-memory)
so agent surfaces can route weak machines away from local diffusion.

- macOS: cpu via sysctl machdep.cpu.brand_string; Apple Silicon -> apple
  unified-memory GPU; Intel Mac -> unknown non-unified GPU (no system_profiler).
- NVIDIA (any OS): nvidia-smi CSV, then ctypes libcuda fallback (reuses
  cuda_detect._load_libcuda).
- AMD (Linux): best-effort rocm-smi --json.
- Never raises, never blocks long: every probe wrapped, subprocesses timeout=5.

Wire hardware into EnvChecker.fill_data() (JSON) and a Hardware row in
fill_print_table() (pretty). Extend schemas/env.json with an OPTIONAL, fully
nullable hardware property (not in required) so older/failed-probe payloads
stay valid. Envelope schema unchanged (envelope/1).
…(BE-3399)

- env_checker: escape untrusted cpu/gpu-model strings in the Rich-rendered
  hardware summary so markup-like content (e.g. "[/]") can't raise MarkupError
  and crash `comfy env` in pretty mode.
- hardware(_detect_gpu_amd): return None (no phantom all-None AMD block) when
  nothing parses, matching the NVIDIA probes; gate the card loop on the "card"
  key prefix so a non-card metadata block isn't parsed as the GPU; exclude the
  "VRAM Total Used Memory" usage key so total capacity isn't understated.
- hardware(_detect_gpu_nvidia_ctypes): pop/restore CUDA_VISIBLE_DEVICES around
  the ctypes probe (mirrors cuda_detect) so an exported ""/"-1" doesn't hide a
  present GPU.
- tests: cover AMD total-vs-used key, metadata-block skip, all-None->None, and
  markup escaping.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…3435)

An x86_64 Python running under Rosetta 2 on an Apple Silicon Mac reports
platform.machine() == 'x86_64', so _detect_gpu skipped the unified-memory
block and fell through to the NVIDIA/AMD probes, reporting gpu: null on a
capable machine. Add _is_apple_silicon(), which keeps the native arm64 fast
path and falls back to sysctl.proc_translated == '1' (via the existing _run
helper) to recognize the translated case. Genuine Intel Macs lack that
sysctl key, so _run returns None and they still fall through unchanged.

Preserves the never-raise contract (_run never raises; the whole probe stays
wrapped). Adds a unit test mocking _run for the Rosetta case.
@mattmillerai mattmillerai added agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review labels Jul 18, 2026
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 54 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3b923c3b-91aa-48f5-8ca5-33f3423a88a4

📥 Commits

Reviewing files that changed from the base of the PR and between 98325c9 and e475dca.

📒 Files selected for processing (2)
  • comfy_cli/hardware.py
  • tests/comfy_cli/test_hardware.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch matt/be-3435-rosetta-gpu-probe
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch matt/be-3435-rosetta-gpu-probe

Comment @coderabbitai help to get the list of available commands.

@mattmillerai
mattmillerai marked this pull request as ready for review July 18, 2026 01:59
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jul 18, 2026

@github-actions github-actions 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.

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

✅ No high-signal findings.

Panel: 6/8 reviewers contributed findings.

Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)

Base automatically changed from matt/be-3399-hardware-block to main July 29, 2026 00:53
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jul 29, 2026
@bigcat88

Copy link
Copy Markdown
Contributor

This PR currently conflicts with main — GitHub reports mergeable: CONFLICTING, so it needs a rebase before I can review it and I'm skipping it in the current review sweep.

Please rebase (or merge main in) and I'll pick it up on the next pass. main moved a fair bit in the last day, including #614 (ANSI sanitisation across the pretty-print call sites) and #628 (the duplicate server_died error-code fix that had main red), so a refresh may also clear unrelated CI noise on this branch.

Resolve conflicts in hardware.py/env_checker.py/test_hardware.py by
keeping both this PR's Rosetta 2 detection (_is_apple_silicon) and
main's AMD rocm-smi multi-card fallback + resolved-local-address env
checker changes.
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-coded PR authored by the agent-work loop cursor-review Request Cursor bot review size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants