feat: add llama_server loader (Stage B1 of the parser-migration roadmap)#111
Conversation
Drives llama-server as a subprocess over its native OpenAI-compatible HTTP API instead of binding llama.cpp in-process, unlocking real concurrent request handling that the llama_cpp loader's per-instance lock couldn't offer. Reuses LlamaCppPreflight's GGUF/RAM-budget sizing, adjusted for llama-server's --parallel slot splitting.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new llama_server loader that runs a llama-server subprocess over its native OpenAI-compatible HTTP API, enabling multi-slot concurrency and offloading chat templating, tool-calling, and reasoning parsing. The changes include the LlamaServerInfer implementation, configuration schemas, preflight estimators, and extensive integration and unit tests. The review feedback highlights several critical improvements for robustness: handling httpx.TransportError during streaming to prevent 500 errors, adding a minimum value constraint to the parallel configuration to avoid division-by-zero, closing the httpx.AsyncClient connection pool on shutdown to prevent socket leaks, using asyncio.get_running_loop() instead of the deprecated get_event_loop(), and adding errors="replace" to subprocess.Popen to prevent decoding crashes during log draining.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Catch httpx.TransportError mid-stream (dropped connection, crashed subprocess) and emit a clean SSE error chunk + [DONE] instead of letting it propagate as an unhandled 500. - Use asyncio.get_running_loop() instead of the deprecated get_event_loop() inside the async _launch(). - Decode subprocess output with errors="replace" so a non-UTF-8 byte in the child's stdout/stderr can't kill the log-drain thread and block the pipe.
- LlamaServerConfig.parallel now requires >= 1; 0 or negative would divide/multiply into a broken -c launch arg or a zero-slot server. - shutdown() previously dropped the httpx.AsyncClient reference without closing it, leaking the connection pool's sockets. Schedule aclose() as a background task when a loop is running, keeping a strong reference so it isn't GC'd before it completes.
Docker images now bundle a digest-pinned llama.cpp build at /opt/llama.cpp (upstream's official server images; CUDA 13 build for the gpu variant, which falls back to its dlopen'd CPU backends when no GPU is visible) with MSHIP_LLAMA_SERVER_BIN preset to a wrapper that scopes LD_LIBRARY_PATH to the subprocess. The loader passes -ngl n_gpu_layers when the deployment reserves GPUs and forces -ngl 0 otherwise, since Ray only sets CUDA_VISIBLE_DEVICES for actors that reserve GPUs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new llama_server loader that runs a llama-server subprocess and proxies requests over its native OpenAI-compatible HTTP API, enabling multi-slot concurrency. The changes include Dockerfile packaging, configuration models, preflight budget estimators, and extensive tests. The code review feedback highlights several critical improvements for robustness: avoiding blocking the main asyncio event loop during subprocess launch and cleanup, using dedicated daemon threads for log draining to prevent thread pool exhaustion, catching httpx.HTTPError instead of httpx.TransportError to safely handle timeouts, catching OSError during stream closure, and omitting empty tool_calls in stream chunks to ensure client compatibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
… alignment - Offload subprocess spawning (fork/exec) to run_in_executor to avoid blocking the main asyncio event loop during actor model cold-starts. - Run wait-and-kill process termination logic in a background daemon thread on shutdown to prevent blocking the Ray Serve actor's event loop. - Use dedicated threading.Thread log-draining daemon threads instead of loop.run_in_executor to avoid exhausting the default executor thread pool. - Explicitly close stdout/stderr pipes on shutdown to immediately unblock and terminate background log-draining threads. - Catch httpx.HTTPError instead of TransportError during health check probes and completions to safely handle network timeouts. - Set tool_calls=tool_calls or None in ChatCompletionStreamResponse choices to omit empty tool calls and maximize OpenAI compatibility. - Added comprehensive unit test covering the non-blocking and timeout fallback behavior during shutdown.
…on (Stage B3) - Define ParsedChatOutput DTO representing the parsed output and build_from_parsed builder in modelship/openai/chat_utils.py. - Rewire _project_chat_response in modelship/infer/llama_server/llama_server_infer.py to extract message content, reasoning, and tool calls into ParsedChatOutput and delegate to build_from_parsed. - Add unit tests verifying multi-choice construction, custom created timestamps, and finish reason fallback derivation under tests/test_chat_utils.py. - Run formatting and static type checking with Ruff and Pyright to ensure zero warnings or errors.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new llama_server loader that runs a llama-server subprocess and proxies requests over its native OpenAI-compatible HTTP API, enabling multi-slot concurrency. Key changes include Dockerfile configurations, documentation, configuration models, and extensive tests. The review feedback highlights critical issues in the LlamaServerInfer implementation, specifically: thread-safety concerns when mutating and iterating over the log-draining deque, a resource leak where failed launch attempts do not trigger self.shutdown() to clean up processes and threads, and a potential uncaught RuntimeError when calling loop.create_task on a closed or closing event loop during shutdown.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…hutdown issues in llama_server loader - Initialize and apply threading.Lock synchronization to all operations (append, clear, join) on self._recent_log_lines to prevent RuntimeError: deque mutated during iteration. - Invoke self.shutdown() on _EarlyCrashError in start() to cleanly close pipes, reap the failed subprocess, and terminate old log threads before retrying launch on a fresh port. - Wrap the entire loop acquisition and task creation logic inside shutdown() within the try...except RuntimeError block to safely catch and ignore RuntimeError raised when the event loop is closed/closing during teardown.
…g for llama_server loader (Stage B4) - Add `--embedding` flag to llama-server launch args under `usecase: embed` and implement `create_embedding` with a secure, projection-based response model. - Add `mmproj` config field to `LlamaServerConfig` and configure resolve-on-startup. - Pass `supports_image` based on `mmproj` config state to `normalize_chat_messages` to enable gateway-side multimodal content validation. - Default `max_ongoing_requests` to `parallel` for the `llama_server` loader. - Support logprobs and top_logprobs forwarding, as well as Choice/Stream-choice projection. - Add comprehensive test coverage in tests/test_llama_server_infer.py and document prompt cache drop in docs/model-configuration.md.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the llama_server loader, which runs a llama-server subprocess and proxies requests over its native OpenAI-compatible HTTP API to support multi-slot concurrency. The changes include Dockerfile packaging, configuration schemas, preflight RAM estimation, and comprehensive integration tests. The review feedback highlights several robustness improvements for the new loader, such as ensuring proper cleanup on startup failures to prevent resource leaks, handling API error payloads returned within 2xx responses (both streaming and non-streaming), avoiding AttributeError or other exceptions during interpreter teardown in __del__ and shutdown, and safely handling falsy but valid values when resolving timestamps.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- Wrap the entire startup retry loop in a try...except Exception block to guarantee that self.shutdown() is called on any launch, wait-healthy, or timeout exception. - This prevents potential background subprocess and log-draining thread resource leaks on startup failures.
…in llama_server loader - Add an explicit 'error' key check to create_chat_completion and create_embedding post-response processing. - If a 2xx response contains a JSON body with an 'error' key, extract the error message and gracefully return an ErrorResponse instead of trying to project malformed choices/embeddings. - Add robust unit testing verifying inline error payloads for both chat completions and embeddings.
…erver - Add an explicit 'error' key check on decoded JSON chunks in _stream_chat_completion. - If a chunk contains an 'error' key, yield an encoded error chunk, send the stream termination [DONE] chunk, and terminate the generator by returning. - Add unit testing to verify that mid-stream errors are correctly intercepted, output, and terminate the stream gracefully.
- Add explicit None checks for module-level _pending_client_closes before accesses. - Catch AttributeError and TypeError in addition to RuntimeError during loop task scheduling inside shutdown() to handle arbritrary garbage collection order of module-level globals during python interpreter teardown.
- Use contextlib.suppress(BaseException) to wrap self.shutdown() inside __del__. - This ensures any shutdown exceptions due to garbage collection of referenced modules/globals are silently ignored during interpreter exit, preventing noisy warnings.
|
/gemini review |
…ings projection - Replace the 'or' operator with an explicit 'is None' check for the 'created' field in _project_embedding_response. - This ensures that valid falsy numeric values (like 0 for a timestamp) are not unintentionally overridden with the current time fallback.
There was a problem hiding this comment.
Code Review
This pull request introduces the new llama_server loader, which runs a llama-server subprocess and proxies requests over its native OpenAI-compatible HTTP API. It includes Dockerfile updates, documentation, configuration schemas, preflight estimation, and extensive tests. The review feedback focuses on improving robustness, specifically by wrapping JSON parsing in try-except blocks, validating that responses and stream chunks are dictionaries to prevent potential crashes, and refining the fallback logic for finish_reasons in multi-choice completions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…rdcoding stop Previously a finish_reasons list shorter than choices silently fell back to "stop" even when the choice had tool calls; now it uses the same tool-calls-aware derivation as the no-finish_reasons case.
…-server resp.json() could raise JSONDecodeError uncaught on a malformed body, and a valid-but-non-dict payload (e.g. list/string) would reach the response projection helpers and raise AttributeError. Applies to non-streaming chat and embedding responses as well as individual SSE stream chunks.
resolve_all_model_sources already resolves llama_server_config.mmproj to an absolute path before the actor launches, same as the primary model path (_resolved_path). The actor was redundantly re-resolving it with a try/except fallback that added no robustness, since a failure there just falls back to the already-resolved value.
model-configuration.md gets a full loader section (field table, tool_choice and response_format gaps/wins vs llama_cpp, vision, embeddings, examples) plus the missing loader/llama_server_config table entries; CLAUDE.md and AGENTS.md architecture maps and sharp-edges gain llama_server coverage; config/examples gets a working llama-server.yaml. This loader shipped across several PRs without any of this ever landing.
Mirrors llama_cpp integration coverage the llama_server loader never got:
tool-call markers quoted inside reasoning not double-counted, the
response_format cluster (json_schema constrains unprompted output,
non-stream + stream, and coexists with tool_choice=none), GPU offload
(chat + tool calling), and real embeddings through a live binary
(previously mock-only). Also adds tool_choice=required/named-function gap
regression tests and a response_format+reasoning free-win test called out
in the roadmap but never written.
Found along the way: bare response_format={"type": "json_object"} (no
schema key) is not enforced by llama-server despite its own docs claiming
support, verified directly against the b9859 binary. Documented as a gap
test rather than asserted as parity.
Stage C1 of the parser-migration roadmap: build_vllm_request, render_and_params, extract_prompt_token_ids, make_parsers, derive_reasoning_ended, and generate wrap every vllm.entrypoints/ vllm.parser/vllm.v1.engine call the vllm loader needs, so a future vLLM bump's blast radius stays confined to one file. vllm_infer.py is untouched (still runs OpenAIServingChat) -- C3/C4 repoint it onto these functions.
Stage C2 of the parser-migration roadmap. llama_cpp's non-stream tool/reasoning path now parses via the shared ChatOutputStreamer (parse_chat_completion_text, promoted from a private helper) and builds its response through chat_utils.build_from_parsed instead of parsers.streaming.build_chat_completion_response -- proving the builder works against a second real parsed-output source (llama_server via B3 was the first). Streaming and the transformers loader are untouched.
Stage C3 of the parser-migration roadmap. Non-stream chat completions now go through the engine_ops pipeline (render_and_params -> generate -> parse -> build_from_parsed) instead of vLLM's OpenAIServingChat monolith; streaming is unchanged. Covers n>1, logprobs, and strips vLLM extension fields structurally (response is built from modelship types only). Adds BaseInfer.run_cancellable(), a generic disconnect-guard usable by any loader (poll RawRequestProxy.is_disconnected() alongside in-flight work, cancel whichever loses, call an overridable on_generation_aborted hook) -- vLLM is the first caller since AsyncLLM.generate() already self-aborts on task cancellation. Real GPU integration run caught and fixed a pre-existing bug: init_serving_chat never passed reasoning_parser to OpenAIServingRender, so a reasoning-only deployment's engine_ops-based parser silently lost reasoning extraction.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new llama_server loader that runs GGUF models via a llama-server subprocess, supporting concurrent requests, vision, and embeddings, and implements a generic non-streaming disconnect guard (BaseInfer.run_cancellable). It also refactors the vllm loader to quarantine internal touchpoints and bypass OpenAIServingChat for non-streaming requests. The review feedback suggests wrapping the llama_server loader's non-streaming chat and embedding requests in self.run_cancellable to handle client disconnects, adding a defensive guard against a potential IndexError in vllm logprobs indexing, and replacing a production control flow assert statement with a defensive check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Routes LlamaServerInfer's non-stream chat completion through BaseInfer.run_cancellable, cancelling the in-flight httpx call to the llama-server subprocess (freeing its slot) instead of running a request to completion for a client that already left. Fixes a latent bug run_cancellable's polling exposed: RawRequestProxy is constructed with registry=None for internal warmup calls across several loaders, and is_disconnected() crashed on that instead of treating it like a dead registry (assume still connected).
assert is stripped under python -O; a completed non-stream request with no prompt_token_ids now returns a 502 ErrorResponse instead of being one flag away from a silent AttributeError further down.
Bypasses OpenAIServingChat for streaming the same way non-stream already was: engine_ops.stream_chat_completion drives Parser.parse_delta per choice and yields modelship's own SSE chunk types directly, so extension fields can no longer leak and the loader stops depending on vLLM's ~800-line monolith for this path. Also adds BaseInfer.run_cancellable_stream, a streaming counterpart to run_cancellable, so a client disconnect mid-stream aborts the engine request via the same explicit polling mechanism non-stream uses instead of relying solely on implicit ASGI-layer cancellation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new llama_server loader that runs GGUF models by launching a llama-server subprocess and proxying its native OpenAI-compatible HTTP API. It also refactors the vllm loader to use a quarantined engine_ops module for rendering and parsing, and adds generic disconnect guards (run_cancellable and run_cancellable_stream) to BaseInfer. Feedback on the changes highlights two critical issues in the new llama_server loader: the streaming chat completion path is not wrapped in run_cancellable_stream, which could cause resource leaks upon client disconnects, and the id field is missing from the ChatCompletionStreamResponse instantiation in _project_stream_chunk, violating the OpenAI protocol.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…projection Prevents an IndexError in build_chat_logprobs if token_ids and top_logprobs sequences are ever mismatched in length.
…p chunk ids
Ray Serve doesn't propagate client disconnects across the process
boundary, so the streaming path now races the SSE consumption against
the same cross-process disconnect signal run_cancellable already uses
for non-stream requests, closing the httpx stream (and freeing the
llama-server --parallel slot) as soon as the client is gone.
Also stamps each ChatCompletionStreamResponse chunk with the actual
completion id instead of a fresh random uuid per chunk, matching the
non-stream path's id=data.get("id") or request_id convention.
C3/C4 already moved all real chat work onto engine_ops; OpenAIServingChat was only being constructed to serve as a truthy/None capability gate, never actually called. Drop its import/construction and gate on hasattr(self, "openai_serving_render") instead, matching the transformers loader's existing gating pattern.
… llama_server Adds a dedicated BaseInfer.create_response hook so vllm/llama_server build Responses items directly from the parsed DTO instead of round-tripping through a ChatCompletionResponse; the gateway route now just dispatches the original request to a new ModelDeployment.respond() method. Non-streaming only for now (falls back to unsupported for stream=True and every other loader) pending native streaming support.
…a_server Feeds ResponsesStreamTranslator directly from each loader's typed stream chunks (engine_ops.stream_chat_completion for vllm, a newly-shared _raw_stream_chunks for llama_server) instead of going through the chat SSE text round trip, with a new translator.fail() path for mid-stream errors. Also fixes a latent bug where responses_request_to_chat's hardcoded stream=False was never flipped back for streaming requests, so llama_server would have answered a streaming call with a single JSON object.
Routes vllm to wheels.vllm.ai's +cpu index under the cpu extra (PyPI doesn't host these wheels, so this needed explicit index wiring, same pattern as llama-cpp-python's cu130 wheel). Also fixes a real bug this surfaced: vLLM's CPU backend repurposes gpu_memory_utilization to mean host-RAM fraction rather than VRAM, so the existing 0.9 GPU default would reserve 90% of node RAM and crash every CPU deploy at worker init — num_gpus: 0 vllm deploys now default it to 0.4 unless set explicitly. This unblocks a non-GGUF (safetensors/AWQ/GPTQ) CPU path for models like gemma whose tool-calling doesn't work on llama.cpp's parser, ahead of the planned llama_cpp loader removal.
llama_server has had full feature parity (embeddings, vision, logprobs, concurrency) since Stage B4, plus real request concurrency via --parallel slots and native /v1/responses shaping from Stage D, leaving no reason to keep both GGUF loaders. llama_cpp's one real gap, gemma tool calling, isn't fixed by llama_server either — the supported path is loader: vllm on the CPU extra (config/examples/vllm-cpu.yaml), landed in Stage E0. Removes the llama_cpp loader module, config schema, preflight registration, llama-cpp-python dependency and its CUDA wheel/CI plumbing, and migrates catalog/profile/example configs and tests onto llama_server. Docs updated throughout; docs/architecture.md's /v1/responses section also brought up to date with the Stage D native-shaping design it had drifted from.
vLLM 0.24's Transformers fallback backend absorbs the long-tail architecture use case with continuous batching; every other transformers usecase already has a production home (embed/vision -> vllm, chat GGUF -> llama_server, STT -> vllm/whispercpp, TTS -> kokoroonnx/orpheus). bark was an unused experiment with no replacement.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a major architectural shift by replacing the llama_cpp loader with a new llama_server loader, which proxies a llama-server subprocess to enable true multi-slot request concurrency. Additionally, the transformers loader and the bark TTS plugin have been removed to streamline dependencies, while the vllm loader has been updated to support CPU-only deployments with adjusted host RAM utilization defaults. The Responses API (/v1/responses) is now natively integrated into both the vllm and llama_server loaders, and generic disconnect guards have been introduced to cancel in-flight requests. Feedback on these changes suggests a robust improvement to the SSE streaming parser in the llama_server loader to silently ignore empty keep-alive data lines, preventing potential log flooding from JSON decoding errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| data_str = line[len("data:") :].strip() | ||
| if data_str == "[DONE]": | ||
| return |
There was a problem hiding this comment.
In Server-Sent Events (SSE), empty data: lines are commonly used as keep-alive ticks to prevent connection timeouts. Parsing an empty string with json.loads will raise a JSONDecodeError and log a warning, which can flood the logs. Consider skipping empty data_str values silently.
data_str = line[len("data:") :].strip()
if not data_str:
continue
if data_str == "[DONE]":
return
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the in-process llama_cpp and transformers loaders with a new llama_server loader that proxies a llama-server subprocess. It also introduces CPU support for the vllm loader, removes the bark TTS plugin, and refactors the /v1/responses API to be handled natively by the individual loaders. Review feedback suggests explicitly setting the type field to "function" when instantiating ToolCall in engine_ops.py to ensure consistency and prevent potential validation errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ToolCall( | ||
| id=tc.id or f"chatcmpl-tool-{random_uuid()}", | ||
| function=FunctionCall(name=tc.name, arguments=tc.arguments), | ||
| ) |
There was a problem hiding this comment.
For consistency with llama_server_infer.py and test_chat_utils.py, and to prevent potential ValidationErrors if type is a required field in ToolCall without a default value, we should explicitly set type="function" when instantiating ToolCall.
| ToolCall( | |
| id=tc.id or f"chatcmpl-tool-{random_uuid()}", | |
| function=FunctionCall(name=tc.name, arguments=tc.arguments), | |
| ) | |
| ToolCall( | |
| id=tc.id or f"chatcmpl-tool-{random_uuid()}", | |
| type="function", | |
| function=FunctionCall(name=tc.name, arguments=tc.arguments), | |
| ) |
modelship/openai/parsers/ mixed a large dead streaming/parsing engine (only ever driven by the now-removed transformers and llama_cpp loaders) with a small amount of still-live vLLM auto-detection plumbing. Delete the dead half (ChatOutputStreamer, per-family tool-call/reasoning parsers, the Phase A responses adapter's chat-completions round trip) and extract the live half (chat-template classification, reasoning-suppression probing) into two small flat modules. Also drop _resolved_skip_special_tokens, a write-only leftover from the same removed loaders, and fix resolve_all_tool_parsers to validate explicit tool_call_parser names against vLLM's own ToolParserManager instead of a hand-maintained registry — this also fixes a real mismatch where FunctionGemma auto-detection produced "function_gemma" instead of vLLM's registered "functiongemma", which broke the documented gemma-on-CPU example at preflight.
1 participant
Drives llama-server as a subprocess over its native OpenAI-compatible HTTP API instead of binding llama.cpp in-process, unlocking real concurrent request handling that the llama_cpp loader's per-instance lock couldn't offer. Reuses LlamaCppPreflight's GGUF/RAM-budget sizing, adjusted for llama-server's --parallel slot splitting.