task: assess the v0.33.2 upstream sync — blocked on ADR 0009 - #232
Merged
Conversation
Co-authored-by: Codex <noreply@openai.com>
Nothing has set Grammar since the CGO engine removal took its writers out; it survived as a read-only pass-through on the llama-server path and a comment claiming it is set before dispatch. Remove the field and the dead pass-through. llama-server keeps its wire-level grammar field, which the "json" format conversion still uses.
Token ids are int32 throughout the runner, so every caller reading ids out of an int32 array narrowed the widened value right back. Make Int and Ints return int32 and Float return float32, matching Floats, and require the exact dtype instead of accepting and widening every integer and float width: no caller read anything through those paths but int32 tokens. Ints and Floats also copied out of the array's buffer without evaluating it first, so reading an array still in flight after an async dispatch could return unwritten data, and correctness depended on every call site remembering an explicit Eval. Evaluate in every reader, matching the scalar readers, which already wait through item. An available array costs a status check and an in-flight one waits for its event; only a never-dispatched array evaluates a graph.
The MLX runner accepted the API's format field but did not enforce it:
requests asking for JSON or a JSON Schema got unconstrained text, and
clients had no way to tell.
Enforce format with xgrammar: each sampling step masks the logits to
the tokens the grammar allows, so every emitted token and the end of
generation are valid under the constraint. Sampling, penalties, and
logprobs see the constrained distribution, and "json" yields a JSON
object, as the API documents and the llama-server path already
enforces. Only sampling waits on the mask; the forward pass is
dispatched before it, so constrained decoding stays pipelined.
The grammar engine is a dynamic library alongside MLX; when it is
missing, plain inference is unaffected and structured requests fail
with an explicit error. Constrained requests decode without
speculative decoding for now.
Decoding 256 tokens of a book-list schema on qwen3.8:27b-mlx (M5 Max,
seed 42, thinking off); pre-decode is the request time spent before
the first token:
unconstrained ~65 tok/s pre-decode ~70 ms
unconstrained, no draft ~32 tok/s pre-decode ~70 ms
JSON schema ~32 tok/s pre-decode ~70 ms
Schema and draft-less decoding are equal to within 0.1 tok/s in
paired adjacent requests, and a cold grammar compile adds nothing
measurable to pre-decode. The gap to unconstrained decoding is the
disabled draft model.
Fixes ollama#16563
Co-authored-by: Daniel Hiltgen <daniel@ollama.com>
…rage
Model load code eagerly evaluated every weight fold (expert stacking,
gather transposes, gate/up fusing) as it was built, with the folds
running on the GPU against lazily loaded tensors: Metal committed
command buffers that waited on file reads, and macOS kills command
buffers that stall too long, so loading a large model from a slow
volume aborted with "Command buffer execution failed". The eager evals
also kept every layer's fold sources alive until the post-load sweep,
transiently holding roughly twice the expert weights on MoE models.
Build the folds lazily and let the runner's weight eval run them, and
on Metal materialize the loaded tensors with CPU reads before any
weight graph exists: no command buffer is ever committed waiting on
file data, at any storage speed, and fold sources free as their folds
execute. CUDA loads read at dispatch and skip the pre-pass. Models no
longer evaluate weights at load; on Metal, tensors the model does not
retain are now read before the sweep frees them.
Measured on an M5 Max, warm page cache, greedy outputs bit-identical:
before after
nemotron-3.5-lightning:30b-mlx 1.9s 39.7GiB 1.45s 24.7GiB
qwen3.6:35b-mlx 1.27s 22.5GiB 1.1-1.2s 22.4GiB
nemotron, reads at ~60MB/s aborts in 6s loads in 346s
Fixes ollama#17902
* MLX: Qwen3.8 Flash Next support * review comments
Build context was missing the new cmake common utility.
Pi's Edit() only set baseUrl when creating a new ollama provider entry. On subsequent launches it preserved whatever baseUrl was already in ~/.pi/agent/models.json, so switching OLLAMA_HOST to a remote server had no effect — Pi would still connect to localhost. Edit() now ensures baseUrl reflects the current OLLAMA_HOST. Models() returns nil when the stored baseUrl no longer matches, so the launcher only calls Edit() when the host has actually drifted. User-customized api and apiKey fields are still preserved.
The MLX gemma3 port implements only the text stack, while gemma3 as GGUF runs on llama-server with vision. Once MLX takes priority for architectures both engines support, a registered gemma3 would route the model to the engine that cannot serve images. No gemma3 safetensors manifests were ever published, so removing the architecture affects no existing installs and keeps gemma3 on llama-server.
Largely from the llama-server work.
21 commits. The payload pin moves b10488 -> b10630, but all seven compat patches apply cleanly at the new pin (verified against a fresh clone), including 801. 903 is still required -- ggml-org/llama.cpp#27044 is open -- though apply-clean is not proof it is still correct there. Preflight expectations do need re-measurement because the pin moved. Six files conflict, twelve hunks. Most are unions of disjoint additions: the mlxrunner runner.go collision is our 181 lines of memory/cache config against upstream's 23-line logitsWidth, not a semantic clash. The llama_server.go conflict is benign -- upstream deleted the dead req.Grammar branch and our gate is elsewhere in the file. The blocked hunks are all one decision: upstream has implemented structured output in the MLX runner and it collides with the fork's constrained-sampling layer. Adopting upstream retires ADR 0009 -- ~78 references and three test files -- so the merge was aborted rather than pushed through. Recommendation and both options are in the task doc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
21 commits, v0.33.0..v0.33.2. Payload pin b10488 -> b10630; all seven compat patches apply cleanly at the new pin, 801 included. Six files conflicted, twelve hunks. Most were unions of disjoint additions -- runner.go's largest was our 181 lines of memory/cache config against upstream's 23-line logitsWidth, not a semantic clash. llm/llama_server.go keeps our applyCompletionFormat helper and adopts upstream's removal of the dead req.Grammar branch (7027546 deleted the field). The real decision was the MLX constrained-sampling collision, taken as ADR 0033: adopt upstream's grammar engine, supersede ADR 0009's implementation. The fork's speculation advantage was measured inert (#201: drafted=0, cold-start deadlock, three fixes never picked); upstream reaches the same no-draft behaviour deliberately and compiles the grammar asynchronously. ADR 0009's guarantee is retained, not dropped: upstream's parseGrammar errors on a format it cannot honour rather than silently dropping the constraint, and is stricter than ours (schema size limit, UTF-8 validation). client_format_test.go now asserts that same table against parseGrammar. The raw-GBNF rejection is structural now that the field is gone. constrain.go (478 lines) is left in place but is provably unreachable: s.matcher is set only by attachGrammar, which is no longer called, and every masking path guards on nil. Deleting it is a follow-up, kept out of this merge to keep the diff reviewable. go build ./... clean; server, model/..., llm and the full x/... suite green; gofmt clean. NOT runtime-validated: unit tests do not exercise MLX kernels, and the pin bump invalidates preflight expectations. Build + full preflight re-measurement and a 903 functional check are required before deploy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
glennneuber
added a commit
that referenced
this pull request
Sep 4, 2026
Gates 1 and 2 of the v0.33.3 fold: the mechanical merge and the no-GPU test set. 28 conflicts, resolved per docs/maxusai/tasks/upstream-sync-2026-09-04.md and Glenn's decisions D1-A and D2. Payload pins take upstream's values: LLAMA_CPP_VERSION b10760, MLX_VERSION 37c26e5755da..., MLX_C_VERSION c74db5307cc8... (the mlx/compat/ carry patch is deleted with upstream's bindings regeneration). D1-A — the gemma4 MLX package stays the fork's --------------------------------------------- media.go / vision.go / media_test.go / vision_test.go: ours whole-file (add/add). gemma4.go: ours, taking only upstream's package doc line — every other upstream hunk there is vision/audio wiring (parseMultimodalConfig, buildMasks threaded through DecoderLayer/Attention, the media-placeholder embed masking, loadAudioWeights) and is incompatible with the fork's visionChunkMask design. gemma4_test.go / gemma4_moe_test.go: upstream's (pure mlxtest port, no vision content); vision_test.go's nine useMLXTestThread sites ported to mlxtest.Run since upstream deleted that helper with gemma4_moe_test.go. Excluded under D1-A (they reference upstream media.go symbols the fork does not define — multimodalConfig, MultimodalEmbedder, makeClippableLinear, m.MM / m.Vision / m.Audio, visionSoftTokenBudget — and do not compile here): x/models/gemma4/audio.go x/models/gemma4/audio_test.go x/models/gemma4/process_image.go Kept, and dead under D1-A: x/models/gemma4/process_audio.go and process_audio_test.go compile standalone (they depend only on the new x/mlxrunner/model/audio package), so they stay as the audio front-end a D1-B spike would need. x/mlxrunner/model/audio is a clean add and is likewise unused. isGemma4Renderer is restored in server/renderer_resolution.go: upstream deleted it when it started advertising gemma4 audio, and this fork ships gemma4 vision without audio, so server/images.go's suppressAudioCapability keeps its gemma4 branch (the auto-merge had silently dropped it) and server/model_list_cache.go keeps its mirror. Upstream's removal of the gemma4 *vision* suppression is taken in both files; server/images_test.go keeps the fork's "keeps vision, suppresses audio" cases. The per-request budget path is unchanged and asserted at compile time (gemma4/media.go's `_ base.MediaBudgetModel = (*Model)(nil)`): PrepareMediaWithBudget, resolveImageBudget, base.MediaBudgetModel and x/mlxrunner/media.go's bm.PrepareMediaWithBudget dispatch all survive, so image_max_tokens cannot fall through to PrepareMedia for gemma4. D2 — ADR 0010's arithmetic on upstream's plumbing ------------------------------------------------ Upstream's shapes are kept everywhere: the IncludeIntermediateMetrics request field (api/llm/mlxrunner client + pipeline), ChatHandler's `includeIntermediateMetrics := req.Format != nil && currentFormat == nil`, the firstPassMetrics capture with its non-terminal blanking, and the PromptEvalCachedCount pass-through (routes both handlers, llama_server completion + chat, mlxrunner client, pipeline's `len(session.inputs) - len(session.remaining)`). The fold stays the fork's. Upstream's `else if Applying && r.Done` block is dropped because ADR 0004's pass1 summing below it does the same job; running both would count pass one twice. What upstream's plumbing buys is better inputs, not a different fold: at the ChatHandler transition site pass one now has a report of its own (every chunk of a deferring pass carries metrics), so reportedPassMetrics() prefers it over ADR 0010's textual reconstruction — that report is the runner's cache-inclusive prefill, image-embedding tokens included, which is exactly what ADR 0010's subtraction exists to recover. transitionPassMetrics()/transitionPromptDelta remain as the fallback for a runner that reports nothing, and the delta is still computed only from a reconstructed pass (it needs the textual count on both sides). PR #238's `deferring` gate is kept on both ChatHandler transition sites, and llm/llama_server.go keeps result.final, applyCompletionFormat, visionServerArgs, kvCacheFlagValues and ggmlCublasComputeTypeEnv. One assertion in upstream's new restart test is re-expressed for the fork (server/routes_generate_test.go, wantMetrics): upstream reclassifies the continuation's prefill as generation work, so it reports prompt duration as pass one's alone and folds pass two's prefill into eval duration. The fork keeps ADR 0004's summing — each pass's prefill is prefill, each pass's decode is decode. Counts, cached count and eval count now match upstream's expectations exactly; only that duration split differs, and adopting upstream's would move every recorded think+format cell's tok/s, which is a measured surface and a decision of its own. New test TestChatThinkFormatTransitionMetricsReportedPassOne asserts the folded count on a vision-shaped request: it fails if the fold ever falls back to a textual count that cannot see the image surplus. MLX bindings and the test harness --------------------------------- x/mlxrunner/mlx/{mlx,stream}.go: upstream's single-buffer error contract (lastError, mlxError[T], mlxCheck[T]). ClaimOSThread and the `__thread _mlx_thread_owned` flag are deleted, with their callers in x/create/mlxthread.go, x/mlxrunner/server.go and the two vision oracle tests. ADR 0017's guarantee is re-expressed on x/internal/mlxthread, whose Start locks the worker goroutine and deliberately never unlocks; the ADR carries a status amendment saying so, and AGENTS.md / docs/development.md now point at mlxthread and mlxtest.Run. memory.go's MemoryLimit / SetMemoryLimit / SetCacheLimit are ported off mlxCall onto mlxError — mlx_get_memory_limit, mlx_set_memory_limit and mlx_set_cache_limit all survive the MLX-C regeneration with unchanged signatures (x/mlxrunner/mlx/generated.h:5301-5316, include/mlx/c/memory.h:34-38). x/mlxrunner/client.go is mechanical: mlxRunnerEnvDefaults, CacheThrashingCheckEnv, budgetWithOverride and MemoryLimitEnv are untouched. x/mlxrunner/pipeline.go keeps every deferred cleanup on guardClose and keeps the stopper block, with upstream's cachedPromptCount and per-chunk metric enrichment folded in. x/internal/mlxtest: upstream's Run / RunSubtest / SkipIfUnavailable. Of the 18 mlxtest.Setup sites, 17 were in upstream-owned files and upstream ported them itself; the two vision oracles (vision_golden_test.go, vision_e2e_test.go) already drive mlxthread.Start and only lost their ClaimOSThread line — both still compile and skip cleanly without a payload. x/mlxrunner/mlx/memory_test.go's fork-only SetCacheLimit test is ported onto upstream's withMLXThread(t, func(*mlxthreadtest.T)) signature. x/mlxrunner/constrain_bench_test.go and constrain_test.go are DELETED rather than ported: the bench calls mlxtest.Setup(b) and the new API takes only *testing.T, and constrain_test.go's skipIfNoMLX came from an upstream file that no longer defines it. Both drive MLX from a plain test goroutine, which is invalid under the shared-thread model. This brings forward part of ADR 0033's stated follow-up; constrain.go itself and the four non-MLX constrain tests are left for that PR. Build, CI and docs ------------------ cmake/mlx/CMakeLists.txt is the union: the fork's $ORIGIN RPATH block and quadmath in MLX_INCLUDE_REGEXES, upstream's cusolver / cusparse / nv[Jj]it[Ll]ink, OLLAMA_LIB_DIR destinations and license installs. .github/workflows/test.yaml is the union of the fork's path filters (as #232) with upstream's go_mod_changed filter, go_license job and MLX Darwin payload steps. Dockerfile auto-merged correctly — upstream's COPY mlx mlx removal and go-license step, the fork's ccache block. llama/compat: upstream's re-cut 001-llama-cpp-hooks.patch wins verbatim, 002/004/005/801/903 are untouched, and the README merged upstream's b10729 load_data_range paragraph into the fork's band text. docs/api.md takes upstream's prompt_eval_cached_count line and its reworded prompt_eval_duration, and keeps the fork's eval_count note; the fork's image_min_tokens / image_max_tokens and kv-cache-type docs live in docs/maxusai/ and were never in docs/api.md, so nothing was re-added. Gate 2 (golang:1.26.0, -u 1000:1000, caches on the 8 TB array) -------------------------------------------------------------- go build ./... PASS go vet ./... 1 finding, pre-existing go test ./llm/ -run 'TestImageTokensForSize|TestKVCacheType' PASS go test ./server/ ./model/... ./llm/ ./api/ ./convert/ \ ./x/structured/... ./x/mlxrunner/... ./x/internal/... \ ./x/models/... ./x/create/... PASS 4230 pass, 0 fail, 237 skip python3 docs/maxusai/vision-suite/preflight/test_verdicts.py 94 tests OK, 6 skipped (quality arm, pre-existing) The vet finding is tokenizer/bytepairencoding_test.go:542:5 "result of slices.Collect call not used". It is not this merge's: the file is byte-identical to v0.33.3's and unchanged since before the merge base, and a control `go vet ./...` on origin/main in the same image reports the same single finding. No new vet findings. 188 of the 237 skips are "MLX not available: failed to load MLX dynamic library" — the golang image has no native payload — and a skip is not a pass: every MLX kernel path, including TestVisionGoldenParity and TestVisionEndToEnd (which also gate on OLLAMA_VISION_E2E), is unexercised here and is Gate 4's job. gofumpt -l over the 120 Go files this merge touched lists one: server/routes_generate_test.go, whose two hunks are pre-existing fork code in TestChatFormatPassthrough's table (identical hunks on origin/main), outside anything changed here; left alone rather than smuggling unrelated reformatting into a merge commit. SPEC H11 -------- Once the tag is cut, this fold's build identity is 0.33.3-dynres-0-g<this merge's sha>. That is a NEW build identity, not an equivalence to any 0.33.2 stamp: the payload moved (b10630 -> b10760) and the MLX/MLX-C pins moved with it, so no 0.33.2 measurement carries over and nothing recorded here may be folded into a 0.33.2 table (ADR 0032, ADR 0011 rule 5). Not verified here: no native build, no GPU, no preflight. Gates 3-6 stand.
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
6 participants
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.
Assessment only; no merge.
mainis untouched — the merge was attempted and aborted cleanly.The good news
The payload pin moves b10488 → b10630, which is normally the expensive part. It isn't: all seven compat patches apply cleanly at the new pin, including the
801meter merged today. Verified withgit apply --checkagainst a fresh clone of llama.cpp at b10630.The
llm/llama_server.goconflict is benign — upstream deleted the deadreq.Grammarbranch, and ourqwen25vlgate is elsewhere in the file. Most mlxrunner conflict is a union of disjoint additions (our 181 lines of memory/cache config vs their 23-linelogitsWidth), not a collision.The blocker
Upstream now implements structured output in the MLX runner, and it collides with the fork's constrained-sampling layer:
Constraint *structured.GrammarGrammar *grammarCompilationcompileFormat(), syncgrammarEngine.prepare(), async, overlaps prefillconstrainedStep+ matcherpipelinedDecoder+ grammar; "a constrained session never drafts"Both reach the same effective behaviour. Ours is correct but inert (#201 — cold-start deadlock,
drafted=0, three candidate fixes never picked). Upstream's is simpler, maintained, and async.Adopting upstream retires ADR 0009 (MLX pure-Go constrained sampling) — ~78 references across
x/and three test files. That is a decision about a recorded architecture, not a merge resolution, so I stopped rather than pushing it through.Recommendation is adopt upstream (our layer is measured inert; upstream is actively developing this surface), but it needs a superseding ADR and it's your call.
Also worth knowing
Nothing in the 21 commits touches the fp16 fault — no qwen25vl/fp16/cuBLAS work, and ollama#18070 is still open, so the gate stays load-bearing.
903is still required (ggml-org/llama.cpp#27044 open), and applying cleanly is not proof it's still correct at b10630 — that needs a functional check.Full options, resolutions table and acceptance criteria in the task doc.