Skip to content

Isolate concurrent image positions and batch-MTP draft paths - #1242

Closed
ilumn wants to merge 6 commits into
Niko1221:mainfrom
ilumn:fix/multimodal-slot-isolation-current-main
Closed

ilumn wants to merge 6 commits into
Niko1221:mainfrom
ilumn:fix/multimodal-slot-isolation-current-main

Conversation

@ilumn

@ilumn ilumn commented Oct 6, 2026 •

Copy link
Copy Markdown

With --vision and concurrent batching, admitting another image or a text request could overwrite the image-position table still used by an active slot's captured decode graph. This PR gives each slot its own table so another admission or slot reuse cannot change that slot's rotary positions. It applies the same isolation to batch-MTP drafting and rejects malformed vision records before they can affect a later valid request.

This is the replacement for #994, which GitHub automatically closed after the upstream history rewrite. The recreation of the PR was requested here. It ports original commit 10114f8 onto the rewritten main at 82f46a8c8f475f001ad76d92f58f4a4f8ffb0253.

Changes

  • Give each batch slot its own image-position table on every stage. Copy the admitted request's positions before restoring draft state, and scope both batch-MTP drafter recording paths to that slot.
  • Guard verifier norm/RoPE and q/gate fusions when rows need slot-local tables; retain the fused global-position paths. Scope the per-token indexer fallback and grouped same-slot commits to the correct table.
  • Preserve the shared draft subset head's GGML type when binding a slot drafter. Talos testing found that the pointer was shared while its type stayed -1, so the first batch-MTP admission failed with unsupported native MMVQ GGML type. Copying that metadata fixes admission with the original subset head and the optional Q4 draft head.
  • Validate vision-record sizes, grids, and finite embedding values before use. Add parser regressions and CUDA graph checks for image and text/null tables, scope restoration, both visible devices, and a wrong-table negative control.

Validation on Talos

All 13 model-backed runs passed, with each checked batch token stream matching its solo reference:

Configuration Runs passed Coverage
One GPU, ordinary batching 4 Text batch, full text interleave, synthetic and encoder-produced mixed image/text
One GPU, --batch-mtp 5 The same four cases, plus the optional Q4 draft-head batch check
Two-GPU layer split 4 Text batch, supported staggered admission and next-turn comparisons, synthetic and encoder-produced mixed image/text

Mixed tests admit a different-grid image and text after the first image starts decoding. Greedy and seeded sampling, malformed-record rejection followed by valid recovery, cancellation, image-slot reuse, and image-to-text reuse all match their solo token references. Exactness controls include STRATA_IQ_MT_MIN=1, --pcie-frac 0, --adapt-every 1000000, and --no-prefill-borrow.

  • Full CUDA build for two RTX 5090s (sm_120) succeeded; the rebuild after the draft-head metadata correction also succeeded.
  • Final focused checks passed 3/3: vision_records_test, mrope_slot_graph_test, and rope_parity, with both GPUs visible.
  • Strict parser compilation and ASan/UBSan passed (detect_leaks=0; leak checking was not run).
  • Full CUDA CTest inventory: 93 passed, 3 missing-fixture failures, 2 skips (98 total). ple_parity, expert_parity, and pool_test lack their required development fixtures; iq_parity_fixtures and iq_parity skipped for unavailable fixtures. This inventory was run before the metadata correction; focused CUDA checks and affected model checks were rerun afterward.

Scope and coverage limits

  • Normal MTP works with a layer split: the drafter lives on the last GPU, and the two-GPU solo references in these tests accepted MTP drafts. The existing engine guard disables only per-slot --batch-mtp when a layer split or helper GPU is configured; its current slot drafter setup uses the first stage's sessions and head. That guard is unchanged by this PR.
  • Two-GPU BYIELD/resume was not validated. In the upstream interleave harness, the idle split-stage prompt path bypassed the chunk-boundary yield handling. The supported staggered-admission and next-turn sections passed in a scoped derivative of that harness; the later yield/resume and checkpoint sections were not counted as passing on two GPUs.
  • Validation used 64-token decode caps and small test images; it does not establish all-model, long-context, or long-duration coverage. Historical GPU results from Isolate concurrent image positions and validate vision records #994 are not counted as validation of this port.

