Skip to content

Command A+: batch image tiles across requests in the vision encoder - #900

Merged
rohansanda merged 2 commits into
mainfrom
rohan/command-a-plus-encoder-batching
Oct 27, 2026
Merged

rohansanda merged 2 commits into
mainfrom
rohan/command-a-plus-encoder-batching

Conversation

@rohansanda

@rohansanda rohansanda commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Follow-up to #838 suggested by @arun-elr. The vision node encoded one request's tiles per call, so concurrent image requests waited on each other. Now one call takes the tiles of every request in the step, encodes them in chunks of 64, and splits the output back per request. 64 is about where the per-tile cost stops dropping on GB200, so the cap just bounds activation memory (~1.6 GB).

For that to be safe, a tile has to get the same embeddings no matter what else is in the call, and it didn't:

  • cuDNN picks the patch conv's algorithm by batch size, which changed embeddings enough to flip greedy replies. The patch embedding is now the same weights applied as a matmul.
  • cuDNN attention built a new plan for every new tile count, about 0.5 s each on GB200. The tower skips it now.

With both changes, batched and per-request embeddings are bitwise equal on GB200.

Median TTFT in ms, 4x GB200 TP4, 16 output tokens, before → after:

request c1 c4 c16 c32
text 41 → 41 80 → 78 125 → 125 155 → 161
1-tile image 62 → 61 151 → 137 415 → 256 714 → 365
4-tile image 81 → 81 185 → 167 522 → 452 816 → 765

Text throughput is unchanged (bench_http: 287 / 1716 / 3379 / 3849 tok/s at c1 / 8 / 32 / 64).

Testing:

  • CPU suite as in CI, plus ruff and test/ci. The new tests check that two requests batched together, with a chunk boundary inside one of them, get the same embeddings as running each alone, and that the matmul patch embedding matches the conv.
  • On GB200 with MSTAR_TEST_HF=1: the Command A+ tests, KV layouts and startup graph validation.
  • HF checkpoint parity (6 prompts x 32 tokens): everything is within the thresholds. Greedy matches 190/192 against 191/192 before. Both misses are exact bf16 ties on our side, like the existing one.

cuDNN picks the patch conv's algorithm by batch size, so a tile's embeddings changed with the number of tiles in the call, enough to flip greedy replies, and cuDNN attention built a new plan, about 0.5 s on GB200, for every new tile count. The patch embedding is now the same weights as a matmul and the tower skips cuDNN attention, so every tile gets the same embeddings at any batch size and no shape stalls.
…der call

The vision node ran one request's tiles per call, and the encoder is launch-bound at a few tiles, so concurrent image requests queued behind each other. Tiles are independent and fixed-size, so a step now concatenates them, encodes in chunks of 64 tiles to bound activation memory, and splits the output back per request.
@rohansanda
rohansanda marked this pull request as ready for review October 26, 2026 23:27
@rohansanda
rohansanda requested a review from NSagan271 October 26, 2026 23:28
@mstar-review

mstar-review Bot commented Oct 26, 2026

Copy link
Copy Markdown

Automated review

🟡 No findings on 1caf65a, light review

Description

  • The TTFT table doesn't name the harness or the command that produced it (concurrency sweep, warmup, image sizes, request count), so it can't be reproduced; the text-throughput line names bench_http but gives no flags either.
  • The text row at c32 goes from 155 to 161 ms, and the description doesn't say whether that is run-to-run noise or a cost of running the larger batched vision steps on rank 0, which also runs the LLM.
What the review checked
  • engine._forward always takes forward_batched now that can_batch returns True; it passes engine_inputs.request_ids in the same order as the inputs list that preprocess concatenates, so the per-request split in forward_batched lines up with the rids.
  • The vision node has no accelerator-graph config and runs alone on rank 0 (configs/command_a_plus_tp4.yaml), so there are no padded rows and no lockstep concerns.
  • The forward still works for a single request: forward accepts the extra tile_counts key from preprocess through **kwargs.
  • The unfold/permute/flatten patch embedding gives the same (c, kh, kw) channel-major order as Conv2d's weight.flatten(1) and the same row-major patch order as conv.flatten(2).transpose(1, 2); the new CPU test checks this against the conv.
  • The new tests (test_vision_node_batches_requests..., test_patch_embedding_matches_the_conv) run on the CPU with tiny encoders and need no GPU or weights.
  • Splitting into TILES_PER_CALL chunks is mechanical processing of a batch the scheduler already formed, not batch selection, so it does not break invariant 1. The forward makes no host syncs (invariant 3).

Advisory, not a merge gate. Rules: AGENTS.md. Reply to discuss.

@Gaurav-Shah05 Gaurav-Shah05 left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've a TP8 run queued on H100s, and I will post the numbers here soon. Please wait for them before merge.

@Gaurav-Shah05 Gaurav-Shah05 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run Configuration:
8xH100s at TP8 with configs/command_a_plus_tp8.yaml: three launches per side, alternating, plus one pair with the API server's preprocessing at 4 threads. The vision step and memory rows come from the vision tower alone on one H100.

Numbers:

