Skip to content

fix(ssd): reuse only finished prefetches; SIGTERM mid-stream test; doc fixes - #198

Open
solderzzc wants to merge 7 commits into
mainfrom
fix/ssd-streaming-followups-3
Open

solderzzc wants to merge 7 commits into
mainfrom
fix/ssd-streaming-followups-3

Conversation

@solderzzc

Copy link
Copy Markdown
Member

Follow-ups from the SwiftLM review session's reviews of #196 and #197 (the owner approved them as one follow-up PR).

Changes

--stream-experts prefetch / load (from the #196 review)

  1. Reuse only a finished prefetch. Because the streamed model now loads from its directory, the loader no longer fills in small missing files. A download interrupted after the last shard but before tokenizer.json or chat_template.jinja would therefore fail (or lose its chat template) on every later start. The reuse check now requires a .swiftlm-snapshot-complete marker. The marker is written only after snapshot returns and the copy has every shard plus the tokenizer files; otherwise the prefetch runs again.
  2. Load from the directory only after the MoE check, and only with tokenizer files present. Before, the switch to ModelConfiguration(directory:) ran before the non-MoE check turned streaming off, so dense models started with --stream-experts were affected too. Copies without tokenizer files keep the id-based load, which can fetch them.
  3. Re-validate after the snapshot. Offline, snapshot returns the repo directory even when it's partial. The prefetch now throws ModelDownloadIncomplete, reported as model_load_failed with a clear message, instead of activating streaming on a partial copy.
  4. Last progress frame. finish() cancelled the Task before its final frame, so the bar stopped around 90%. It now draws one frame at the real fractionCompleted before ending the line. The frame rendering moved into ProgressTracker.frame(fraction:).
  5. README: the crash warning gives the broadcast_shapes shape as an example (with top-k 8) instead of a fixed (N,8,8,D).

#197 review (docs and test)
6. exitAfterShutdownRequest() and its doc comment sat between emitEvent's /// block and func emitEvent, so they took over emitEvent's documentation. The function now sits after emitEvent.
7. The SIGPIPE comment now names exitAfterShutdownRequest() instead of Darwin.exit(0).
8. Test 39 in tests/test-server.sh: a SIGTERM sent while a stream is generating must exit with status 0 and put exiting{reason:"requested"} on its own JSON line. It must also leave no crash report for that server's PID; the check matches "pid" : N in .ips files so it doesn't pick up reports from other processes. Before this, only idle servers were SIGTERMed and the exit status was never read.

Verification (Mac mini M6)

  • Test 39, run standalone against real binaries: b782 (exit(), before fix: exit with _exit after a requested shutdown (low priority) #197) fails with status=139 (SIGSEGV) and 1 crash report. This branch passes twice in a row. Local runs used mlx-community/SmolLM-360M-Instruct-4bit; CI uses the script's default model.
  • Prefetch, with SmolLM-135M present only in the Application Support hub:
    • The first start runs snapshot once, writes the marker, and the bar ends at 100%.
    • The second start reuses the copy without a prefetch.
    • After removing the marker and a shard, the next start fetches again.
    • The offline-partial throw path wasn't exercised: this HubApi ignores HF_HUB_OFFLINE, so the shard was simply re-downloaded.
  • Regression check, Qwen3.6-35B-A3B --stream-experts at 548 tokens: 14.16 tok/s decode at 5.7 GB. Streaming activates, with no "without streaming" line.

AI usage: written by Claude Code (Claude Opus 5.5) in the M6 benchmarking session, with the repo owner's approval to open this PR. The findings came from the SwiftLM review session's reviews.

🤖 Generated with Claude Code

solderzzc and others added 4 commits September 27, 2026 15:57
…E check

Follow-ups from the #196 review:
- The prefetch reuse check requires a marker that is written only after
  snapshot returns and the copy has every shard plus the tokenizer files. A
  download interrupted after the last shard (tokenizer or template missing)
  is fetched again instead of failing on every later start.
- After the snapshot, re-validate and throw ModelDownloadIncomplete
  (model_load_failed) when the copy is partial; offline, snapshot returns the
  directory even then.
- Switch the main load to the streaming directory only after the non-MoE
  check, and only when tokenizer files are present, so dense models started
  with --stream-experts and incomplete copies keep the id-based load that can
  fill in missing files.
- ProgressTracker.finish() draws a last frame at the real fraction before
  ending the line, so the bar no longer stops at ~90%.
- README: give the broadcast_shapes crash as an example, not a fixed top-k.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Follow-ups from the #197 review:
- Move exitAfterShutdownRequest() below emitEvent. It sat between emitEvent's
  /// block and its declaration, so it took over that doc comment and
  emitEvent had none.
- The SIGPIPE note now names exitAfterShutdownRequest() instead of exit(0).
- Test 39 (test-server.sh): SIGTERM while a stream is generating must exit 0,
  put exiting{reason:"requested"} on its own JSON line, and leave no crash
  report for that PID. Fails on b782 (exit(), SIGSEGV 139); passes with #197.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A resolved model directory without tokenizer.json is treated as not found
  under --stream-experts, so the prefetch builds a complete copy and streaming
  targets the directory that is loaded (was a re-download plus a non-streaming
  load).
- Check tokenizer.json, which swift-transformers requires; tokenizer_config.json
  is optional and vocab.json is no substitute. A repo without it fails with
  ModelMissingTokenizer instead of 'download incomplete'.
- Write the completion marker only when the online file list succeeds and every
  listed file exists, so an offline partial snapshot never pins itself.
- If the Hub is unreachable but a loadable local copy exists (for example one
  from before the marker), warn and use it instead of failing.
- Finish the prefetch bar as soon as snapshot returns, before validation output.
- ModelDownloadIncomplete/ModelMissingTokenizer are CustomStringConvertible, so
  the exiting event's detail is the message, not a struct dump.
- The shutdown notice and the exiting JSON go out in one print, so a generation
  token can't land between them and push the JSON off its line.
- Test 39: nothing in it can abort the script under set -euo pipefail; it fails
  cleanly if the server never gets ready, bounds the shutdown wait at 30 s, and
  drops the .ips check (reports arrive too late; a crash already shows as a
  nonzero status).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 2 for #198. The prefetch guessed completeness from the shard
layout, which rejected valid repos and trusted partial ones. It now asks the
Hub which files the repo has (StreamingDirectory.swift):

- Online, a local copy is used only if it has every listed file: the
  ~/.cache copy first, then the loader's Application Support copy. Otherwise
  the model is downloaded and must then have them all. This accepts any
  weight layout the loader does (weights.NN.safetensors, no index) and stops
  an interrupted download in ~/.cache from being reused forever.
- A repo without tokenizer.json fails before downloading, with its own message.
- A Hub error (unknown or gated id) fails the start with a clear message; only
  network errors and timeouts count as offline.
- Offline, only a copy known to be complete is used: the marker written after
  a listing check, or a quietly validated copy with tokenizer.json.
- A dense model found locally isn't downloaded in full first; streaming turns
  off for it and the loader completes it.
- --info and local paths are left as they were.
- Non-streaming starts also check a validated local copy against the listing
  (best effort) and load through the Hub when files are missing.
- Test 39 parses each log line as JSON on its own, keeps the log on failure,
  and cleanup() removes its files and curl.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
solderzzc and others added 3 commits September 27, 2026 21:56
…wnload

Review round 3 for #198. Checking every start against the Hub's file list
rejected copies the loader reads fine, contacted the Hub on starts main made
offline, and failed on any Hub error. Back to local-first:

- Non-streaming starts are unchanged from main: no Hub request.
- --stream-experts uses the first local copy the loader can read (config.json,
  tokenizer.json, and either every indexed shard or any top-level
  *.safetensors) without contacting the Hub. Only when none exists does it ask
  the Hub and download into the Application Support copy.
- Hub 401/404 (with no loadable copy) is ModelNotOnHub; any other Hub or
  network error is ModelUnavailableOffline. No custom timeout.
- The listing only checks the repo has tokenizer.json and which top-level
  files the finished download must contain.
- An in-progress marker is written before a download into Application Support
  and removed after success, so an interrupted download or update is re-fetched
  instead of trusted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 4 for #198:
- localWeightState mirrors safetensorWeightURLs: the index only when every
  file it names exists, otherwise model*, weight*, then all top-level
  *.safetensors. Shards named stem-NNNNN-of-MMMMM must all be present, a lone
  model.safetensors is complete, and other layouts are unverified, so the Hub
  listing decides. A partial download without an index is no longer served,
  and a complete copy with a stale index (Qwen3-VL MoE repos) is no longer
  re-downloaded forever.
- Unverified copies are checked against the listing; if the Hub can't be used
  they are used with a warning.
- A Hub 404 surfaces as .fileNotFound in swift-transformers; treat it as not
  on the Hub.
- A dangling symlink counts as missing.
- resolveStreamingDirectory reports whether the copy is complete, so a dense
  model's complete copy loads by directory after streaming turns off (no
  network), and only the dense shortcut's partial copy loads by id.
- Unit tests: partial no-index, complete and partial stale-index, single file,
  unverified layout, dangling symlink, tokenizer.json required.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review round 5 for #198:
- localWeightState falls back to the shard check only when the index's
  missing names are top-level (a stale index). A missing file in a
  subdirectory, such as OptiQ's optiq/optiq_vision.safetensors, makes the copy
  incomplete; before, a model*-only download of an OptiQ VLM was served and
  its vision tower silently ran on random weights.
- The Hub-listing and post-download checks also require listed nested files
  the copy's index names (requiredListedFiles).
- --info --stream-experts drops a clearly incomplete candidate again, so it
  says the model isn't downloaded instead of profiling a partial copy.
- Unit tests: nested indexed file missing → incomplete; present → complete;
  requiredListedFiles keeps indexed nested files and skips unindexed ones.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant