Repository navigation
fix(video): align the CLI and server video-frames fallback - #1777
Merged
Merged
Conversation
The video-frames fallback from #1322 left five follow-ups on its two fronts. The CLI announced every --video clip with one sentence naming their summed frame count, placed after all frames, while the server gives each clip its own sentence ahead of its own frames. The sentence now lives in multimodal::video, both fronts call it, and the CLI renders each clip's sentence before that clip's frames through a new CliPromptMedia layout. Text-only renders still carry the sentences ahead of the question. CLI frame PNGs went into the shared temp directory at the default mode. They are now written 0600 into a per-run 0700 directory (PrivateTempDir, std only) that is removed when the run ends. The server resolved MLXCEL_VIDEO_DIR_ALLOWLIST and spawned the ffmpeg probe on a Tokio worker. Startup now resolves the allowlist into AppState and warms the probe for video-capable models, in the single-model server and per router model load. The native-video path skips the allowlist entirely for bodies without a clip and otherwise resolves it on the blocking pool. render_chat_prompt takes the request by value and expands it in place instead of cloning the whole body. The three wiring routes pass a CancellationToken whose drop guard lives in the handler future. It is checked before the clip loop, before each clip is resolved, and at the start of the blocking decode, and a cancelled expansion answers 499 instead of 400. Validated with the affected lib scopes, the generate bin scope, the ffmpeg-gated fallback tests, clippy and fmt. Closes #1766
inureyes
added
type:enhancement
The test comment claimed the pre-#1766 server decoded every remaining clip after a disconnect. Dropping the handler future already stopped that loop; what the check guards is a caller that keeps polling after the token fires, which the comment now says.
For a template without image content items the CLI joined two clips' lead sentences and the question with blank lines, while the server's typed-message render concatenates text parts with no separator, so InternVL3, DeepSeek-VL2, FastVLM, Molmo and dots.ocr prefilled different prompts for the same clips. The flattening now lives in chat_template::flatten_template_text, the server's typed and raw message builders call it, and the CLI flattens its text-only renders through it. A lib test pins the prepare_chat_request render and a bin test pins the resolve_cli_prompt render to the same string; the bin test fails with the old join. Review follow-ups in the same area: ChatCompletionRequest::has_video_urls replaces a video_urls().is_empty() check that cloned inline data:video payloads; the route helper returns before allocating a cancellation token when the request has no clip or the checkpoint has no fallback; PrivateTempDir::create validates its prefix as a plain name, like write_file does; the writable-allowlist warning runs from resolve_video_request_inputs, so router-loaded models warn too; the Cancelled and 499 docs now say the path is defensive; supported-models.md states the parity for bodies that list clips before the question.
This was referenced Sep 11, 2026
9 of 14 tasks
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 the five follow-ups #1759 left on the video-frames fallback. The CLI and the server now render the same prompt for the same clips, and the server's request path neither blocks nor copies the body for it.
What changed
video_frames_lead_textmoved tomultimodal::videoand both fronts call it.CliVideoFramesholds one entry per clip. A newCliPromptMedialayout renders each clip's sentence immediately ahead of that clip's frames, matchingapply_video_frame_expansion. The issue's plan to prepend the sentences to-pwould not have worked, because the CLI renders every image before the text item.-pitself is no longer rewritten.flatten_template_text(server::chat_template). Before this, the CLI joined the parts with\n\nand the server with no separator, so families such as internvl3, deepseek-vl2, fastvlm, molmo and dots.ocr got different prompts on the two fronts. The CLI's no-template path now uses the same join.PrivateTempDir(std only) creates a per-run 0700 directory whose prefix and file names must be plain names. It writes each PNG withcreate_newat 0600 and removes the directory on drop.MLXCEL_VIDEO_DIR_ALLOWLISTintoAppState, warms the ffmpeg probe, and warns about writable allowlist directories. The router does the same on each model load.extract_chat_video_pathschecks a non-allocatinghas_video_urls()and returns early for bodies without a clip. Operator-visible change: the fallback reads the allowlist once, so a directory created later needs a restart (documented).render_chat_prompttakes the request by value and expands it in place.expand_request_video_partscreates aCancellationTokenonly when the request actually has a clip to expand, and holds its drop guard in the handler future. The token is checked before the clip loop, before each clip, and when the blocking decode starts. A correction to the issue: the guard fires only when the future is dropped, which already stops the loop, so the check that matters in production is at decode start, where a queued decode now never begins.Cancelledmaps to 499 as defensive code; no live route can receive it.Validation
mlxcel-corelib binary aborted mid-run with[METAL] Command buffer execution failed: Discarded (victim of GPU error/recovery) (kIOGPUCommandBufferCallbackErrorInnocentVictim), the error macOS reports when another process's GPU fault forces a recovery. Re-run alone, that binary passes 1,731/1,731. This PR does not touchmlxcel-core. On c5204bf the gate passed in one run: 123 binaries, 11,024 passed. Workspace clippy with-D warnings, fmt and themake verify-*contract checks are clean.\n\njoin and the prefix check.gemma-3-4b-it-4bit, two synthetic clips (the red square moves left to right, the blue one top to bottom; first and last frames checked by eye) and--video-max-frames 4:Prompt tokens:and the server'susage.prompt_tokensare both 2,129. The CLI prints one frame line per clip. The server returns HTTP 200, and/apply-templateshows two lead sentences and eight image markers.--models-dirover the store) gives the same 2,129 for the same body./v1/chat/completions/input_tokensreturns 57 for this body, before and after this PR. It counts the rendered template text without expanding image soft tokens. That is a separate property of the route, not a regression here, and it is noted as a follow-up rather than changed.Not changed (follow-ups)
write_video_temp_filestill writes a 0644 file that a disconnect can leave behind.input_tokensdoes not count image soft tokens.videooraudioitems but notimagestill receives the content list on the server and a flattened string on the CLI.Closes #1766