fix: review follow-ups — CLI re-download, nested expert configs, refresh races - #125
Merged
Conversation
Deferred LOW findings from the #116 review, now addressed. - The CLI server still built ModelConfiguration(id:) for anything that was not an explicit filesystem path, so `SwiftLM --model org/name` re-downloaded a model already present in a hand-copied or huggingface-cli layout — the same bug #116 fixed for the app, on the surface it did not cover. It now reuses ModelStorage.localLoadDirectory(for:). Verified end to end: a plain org/name folder under HF_HUB_CACHE logs "Loading from local cache" and generates, with no download. - rejectionReason reported a weights problem for directories that fail on metadata. validateModelFiles rejects a zero-byte config.json or tokenizer.json before it looks at weights, so a user with an empty config.json was told to inspect an index.json that never existed. Metadata is now checked first, and a too-small single-file model gets its own message instead of the index.json one. - refreshInBackground had no coalescing: a scan started before a delete could land after it and re-add the deleted model. Refreshes now carry a generation stamp, and any newer refresh — background or synchronous — cancels and invalidates the in-flight scan. Tests: localLoadDirectory across all hand-copied layouts, nil for the materialized layout (which must keep the id-based flow), and nil when absent or failing verification. Verified red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deferred LOW findings from the #114 review. Expert counts were read from the top level and `text_config` only. Two real shapes were missed, both confirmed against the published configs: - deepseek-ai/deepseek-vl2-tiny puts n_routed_experts under `language_config`, and its model_type ("deepseek_vl_v2") contains no "moe" — so neither the key lookup nor the name heuristic caught it. Fully misdetected as dense. - Qwen/Qwen3-Omni-30B-A3B-Instruct nests two levels down, under thinker_config.text_config. Replaced the fixed two-level lookup with a breadth-first walk over nested containers. Shallowest wins, so an outer explicit count stays authoritative over one nested deeper; non-positive values are still skipped as placeholders; and the active count is paired with the container its total came from. Sibling containers are visited in a deterministic order — known language-model container names first, then alphabetically. Qwen3-Omni carries a count under both talker_config and thinker_config with *different* active counts (6 vs 8), so an unordered walk would have reported a different number run to run. Also log the config.json decode error instead of swallowing it: returning nil with no explanation is exactly the diagnosability failure that made #112 hard to pin down in the first place. Tests cover both real shapes, the thinker-vs-talker ordering (repeated to catch nondeterminism), and outer-beats-nested at depth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of this PR found the headline fix did not cover the main case, plus a regression already shipped in b674. - The CLI's HubApi is rooted at Application Support, not ModelStorage.cacheRoot, so localLoadDirectory's "materialized layouts are resolved by HubApi" guard — true for the app — meant a model the app had downloaded was still invisible to the CLI and fetched again. The CLI now asks for any validated on-disk copy. The earlier verification used HF_HUB_CACHE, the one layout that dodged this. - localLoadDirectory could return a directory that exists but is not the one that validated, handing a caller with no download fallback a broken path. Added validatedContentDirectory. - --stream-experts silently no-opped on the newly-supported layouts: resolveModelDirectory knows none of them, so modelDirectory was nil, which skipped both the MoE guard and ExpertStreamingConfig.activate while still setting lazyLoad — lazy weights with no streamer, and no diagnostic. - findExpertCounts no longer descends into encoders (vision/audio/projector), which could outrank the language model's count from a shallower depth, and a container with a total but no per-token count inherits the nearest ancestor's. - Removed the Codable expert plumbing that findExpertCounts superseded; two implementations of the same rule is how #112 came back the first time. - rejectionReason's new branch was unreachable and described a truncated single-file model as sharded. - materializedDirectory now applies the delete guard, so an org-less id is uniformly unsupported rather than loadable-but-undeletable. - Corrected the isSafeModelDirectory comment: resolvingSymlinksInPath only resolves paths that exist, so non-existent ones fail closed. - Dropped a 20x test loop that could not surface the nondeterminism it implied (Swift seeds dictionary hashing per process). Submodule bumped to b320bc4, which carries the fix for gemma-4-e2b-it-4bit — broken since #44 and shipped in b674, because that checkpoint ships K/V weights for its KV-shared layers while e4b does not. Verified on the merge tree: e2b, e4b and Qwen3.6-27B-OptiQ all answer correctly. 151 tests across 11 suites, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The LOW-severity findings deferred from the #114 and #116 reviews. Two of them turned out to have real user-visible impact.
1. The CLI re-downloaded models already on disk (#110's other surface)
#116taught the app to find hand-copied models, but the CLI still builtModelConfiguration(id:)for anything that was not an explicit filesystem path — soSwiftLM --model org/namemissed on disk and re-downloaded several GB of a model the user already had. Same bug, different surface. It now usesModelStorage.localLoadDirectory(for:).Verified end to end: a plain
org/namefolder underHF_HUB_CACHEnow logsand generates, with no download.
2. Expert counts hidden in other nested containers (#112's remaining gap)
Detection read the top level and
text_configonly. Two published shapes were missed — both confirmed against the real configs, not guessed:deepseek-ai/deepseek-vl2-tinylanguage_config.n_routed_experts: 64model_typeisdeepseek_vl_v2, so the name heuristic did not fire eitherQwen/Qwen3-Omni-30B-A3B-Instructthinker_config.text_config.num_experts: 128Replaced the fixed lookup with a breadth-first walk over nested containers. Shallowest wins, so an outer explicit count stays authoritative; non-positive values are still skipped as placeholders; and the active count is paired with the container its total came from.
Sibling order is deterministic — known LM container names first, then alphabetical. This matters: Qwen3-Omni carries a count under both
talker_configandthinker_configwith different active counts (6 vs 8), so an unordered dictionary walk would report a different number run to run. The test repeats 20× to catch that.3. Smaller items
config.jsondecode errors are logged instead of silently returning nil — the same diagnosability failure that made Support for SSD expert streaming is not being detected correctly. #112 hard to pin down.rejectionReasonno longer misdirects.validateModelFilesrejects a zero-byteconfig.jsonbefore it looks at weights, so a user with an empty config was told to inspect anindex.jsonthat never existed. Metadata is checked first now.refreshInBackgroundcoalesces. A scan started before a delete could land after it and re-add the deleted model. Refreshes carry a generation stamp; any newer refresh, background or synchronous, cancels and invalidates the in-flight one.Tests
New:
localLoadDirectoryacross every hand-copied layout, nil for the materialized layout (which must keep the id-based flow), nil when absent or failing verification; both real nested config shapes; thinker-vs-talker ordering; outer-beats-nested at depth. Verified red where the behaviour is new.Full suite: 149 tests across 11 suites, 0 failures.
🤖 Generated with Claude Code