main this PR Δ
Vision step, 16 requests, 117 tiles 387 ms 288 ms 1.35x ↓
Extra TTFT at an unseen tile count 33 to 45 ms 1 to 18 ms ~31 ms ↓
TTFT p50, c16 4857 to 4910 ms 4938 to 4958 ms 1.0x
TTFT p50, c1 / c4, preprocessing at 4 threads 207 / 373 ms 176 / 313 ms 1.18x ↓ / 1.19x ↓
Vision tower peak memory, 117 tiles 0.30 GiB 1.82 GiB 6.0x ↑
Text output, c8 794 to 830 tok/s 778 to 802 tok/s 1.0x

Batching rarely engages, because the API server tiles images one at a time (26 to 264 ms each) and feeds the encoder slower than it drains them.

Good to merge.

@rohansanda
rohansanda merged commit 6aabf71 into main Oct 27, 2026
12 checks passed
@rohansanda
rohansanda deleted the rohan/command-a-plus-encoder-batching branch October 27, 2026 16:46
dghosef added a commit that referenced this pull request Oct 28, 2026
Main brought (since 6cfe39c): #892 Parakeet and Nemotron ASR with the
transducer as a graph loop (and the shared mrope_position_ids helper),
#917 pytest 9.2 flag proxies, #897 one captured all-to-all for Ulysses,
#915 NCCL graph registration off by default, #836 Qwen3-Omni Thinker
prefix caching (ARNodeSubmodule.cuts_tensor_inputs, marks_dynamic_dims,
the conductor's _streamed_partitions), #909 bench cells, #900 Command A+
batched image tiles.

Conflict:
- mstar/model/submodule_base.py: main's ARNodeSubmodule.cuts_tensor_inputs
  beside the stack's token_stop and its stop checks. Independent class
  attributes; both kept.

Ported onto the stack's mechanisms:
- mstar/model/parakeet/submodules.py: check_stop_batched drops the
  step_walk parameter (the stack's three-argument signature) and reads
  its "active" flags through HostRows.first_values instead of its own
  row-map loop. Its stop is a flag, not a token, so it has no TokenStop.
- test/modular/test_transducer_loop.py: the call without step_walk.

The conductor's _streamed_partitions (#836) sits beside _pick_replicas /
_register_request; it reads only partition_states and _resource_specs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dghosef added a commit that referenced this pull request Oct 28, 2026
Brings #858's merge of main: #892 Parakeet / Nemotron ASR, #917, #897
Ulysses all-to-all, #915, #836 Qwen3-Omni Thinker prefix caching, #909,
#900.

Conflicts:
- docs/adding_models.rst: main's marks_dynamic_dims sentence and #859's
  compile_regions paragraph are independent facts; both kept, in one
  paragraph.
- mstar/model/qwen3_omni/submodules.py (3 hunks, plus auto-merged hunks
  in the same class): AudioEncoderSubmodule. Two mechanisms for one cause
  (the HF encoder recompiling per clip length): #859 runs it eagerly
  (disable_torch_compile) and packs a step's clips into calls of at most
  MAX_BATCH_FRAMES; #836 builds the encoder's chunk tensors in
  prepare_inputs with mark_dynamic so the per-clip forward compiles once
  (marks_dynamic_dims, _max_seqlen). One mechanism: #859's class verbatim.
  Under disable_torch_compile the engine never compiles the encoder, so
  #836's concern (a recompiled graph rounding differently from the one
  whose output the prefix cache holds) cannot arise. The auto-merge had
  taken #836's kwargs-only forward beside #859's forward_batched, which
  calls forward with the clips positionally; that keep-both is undone.
  Main's VisionEncoderSubmodule (marks_dynamic_dims, prepared indices) and
  Thinker changes (cuts_tensor_inputs, split_inputs, mrope_position_ids,
  image_span) are kept; #859 did not change those. Imports: both
  cp_embed_accumulate and mrope_position_ids; the audio helpers only
  #836's prepare_inputs used are dropped.

Tests following from it (test/modular/test_qwen3_omni_prefix_cache.py):
- the test that #836's prepared chunk tensors embed a clip as the encoder
  does alone is deleted with that code;
- the compile test now asserts the audio encoder stays eager while its
  output is cached;
- the CUDA no-recompile test keeps its vision case only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dghosef added a commit to dghosef/mstar that referenced this pull request Oct 28, 2026
…igin/main 73c6c17) into merge/860-main

Brings main's mstar-project#892 Parakeet / Nemotron ASR, mstar-project#917, mstar-project#897 Ulysses as one
captured all-to-all (prefer_all_gather and the all-gather exchange gone,
Cosmos3 captures every tier under TP x SP), mstar-project#915, mstar-project#836 Qwen3-Omni Thinker
prefix caching, mstar-project#909 and mstar-project#900, with mstar-project#858's and mstar-project#859's resolutions.

No conflicts. Files both sides touched (docs/clients.rst,
docs/installation.rst, docs/models.rst, sequence_parallel.py,
cosmos3/submodules.py, test_accelerator_graph_capture.py,
test_flux2_klein_model.py): each merged change equals mstar-project#859's own delta
since 3166401, so nothing kept both sides. mstar-project#860's my_rows / gather_rows
(Wan2.2's CFG split) sit beside main's new ulysses_attention and use
CommGroup.all_gather, which mstar-project#897 left unchanged; nothing in mstar-project#860 passed
prefer_all_gather.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants