fix(hardware): detect Apple Silicon under Rosetta 2 in GPU probe (BE-3435) - #568
fix(hardware): detect Apple Silicon under Rosetta 2 in GPU probe (BE-3435)#568mattmillerai wants to merge 4 commits into
Conversation
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.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
🔍 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)
|
This PR currently conflicts with Please rebase (or merge |
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.
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_gpukeyed Apple Silicon unified-memory detection onmachine == 'arm64'. An x86_64 Python running under Rosetta 2 on an Apple Silicon Mac reportsplatform.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 asgpu: null.Fix: a small
_is_apple_silicon(machine)helper keeps the nativearm64fast path and, whenmachine != 'arm64', falls back tosysctl -n sysctl.proc_translated(via the existing_runhelper). 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
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'andproc_translated == '1', i.e. exactly Apple Silicon under Rosetta. On a genuine Intel Mac thesysctl.proc_translatedOID is absent,_runreturnsNone,None == '1'is False, so Intel behavior is unchanged (the existingtest_intel_mac_has_no_apple_gpustill passes)._is_apple_silicononly calls_run(which never raises and istimeout-bounded) plus a string compare, and it runs inside_detect_gpu's existingtry/except.archis still honestly reported asx86_64(that's what the process is); only thegpublock is corrected. Themodelcomes frommachdep.cpu.brand_string, which returns the real Apple chip name even under translation._runto return'1'for thesysctl.proc_translatedkey and asserts the unified block.Stacking
Opened stacked on
matt/be-3399-hardware-block(#562) —hardware.pydoes not exist onmainyet; it lands with #562. GitHub will retarget this PR tomainwhen #562 merges; the diff here is just the net Rosetta change.Testing
pytest tests/comfy_cli/test_hardware.py→ 20 passedruff check+ruff format --checkon the touched files → clean