feat: let SwiftBuddy load a model from an arbitrary local folder - #161
Merged
Conversation
Fixes #160. A user storing models on an external drive (downloaded via `hf download --local-dir <path>`, entirely outside the app's HF cache) had no way to point SwiftBuddy at that folder directly — only the `SwiftLM` CLI's `--model <path>` already supported this. Distinct from issue #110's "hand-copied into a recognised cache-root layout" support (ModelStorage.localLoadDirectory, still used unchanged for that case): this is for a directory that can be anywhere on disk, addressed by its own absolute path rather than an HF-style "org/name" id resolved relative to cacheRoot. ## ModelStorage.swift - `isLocalDirectoryPath(_:)` — is this string actually a directory that exists on disk, mirroring the identical check Server.swift's CLI `--model` handling already does. - `validateLocalModelDirectory(_:)` — reuses the same weight/config validation `verifyModelIntegrity` applies to HF-cache layouts, so a bad folder selection is rejected with a clear message before `InferenceEngine.load` ever attempts to construct a model from it. - `readModelConfig(inDirectory:)` / `readMaxContextLength(inDirectory:)` — directory-addressed variants of the existing id-addressed functions (refactored to share the same underlying logic rather than duplicating it). ## InferenceEngine.swift `load(modelId:)` now recognises a local-directory modelId early and skips straight to loading it — the usual verify-or-download flow assumes an HF-style id and would otherwise try to download the path as if it were a repo id. `loadVerifiedModel` threads an `explicitLocalDirectory: URL?` through: building `ModelConfiguration(directory:)`, the SSD-streaming directory, the post-load integrity check, and the context-length read all branch on it instead of going through the id-based `ModelStorage` helpers (which only ever resolve paths under cacheRoot and would find nothing for an external path). Also: a load failure for a local directory no longer offers the "Delete & Re-download" recovery flow, since there's no repo to re-download from (confirmed `ModelStorage.delete` would silently no-op for such a path anyway — it only ever resolves candidates under cacheRoot — but showing that recovery option would still be misleading UX). ## ModelManagementView.swift (macOS only) "Add Local Model…" button next to "Search HuggingFace MLX Models" (in both the populated list and the empty state), opening an NSOpenPanel folder picker. Validates the selection with `ModelStorage.validateLocalModelDirectory` before calling `engine.load(modelId: url.path)`; a bad selection shows an alert instead of proceeding. ## Tests New tests/SwiftBuddyTests/LocalModelDirectoryTests.swift (13 tests) — deliberately does NOT use `cacheRootOverride`, unlike the #110 layout tests, since these functions must work independent of cacheRoot entirely. Full suite: 282/282 passing, zero regressions (including all 21 existing ModelStorageLayoutTests, confirming the readModelConfig/ readMaxContextLength refactor didn't change #110's behavior). Model persistence across app relaunch (`lastLoadedModelId`) works unchanged for a local path — it's stored as a plain string, same as an HF id, so no separate persistence code was needed.
…y support A code review found several issues in the previous commit's local-directory support (issue #160), stemming from modelId being overloaded to mean either an HF repo id or a local path with no single place resolving that once. ## Fixed - isMoE always false for local-directory models: ModelCatalog.all.first(where: { $0.id == modelId }) can never match a filesystem path, silently disabling SSD expert streaming for exactly the "large MoE model on external drive" use case this feature exists for. Now falls back to inspecting the local config.json directly (new ModelStorage.configIndicatesMoE, checking the same expert-count field names ModelProfiler.findExpertCounts uses) when the catalog lookup misses. Fixed in both InferenceEngine.loadVerifiedModel and the same independent pattern in SettingsView's currentModelIsMoE. - markModelCorrupted's "Delete & Re-download" recovery was only suppressed for local directories at the load-time catch block; three generation-time call sites (SSD streaming error, generic corruption error, latched SSD error) had no such guard, so a transient error during generation on a local-directory model would still offer a recovery button that doesn't work (delete no-ops, re-download treats the path as a repo id). Moved the guard into markModelCorrupted itself so every current and future call site gets it, rather than relying on each one remembering to add it. - A stale persisted local path (drive unplugged, folder moved/deleted since last session) silently fell through to the HF-id download path, attempting to download the raw filesystem path as a repo id instead of showing a clear error. load() now recognizes a path-shaped modelId ("/...", which no real HF id ever is) that no longer resolves to a directory and surfaces "the folder may have moved or its drive isn't connected" immediately. - isLocalDirectoryPath was checked twice per load (once in load(), again inside loadVerifiedModel), leaving a narrow window where the two checks could disagree if the path's existence changed in between. loadVerifiedModel now takes the already-resolved localDirectory as a parameter instead of re-deriving it. - Local-directory detection was duplicated three ways with two different definitions: Server.swift's CLI --model handling (plain dir check), Server.swift's resolveModelDirectory (dir + config.json check), and the new ModelStorage.isLocalDirectoryPath (plain dir check, previously described in its own doc comment as merely "mirroring" Server.swift rather than sharing logic with it). Both Server.swift call sites now call ModelStorage.isLocalDirectoryPath directly (the CLI target already depends on MLXInferenceCore). - A model loaded via "Add Local Model…" was invisible everywhere in the app once loaded — never in dm.downloadedModels since it was never downloaded, so no view indicated it as the current model. ModelManagementView now shows a "Current Model (Local)" section for it, with a "Show in Finder" action and deliberately no delete option (there is nothing in the app's cache to remove). - Load-failure error messages interpolated the raw absolute path; now shown as just the folder name, matching how downloaded models are already displayed elsewhere trimmed to their last path component. ## Tests - New ModelStorage.configIndicatesMoE tests (top-level and nested text_config expert-count fields, dense-model and zero-count negative cases). - New InferenceEngine test verifying a missing local path produces a clear, immediate error without ever reaching the network (hermetic — no real download attempt). - Full suite: 288/288 passing, zero regressions.
Member
Author
|
Pushed a follow-up commit addressing all 8 findings from the code review: Fixed:
Not fixed (scope): the full New tests for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #160. A user storing models on an external drive (downloaded via
hf download --local-dir <path>, entirely outside the app's HF cache) had no way to point SwiftBuddy at that folder directly — only theSwiftLMCLI's--model <path>already supported this. Adds a matching "Add Local Model…" option to the SwiftBuddy app.Distinct from issue #110's "hand-copied into a recognised cache-root layout" support (
ModelStorage.localLoadDirectory, left unchanged and still used for that case): this is for a directory that can be anywhere on disk, addressed by its own absolute path rather than an HF-styleorg/nameid resolved relative tocacheRoot.What's new
ModelStorage.swift:isLocalDirectoryPath(_:),validateLocalModelDirectory(_:), and directory-addressed variants ofreadModelConfig/readMaxContextLength(refactored to share logic with the existing id-addressed versions rather than duplicating it).InferenceEngine.swift:load(modelId:)recognises a local-directorymodelIdearly and skips the usual verify-or-download flow (which would otherwise try to download the path as if it were a repo id).loadVerifiedModelthreads anexplicitLocalDirectory: URL?through model config construction, SSD-streaming setup, the post-load integrity check, and context-length reading — all of which normally go throughModelStorage's id-based helpers that only ever resolve paths undercacheRoot. Also suppresses the "Delete & Re-download" recovery flow for local-directory load failures (no repo to re-download from — confirmedModelStorage.deletewould silently no-op for an external path anyway, but offering that option would still be misleading).ModelManagementView.swift(macOS only): "Add Local Model…" button opening anNSOpenPanel, validated before callingengine.load(modelId:).Tests
New
LocalModelDirectoryTests.swift(13 tests), deliberately independent ofcacheRootOverridesince these functions must work regardless of where the folder actually is. Full suite: 282/282 passing, zero regressions (including all 21 existingModelStorageLayoutTests— confirms thereadModelConfig/readMaxContextLengthrefactor preserved #110's behavior exactly).Model persistence across app relaunch (
lastLoadedModelId) works unchanged for a local path since it's just a plain string, same as an HF id.Test plan
swift build --target MLXInferenceCore/SwiftBuddy— clean.