Port the original slot-isolation fix onto the rewritten upstream main,
reconcile verifier fusion guards, and scope batch-MTP graph recordings.
Validate vision records and retain shared draft-head format metadata.

Original-author-commit: 10114f8
Superseded-PR: Niko1221#994
Talos-validation: CUDA build, focused regressions and 13 model checks passed
@ilumn
ilumn marked this pull request as ready for review October 6, 2026 20:10
@ilumn

ilumn commented Oct 6, 2026 •

Copy link
Copy Markdown
Author

Follow-up: #1253 adds opt-in batch MTP across serial multi-GPU layer splits (--batch-groups 1) on top of this PR at 8fc9490e9cb62e4d3c3ab5f09c1ca8d42f4a8623. This PR keeps its recovery and slot-isolation scope; the new feature is a separate commit and branch.

Talos validation passed all 11 model runs, including five-slot rotation, greedy/seeded lifecycle restores, and synthetic/real mixed images; host native checks also passed. Both matched-expert-profile benchmarks showed no throughput gain over ordinary batching, and pipelined batching was faster. The new PR stays draft with the measurements and limits documented: wider hardware/model/context coverage remains unrun, and helpers/shared-device stages/pipelined speculative groups retain guards.

@blange48

blange48 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Running this instead of my own port of #994 (I had resolved the #783 fused norm-rope conflict the same way, by guarding the fused paths when rows need slot-local tables). Rig: 4× RTX 5080 16 GB (PCIe Gen3), i9-10920X, Qwen3.8-Flash-Next IQ3_S, 262K, int8 KV, --layer-split 12,24,36 --trim-stage-weights, MTP spec 4, --batch 8 --batch-groups 4, conversation cache 8 GiB, on v0.1.40.1 plus our serve patches; HTTP server, T=0.7, 300 tokens. With --vision:

  • batch_test: 8/8 and 3/3 identical to solo. Throughput unchanged: 121 / 154 / 218 / 258 / 378 tok/s at 1 / 2 / 3 / 4 / 8 clients. It's in our production.
  • Staggered-admission test over HTTP (--pcie-frac 0 --adapt-every 1000000): a greedy image reply while 3 text requests are admitted at 0.8 / 1.5 / 2.5 s matched solo 1 time in 3. A text reply while 2 images are admitted matched 1 time in 2. But the text-only control (text + 3 texts admitted) also matched only 1 time in 2, so on this rig staggered admission changes greedy output independently of images. The image effect can't be isolated this way. Simultaneous admission (batch_test) is exact. A staggered-admission regression test in batch_test would make this visible.

@havok2-htwo

Copy link
Copy Markdown
Contributor

Thanks for this. We found the same problem in a code review on v0.1.43 (4x RTX 3090, --layer-split 12,25,37, --batch 16 in 4 pipelined groups of 4, --vision), before we turned --vision on, so we never served the mixed path. Our first fix, a server gate that runs image requests alone, made new text requests wait while a picture ran.

Our fix takes a different route:

  • We keep one table per device and grow it to 1 + B regions; Isolate concurrent image positions and batch-MTP draft paths #1242 gives each slot its own table. A slot row passes region base + cell as its rope position. The kernels already read that value as a table row, so no kernel changes. The base is added in stage_batch, in the indexer append and commit re-append, and in the slot drafter's position writers.
  • The fused norm+rope and q/gate paths stay unchanged. If I read Isolate concurrent image positions and batch-MTP draft paths #1242 right, batch windows take the unfused path there once --vision sets a table.
  • It is opt-in (STRATA_MROPE_SLOTS=1) and costs about 51 MiB per GPU at 262K context with 16 slots.

