fix: macOS engine parity — Metal builds, Rapid-MLX, and cross-engine test fixes#50
Conversation
mohitsoni48
left a comment
mohitsoni48
left a comment
There was a problem hiding this comment.
Reviewed on Windows: rebased fix/macos-support onto current main (clean, no conflicts), ran the full test suite/typecheck/build for root + web/ — all green (746/749 tests pass, 3 skipped, 0 failures).
The macOS-parity work itself looks solid and doesn't regress Windows/Linux — the process.platform gating in build-prereqs.ts/build-runner.ts/manager.ts is correctly isolated, and the llamafile shell-wrap fix passes args via argv (not string concatenation), so no shell-injection risk despite routing through /bin/sh -c.
Ran a deeper structured review on top of that and independently re-verified each candidate against the current code (not just the diff) before posting. 4 findings survived, left as inline comments below. The one on profile.ts:465 is the important one — it means the vLLM crash fix this PR calls out doesn't actually cover the common case (model context > 8192) and reproduces the same crash it's meant to fix. I'd hold off merging until that one's addressed; the other three are lower-severity and can land as-is or as quick follow-ups.
(Mac-only paths — Metal build, real Rapid-MLX install, ik_llama fallback — reviewed for correctness only; couldn't functionally exercise them from this Windows box.)
| // model's max_position_embeddings when --max-model-len is left unset, so raise | ||
| // --max-num-batched-tokens in lockstep whenever that would otherwise exceed 2048. | ||
| const effectiveMaxLen = v.maxModelLen > 0 ? v.maxModelLen : p.ctx | ||
| if (effectiveMaxLen > 2048) a.push('--max-num-batched-tokens', String(effectiveMaxLen)) |
There was a problem hiding this comment.
High: this doesn't cover the common case — reproduces the exact crash it's meant to fix.
deriveDefault() caps p.ctx at 8192 for every engine kind (Math.min(m.nativeCtx || 8192, 8192), line 224). When maxModelLen is left at its default (0, the documented "derive from model config" case), effectiveMaxLen here falls back to that capped p.ctx — so --max-num-batched-tokens gets emitted as 8192 while --max-model-len is omitted entirely. vLLM then derives its real max-model-len straight from the model's own config (e.g. 32768 for most modern models), and its scheduler validator rejects max-num-batched-tokens < max-model-len — reproducing the identical crash this PR fixes.
Repro: deriveDefault(model({ nativeCtx: 32768 }), sys) → p.ctx = 8192; vllmProfileToArgs(p) → ['--max-num-batched-tokens','8192'], no --max-model-len.
The new tests in profile.vllm.test.ts only ever set nativeCtx to 2048 or exactly 8192 (the cap boundary), so this gap passes CI undetected — even though the test file's own model() helper defaults nativeCtx to 32768. Needs the model's real uncapped nativeCtx threaded into this computation instead of the llama.cpp-oriented, 8192-capped p.ctx.
| expertCount, | ||
| nextnLayers: 0, | ||
| vision: false, | ||
| vision: cfg.vision_config != null, |
There was a problem hiding this comment.
Low: this fix is correct here, but it now surfaces a downstream inconsistency.
Now that vision is genuinely true for some MLX models instead of always false, three other call sites that build the loaded-model descriptor still hardcode vision: false regardless of entry.vision: src/api/routes.ts:1087, src/gateway/model-router.ts:276, and src/cli.ts:561 (none touched by this PR). Since manager.status() returns that object verbatim, GET /api/v1/status will report vision:false for an actively-loaded MLX vision model while GET /api/v1/models correctly reports vision:true for the same key — an internally inconsistent API. Before this PR the hardcode was harmless (every MLX entry was vision:false anyway); now it's a real, if currently invisible, data-correctness bug. No in-app consumer branches on it yet, but worth threading entry.vision through those three sites alongside this fix.
| </div> | ||
| )} | ||
|
|
||
| {isRapidMlx && ( |
There was a problem hiding this comment.
Medium: this banner contradicts the still-active Sampling section right below it.
The banner says Rapid-MLX has "no launch-time sampling defaults," but the Temperature/Top-P/Top-K/Min-P sliders directly below (gated by isLlamaCpp only for the extra penalty/stop-string fields, not these four) render unconditionally and stay wired into draft.sampling → saved via actions.save.mutate(...) regardless of load mode. For a rapid-mlx model this makes them dead controls: editing and saving does nothing at launch time (confirmed on the backend — rapidMlxServerCommand in manager.ts takes no sampling args), but nothing in the UI tells the user the edit was a no-op or that sampling actually lives in the per-conversation chat settings.
| <p className="text-[13px] text-muted"> | ||
| <span className="font-medium text-ink">{build.engine}</span> was built from source, bundled | ||
| with its CUDA runtime, and set as your active engine. Load a model to start using it. | ||
| <span className="font-medium text-ink">{build.engine}</span> was built from source and set as |
There was a problem hiding this comment.
Low: this success message is now inaccurate for Windows/Linux CUDA builds.
The "bundled with its CUDA runtime" clause was dropped unconditionally from the success copy, but build-runner.ts's runBuild() still does if (isWindows) copyCudaRuntimeDlls(...); else if (!isMac) copyCudaRuntimeLibs(...) — Windows and Linux CUDA builds still physically bundle the runtime. The new text ("...was built from source and set as your active engine.") is only fully accurate for macOS/Metal, where there's genuinely nothing to bundle. BuildProgress has no os prop to condition this on currently.
ddd823a to
a6a60cc
Compare
…UDA) The in-app compile-from-source flow (ADR-100) was hardcoded to Windows/Linux + CUDA only, with three separate platform gates (build-prereqs.ts, build-runner.ts, and a route-level check in routes.ts) all rejecting darwin. macOS has no CUDA but does have Metal via the system Xcode toolchain, so there's no reason the feature can't work there too — it just needs git/cmake/clang++ instead of a CUDA toolkit, and -DGGML_METAL=ON instead of -DGGML_CUDA=ON, with no runtime libs to bundle afterward (Metal is a system framework). Verified end-to-end on an Apple M2 Pro: built upstream llama.cpp from source with the new flow, confirmed cmake picked up the Metal backend, and the resulting binary ran natively.
A small local model (gemma-4B via MLX) emitted {"queries": [...]} instead
of the schema's singular {"query": ...}, which made the tool-approval
dialog render the literal string "undefined" instead of the real search
query, and would have made execWebSearch silently search for an empty
string server-side. Extracted a resolveSearchQuery() helper (mirrored on
both the backend and the web bundle, which can't share Node code) that
prefers query but falls back to queries[0].
…omplete Reproduced on this Mac: building ik_llama.cpp with -DGGML_METAL=ON fails — its src/llama-dflash.cpp calls ggml_backend_is_metal/ggml_backend_metal_set_n_cb, but its vendored ggml doesn't implement them (matches its own catalog note, "CPU + CUDA only, no ROCm/Metal" — this fork's Metal support is genuinely incomplete, not a TurboLLM config issue). Rather than failing the guided build outright, detect this specific undefined-symbol signature and retry the same repo as a CPU-only build (-DGGML_METAL=OFF). Verified end-to-end: ik_llama.cpp now builds and runs on this Mac.
Rapid-MLX (github.com/raullenchai/Rapid-MLX, PyPI "rapid-mlx", 3.2k stars, actively maintained) was only a catalog placeholder (comingSoon: true, no install/launch/readiness wiring). Added a full integration mirroring the existing MLX engine's pattern: uv-bootstrapped isolated venv, pip install, launch via its own console-script binary, OpenAI-compatible server. Touches registry (addRapidMlx), manager (spawn command, readiness/load-failure detection, python-engine env), routes (install/update/purge endpoints, catalog installed/enabled detection — fixed a pre-existing bug where sglang incorrectly checked vllm's venv path), compat (model-format acceptance, and the "default" model alias Rapid-MLX expects — distinct from mlx/vllm's "default_model"), HF search filtering, update-checking, and the corresponding frontend (install/update mutations, engine grouping, load-mode UI copy, provision banner). Verified end-to-end on this Mac: installed for real, activated, loaded a local MLX model directory, and got a correct real chat completion through both TurboLLM's own gateway and a standalone instance. Also fixes a real bug found while testing every catalog engine end-to-end: llamafile ships as an "Actually Portable Executable" (Cosmopolitan libc) polyglot binary, which needs shell interpretation to dispatch to the right native format. Node's spawn() calls execve() directly (no shell), which silently failed with ENOEXEC on macOS — the daemon accepted the start request but the process never actually spawned, with no visible error beyond a console.warn on the daemon's own stdout. Fixed by routing llamafile specifically through `/bin/sh -c 'exec "$0" "$@"'` on non-Windows platforms (Windows already recognizes the polyglot's leading MZ/PE header natively) — the standard safe shell-wrapping idiom, no manual argument quoting needed. Verified end-to-end: llamafile now loads a real GGUF model and serves a correct chat completion on this Mac.
…heck
verifyWithVision() in ArtifactCard.tsx POSTed to /api/v1/chat/completions
(the extra "/api" prefix doesn't exist as a route — every other caller in
the codebase correctly uses /v1/chat/completions), so this call always
404'd and silently fell through to `catch { return true }` — meaning the
artifact "does this render correctly?" self-check has never actually run.
Found live on macOS while testing chat artifact rendering: an HTML artifact
that got cut off mid-generation by the model's token limit rendered as a
blank iframe, and this self-check should have (but didn't) catch it.
Note: fixing the URL surfaces a second, separate, deeper issue — the
gateway route itself rejects image content ("Only 'text' content type is
supported"), so the self-check still can't succeed end-to-end. Flagged
separately for follow-up (needs a design decision on which endpoint this
should actually hit).
…es, Rapid-MLX audio-model incompatibility vLLM never emitted --max-num-batched-tokens, so its own scheduler validator rejected any model with context >2048 (nearly all real models) — a universal bug, not Mac-specific. Fixed in vllmProfileToArgs. TurboLLM's HF downloader only fetched .safetensors/.json files, silently skipping chat_template.jinja (the current HF convention for standalone chat templates) — leaving MLX-format downloads with no usable template on engines without a template-less fallback (Rapid-MLX/vLLM). Fixed in HfClient.getRepo, plus the scanner's hasChatTemplate check now also recognizes the standalone file, not just an embedded one. Rapid-MLX's bundled mlx_vlm has a confirmed, reproducible bug loading any MLX-format model with an audio tower (double-transposes conv weights, verified across two independently-converted gemma4 checkpoints and the latest available mlx-vlm release). Added vision/audio detection for MLX-format models (previously vision was hardcoded false) and used it to hide audio-tower models from Rapid-MLX's Library view and reject loading them with a clear error instead of crashing the engine process. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ailure tool-explain.ts (a pure string-formatting utility, no React) imported friendlyName from MessageBubble.tsx (a React component), pulling the whole React dependency chain into its module graph. This passed locally (web/ node_modules happened to be installed) but failed in CI, which only runs `npm ci` in the root turbollm/ package: `Cannot find package 'react'`. Moved friendlyName into tool-explain.ts (the natural home — it's already the shared "describe a tool call" utility) and updated the three importers to pull it from there instead, reversing the dependency direction so a utility module never depends on a component file. Also removes the tool-explain.test.ts localStorage stub, which was only needed to survive this same bad import chain — verified by running the test with web/ node_modules removed entirely (reproducing the CI failure), confirming it now passes standalone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
High: vllmProfileToArgs used LoadProfile.ctx (capped at 8192 by deriveDefault for llama.cpp/MLX's KV-memory sizing) as the proxy for what vLLM derives when --max-model-len is left unset — but vLLM actually derives its real max-model-len straight from the model's own uncapped native context. For any model with context > 8192 (the common case), this reproduced the exact --max-num-batched-tokens crash the fix was meant to prevent. Now takes the model's real nativeCtx as an explicit parameter instead of reading it off the capped profile; added a regression test using this file's own 32768-context model() default, matching the reported repro. Low: three call sites building the loaded-model status descriptor (routes.ts, model-router.ts, cli.ts) hardcoded vision: false regardless of entry.vision, made incorrect now that MLX vision detection actually works — GET /api/v1/status would report vision:false for an actively-loaded MLX vision model while GET /api/v1/models correctly reported vision:true for the same key. Now threads entry.vision through. Medium: the Rapid-MLX "no launch-time sampling defaults" banner in ModelDetailDialog was directly contradicted by four Temperature/Top-P/ Top-K/Min-P sliders rendering unconditionally right below it — editing and saving them is a genuine no-op (rapidMlxServerCommand takes no sampling args), with no indication given to the user. Hidden for Rapid-MLX to match the banner's own claim. Low: the guided-build success message's "bundled with its CUDA runtime" clause was dropped unconditionally, but Windows/Linux CUDA builds still physically bundle the runtime (runBuild() copies the DLLs/libs) — only macOS/Metal has nothing to bundle. Now conditioned on the real detected OS via useSysInfo() instead of a blanket claim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rebase-only fixup: main's v1.7.7 added this test file with its own ModelEntry fixture, predating the audio field this branch introduced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
d04fecc to
5dc5f3d
Compare
1 participant
Summary
Brings macOS to parity with Windows+CUDA across the engine subsystem, found by concretely testing every installable engine end-to-end (real model load, real chat, real long-generation, real streaming) rather than by code review.
ik_llama.cppauto-falls-back to a CPU-only build when its own Metal backend is incomplete, instead of failing outright.llamafilespawn fixed — its Cosmopolitan APE polyglot format needs shell dispatch; Node'sspawn()was silently failing withENOEXEC.web_searchtool-call display fixed a plural{"queries":[...]}vs schema{"query":...}mismatch that rendered the literal string"undefined"./api/v1/chat/completions→/v1/chat/completions).--max-num-batched-tokens, so its own scheduler validator rejected any model with context >2048 — a universal bug, not Mac-specific..safetensors/.jsonfiles, silently skippingchat_template.jinja(the current HF convention for standalone templates) — verified end-to-end by downloading the missing file for a real model and confirming chat now works on the engine that previously hard-failed.visionwas hardcodedfalsefor all of them) and used it to hide audio-tower models from Rapid-MLX's Library view with a clear error instead of a crashed engine process — confirmed as a genuine upstreammlx-vlmbug across two independently-converted checkpoints, not a per-download conversion issue.Test plan
npm test)npm run build, bothweb/and root)transformersv5 packaging issue in vLLM itself)llvmlite==0.36.0), unchanged, not a regressionrequirements/feature/engines-redesign/requirements.md🤖 Generated with Claude Code