fix: show warning when pulling model in offline mode (#2412)#2468
fix: show warning when pulling model in offline mode (#2412)#2468bong-water-water-bong wants to merge 14 commits into
Conversation
…r OpenAI compat Closes lemonade-sdk#1370 — OpenCode / @ai-sdk/openai-compatible streaming crash Three fixes for reasoning model streaming: 1. Streaming proxy normalization (streaming_proxy.cpp): - Intercepts each SSE data: {...} line in forward_sse_stream() - Injects content: "" when reasoning_content is present without content - Injects role: "assistant" when null/missing on assistant deltas - Only applies to chat.completion.chunk objects (non-chat passthrough) 2. Non-streaming response (server.cpp): - Same content injection for REST chat completions response 3. thinking: false passthrough (server.cpp): - Replaced strip_handled_thinking_fields() (which erased enable_thinking/ thinking before forwarding) with normalize_thinking_fields() which renames thinking → enable_thinking and keeps it in the forwarded request. FLM/vLLM/cloud backends now see enable_thinking. - /no_think prefix retained for llama.cpp compatibility Tests: 11 unit tests covering role normalization, reasoning content normalization, carriage return, multi-choice, multi-line streams. 7/7 C++ tests pass (100%).
Closes lemonade-sdk#2371 — multi-GPU systems only got ROCm for the first GPU arch get_rocm_arch() iterates AMD GPUs (iGPU first, then dGPU) and returns only the first match. On systems with both an iGPU and dGPU with different architectures, TheRock was only installed for the iGPU. Fix: - Add get_rocm_arches() returning ALL detected AMD GPU architectures (deduplicated, iGPU-first ordering preserved) - Update install_therock_if_needed() to install TheRock for every arch - Keep get_rocm_arch() for backward compat (rocm_channel, display)
…dk#1364, lemonade-sdk#1546) lemonade-sdk#1364 — Large Prompts Timing Out - Add SSE keepalive heartbeat thread in forward_sse_stream() - Sends : keepalive\n\n every 10s during prefill while waiting for first token - Prevents client-side read timeouts on long-running prompt processing - Thread-safe via shared mutex with the libcurl write callback lemonade-sdk#1546 — Model Download Resilience - Add .completed sentinel written after all files are verified in download - is_checkpoint_path_complete() checks for .completed as authoritative marker - Prevents corrupt partially-downloaded files from appearing complete - Hardened recursive_directory_iterator with skip_permission_denied + error_code
Add a pre-load memory check in Router::load_model() that compares model_info.size (file size in GB) against get_available_memory_gb() for the target device. Logs a warning when the model may not fit. Chose warn-only (not block) because: 1. GGUF file size != load-time memory (mmap'd, paged) 2. Auto-tune ctx_size resolver will reduce context to fit 3. A hard block would frustrate users who know their setup
Completes fixes for lemonade-sdk#1364, lemonade-sdk#1546, lemonade-sdk#1804: ## lemonade-sdk#1364 — SSE heartbeat during long prefill Injects : keepalive\n\n every 10s during prefill to prevent client-side read timeouts on long prompts (15k tokens → 5 min prefill). ## #1546b — .completed sentinel for download verification Written after all files are verified in download_from_huggingface(). is_checkpoint_path_complete() checks for it, preventing corrupt partially- downloaded files from appearing complete after a crash. ## #1546a — Model-level download resume fast-path download_from_huggingface() now accepts do_not_upgrade flag. When set and .completed sentinel exists, skips the HF API call entirely. ## #1546c — Directory iterator hardening discover_extra_models() uses skip_permission_denied + error_code handling to prevent crashes on temp files from interrupted downloads. ## lemonade-sdk#1804 — Pre-load OOM guard Upgraded the pre-load memory check from warning to hard block when model size exceeds 2x available memory headroom. Prevents OOM killer crashes with a clear error message instead. ## CI — PR-Agent + Qodo dual review Added pr-agent-review.yml (DeepSeek) and qodo-merge.yml workflows.
Before fetching the HF API and rebuilding the file list, check for an existing .download_manifest.json with incomplete files. If found, resume downloading from the partial state instead of starting over. This avoids re-downloading already-completed files after a network interruption or Ctrl-C during model pull.
This reverts commit 11991bc.
…ection (lemonade-sdk#2414) The function used a std::lock_guard which caused a deadlock when build_recipes_info re-entered get_system_info_with_cache via get_rocm_arch(). Fix by switching to std::unique_lock, marking s_recipes_computed early, and unlocking during recipe computation. On failure the flag resets so the next call retries.
…ete (lemonade-sdk#2435) The draft checkpoint is optional — the model can run without MTP spec decoding. But are_required_checkpoints_complete() iterates over ALL checkpoint types, and if the draft model hasn't been downloaded yet the entire model gets marked as not-downloaded / 'unreadable'. Fix by skipping the draft type alongside npu_cache.
…crets Adds a setup script that reads API keys from ~/Documents/ and pushes them as GitHub secrets via \`gh secret set\`, plus comment headers in both PR-Agent workflows referencing how to configure secrets. Workflows already skip gracefully when secrets are missing, but the script and comments make it discoverable for new contributors. - .github/scripts/setup-repo-secrets.sh — idempotent, dry-run, --repo support - pr-agent-review.yml — header: DEEPSEEK_API_KEY required - qodo-merge.yml — header: QODO_API_KEY required
…-sdk#2413) install_therock_if_needed() didn't check if TheRock was already installed before calling install_therock(). When hot-swapping multiple ROCm backends (e.g. llamacpp:rocm and sd-cpp:rocm), each triggered a separate 3 GB TheRock download. Fix by adding is_therock_installed_for_current_arch() check at the top of install_therock_if_needed().
When offline mode is enabled, the server silently skipped the download but the CLI still printed 'Model pulled successfully'. Fix: server now includes a 'warning' field in both legacy and SSE streaming completion responses when offline, and the CLI surfaces it as 'Model pulled with warning: ...' instead of 'successfully'.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4b2dc43be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (model_info.size > headroom * 2.0) { | ||
| is_loading_ = false; | ||
| load_cv_.notify_all(); | ||
| throw std::runtime_error( |
There was a problem hiding this comment.
get_available_memory_gb() uses 0.0 as the "could not determine memory" fallback, but this new check treats that as zero available memory. On CPU-only non-Apple/non-AMD systems, CPU recipes such as whispercpp, sd-cpp, Moonshine, and Kokoro can have model_info.size > 0 while the helper returns 0.0, so /load now throws before the backend is even tried. Please skip the hard rejection when availability is unknown, or use a real system-RAM total for CPU/NPU checks.
Useful? React with 👍 / 👎.
| if (entry.is_directory() && safe_exists(entry.path() / ".completed")) { | ||
| LOG(INFO, "ModelManager") << "Model already downloaded (found .completed sentinel), skipping API call" | ||
| << std::endl; | ||
| return; |
There was a problem hiding this comment.
This sentinel fast path only proves that some snapshot for the repo completed, not that the files for this model's checkpoint/variant are present. For shared repos such as ggerganov/whisper.cpp or UsefulSensors/moonshine-streaming, auto-loading Whisper-Base after Whisper-Tiny was cached calls download_registered_model(..., true), hits this return, marks the second model downloaded, and then the backend fails because its resolved path is empty. The shortcut needs to validate the requested checkpoint files, not just any .completed snapshot.
Useful? React with 👍 / 👎.
| // indistinguishable from a complete download by other checks alone. | ||
| // The sentinel is written after all files are verified in | ||
| // download_from_huggingface(). | ||
| return safe_exists(marker_dir / ".completed"); |
There was a problem hiding this comment.
For directory variants, resolved can be a subdirectory inside the HF snapshot, but the new sentinel is written to snapshot_path / ".completed". Moonshine checkpoints resolve to paths like .../snapshots/<sha>/onnx/tiny, so after a successful download this check looks for onnx/tiny/.completed, never finds it, and cache rebuilds mark the model as not downloaded. The directory-case check needs to locate the containing snapshot root, or the writer must place the sentinel where this code expects it.
Useful? React with 👍 / 👎.
| auto result = utils::HttpClient::post_stream( | ||
| backend_url, | ||
| request_body, | ||
| [&sink, &telemetry_buffer, &has_done_marker, &has_first_token, | ||
| &time_to_first_token, &start_time, &on_chunk](const char* data, size_t length) { | ||
| [&sink, &sink_mutex, &telemetry_buffer, &has_done_marker, &has_first_token, | ||
| &time_to_first_token, &start_time, &on_chunk, &line_buffer](const char* data, size_t length) { |
There was a problem hiding this comment.
HttpClient::post_stream can throw before the explicit heartbeat_thread.join() runs, for example when the write callback returns false after the client disconnects (CURLE_WRITE_ERROR). With a still-joinable std::thread on the stack, unwinding calls std::terminate, turning a normal streaming disconnect or curl error into a process crash. Please wrap the call with RAII/finally-style cleanup that stops and joins the heartbeat before any exception leaves this scope.
Useful? React with 👍 / 👎.
| s_recipes_computed = true; | ||
| lock.unlock(); |
There was a problem hiding this comment.
Setting s_recipes_computed = true before releasing the lock means any concurrent caller entering while build_recipes_info() is still running will take the fast return at the top and receive s_cached_system_info without a recipes section. That can make recipe/backend status endpoints or FLM readiness checks report unsupported/missing recipes transiently under concurrent requests. Use a separate "computing" state or only flip s_recipes_computed after s_cached_system_info["recipes"] is assigned.
Useful? React with 👍 / 👎.
| bool enable = true; | ||
| if (thinking.is_boolean()) { | ||
| enable = thinking.get<bool>(); | ||
| request_json["enable_thinking"] = enable; |
There was a problem hiding this comment.
When a request includes both enable_thinking and thinking, this assignment overwrites the canonical field from thinking even though should_disable_thinking() just above gives enable_thinking precedence. For clients that send enable_thinking: false alongside thinking: true, Lemonade prepends /no_think but still forwards enable_thinking: true to backends that honor it, re-enabling reasoning. Only fill enable_thinking from thinking when the canonical field is absent.
Useful? React with 👍 / 👎.
| if (is_therock_installed_for_current_arch(backend_versions)) { | ||
| LOG(DEBUG, "BackendManager") << "TheRock already installed, skipping" << std::endl; | ||
| return; |
There was a problem hiding this comment.
On mixed AMD-GPU systems where the primary ROCm arch already has TheRock installed, this early return fires before get_rocm_arches() is iterated, so a second arch added later or missing from the first install never gets its runtime. That leaves the new multi-arch install path reporting success while the non-primary GPU's ROCm runtime is absent; skip per arch inside the loop instead.
Useful? React with 👍 / 👎.
| pr_agent_job: | ||
| name: PR-Agent (DeepSeek) | ||
| runs-on: ubuntu-latest | ||
| if: ${{ github.event.sender.type != 'Bot' && secrets.DEEPSEEK_API_KEY != '' }} |
There was a problem hiding this comment.
GitHub documents that secrets cannot be directly referenced in if: conditionals. This job-level guard, and the same pattern in .github/workflows/qodo-merge.yml, uses secrets.* in if, so the new review workflows are invalid instead of being skipped based on whether keys exist; gate via an env/output check instead.
Useful? React with 👍 / 👎.
| if (!line_buffer.empty()) { | ||
| std::string tail = StreamingProxy::normalize_chat_completion_chunk(line_buffer); | ||
| sink.write(tail.data(), tail.size()); |
There was a problem hiding this comment.
Because the new normalizer only writes newline-terminated lines, a backend error response such as a one-line JSON body from a non-200 /v1/chat/completions response stays in line_buffer. The tail is flushed only under !stream_error, so these error bodies are now dropped and the client gets an empty SSE stream instead of the backend error it used to see. Flush or wrap the buffered tail before sink.done() on the error path.
Useful? React with 👍 / 👎.
1 participant
Description
Fixes #2412 — `lemonade pull` printed "Model pulled successfully" even when offline mode prevented any download.
Root cause
When offline mode was enabled, `download_model()` returned early without downloading, but:
Fix
Server (legacy + SSE paths): includes a `"warning"` field in the completion response when offline mode caused the download to be skipped.
CLI: parses the `warning` field from the completion event and prints "Model pulled with warning: " with the warning message, instead of "Model pulled successfully".