What it does not cover: record validation and the pipelined slot_cache change. Batch-MTP: the slot drafter's position writers add the base, but 0.1.43 turns batch-MTP off with pipelined groups, so that path never ran on a GPU. Image slots are never cached or parked. The patches sit on top of 0.1.43 plus our own series (0001-0048; 0049 needs at least 0042/0043), not main.

Validation (greedy, 640x480 and 512x512 pictures; replies up to 96 tokens in the text A/B, and in the mixed checks image replies up to 126 tokens and text replies up to 352):

  • Text tokens match the previous binary 30/30.
  • Text slots next to image slots match solo 17/17.
  • Image slots match their references 14/14.
  • All 4 groups ran both image and text slots.
  • Negative control (region copy skipped): 12 of 14 image comparisons differ. The two "picture B into image A's slot" comparisons did not catch the skip.

In production since 2026-10-13 22:35 CEST: 399 image requests in about 5.6 h, no engine errors. We did not log how many of them decoded next to text slots, and their output was not checked.

@blange48, on staggered admission: our checks admit requests while other slots decode and match solo with STRATA_IQ_MT_MIN=1 and --pcie-frac 0 --adapt-every 1000000 --no-prefill-borrow --suffix-draft 0. Without --no-prefill-borrow, slots that decode while a long prompt is read see the experts that read borrowed run on the CPU, which rounds differently. Upstream's tools/batch_interleave_test.py (a staggered-admission test, in since v0.1.39) says the same. If your text-only control ran without that flag, that may explain its mismatches.

Patches: 0049 (regions), 0050 (opt-in), 0051 (GPU check tool, built around our patch).

I'm happy to run #1242 on our 4x 3090 (sm_86) pipelined split (on 0.1.44 that is 8 slots in 4 groups of 2) with its own batch_test and mrope_slot_graph_test, plus an image-beside-text check like ours ported to main. If the maintainers would rather have the region approach, I can port it to main as a separate PR. This is meant as a second data point, not a replacement for #1242.

@Niko1221

Copy link
Copy Markdown
Owner

This is a correctness fix without a speed claim, so it is outside the speed pass; it stays on the list for the fix review.

@Niko1221

Copy link
Copy Markdown
Owner

Thanks for the pull request.

We're resetting the tracker for 0.2.0-beta. There are more than 1,300 open issues and pull requests, too many for us to review properly, so we're closing everything that isn't being worked on right now. Closed here does not mean rejected.

To reopen it takes about two minutes:

  1. Rebase on main if it no longer applies, and re-run your checks on 0.2.0-beta.
  2. Start the title with its area: [Engine], [GUI], [TUI], [Benchmark], [Docs] or [Other].
  3. Fill in the pull request template: the problem and result, and the validation (what you ran, on which revision, what you saw).
  4. Comment /reopen. A check runs and either reopens the pull request or lists what is still missing.

The rules are in CONTRIBUTING.md.

@Niko1221 Niko1221 closed this Oct 19, 2026
@brocktice

Copy link
Copy Markdown

This still reproduces on v0.2.2, so the fix looks needed.

On a single RTX PRO 6000 Blackwell (sm_120) running v0.2.2 with --vision and --batch, I ran six mixed image/text slot pairs and compared each batch token stream with the same request run alone. Five pairs were identical. One pair (image request in slot 0, a second request in slot 1) diverged from its solo reference at token 57, the same way on two runs. It matches the symptom this PR describes (one slot's image-position table being used by another slot's decode).

Applying this PR to v0.2.2 conflicts in src/core/verify.cpp, because the indexer append and query steps there were refactored into idx_append / idx_query / q_chain (some of which run on a side stream). I have not ported it, so I can't say yet whether the PR's fix lands cleanly in the new structure.

@ilumn, a rebase onto v0.2.2 would let this be re-tested. I'm not the author so I can't use /reopen here.

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.

5 participants