Repository navigation
[diffusion] comfyui: split batched DiT calls into per-row requests - #43163
OnePunchMonk wants to merge 4 commits into
Conversation
_cond_key used the shape plus the first and last scalar of prompt_embeds[0]. For Flux that is the pooled CLIP-L vector, so prompts that only differ in T5 got the same key and later steps reused the first prompt's T5 context. Hash every field drop_cached_fields removes instead. Fixes sgl-project#43107
Adapters now declare cached_extra_keys; drop_cached_fields and the cond key both read it, so H3's h3_* fields and sigmas are part of the key. The worker stored packed extras in one shared slot, so on a cache hit the positive cond could get the negative's h3_context/payload back. Store them per comfyui_cond_key instead. Hash nested dicts/lists by content with framing instead of repr().
ComfyUI batches CFG cond/uncond (and batch_size > 1) into one diffusion_model call. The executor crashed on it in should_suppress_logs, and the worker's comfyui path is per-sample anyway (req.timesteps is the schedule, seq lens are per request). Send one request per row and concatenate the outputs. Fixes sgl-project#43111
| # ComfyUI batches CFG cond/uncond (and batch_size > 1) into one call, | ||
| # but the worker's comfyui path is per-sample: req.timesteps is its | ||
| # schedule and seq lens are per request. Send one request per row. | ||
| return torch.cat( |
There was a problem hiding this comment.
Native worker batching would be a useful follow-up for throughput.
There was a problem hiding this comment.
will open a follow up pr for this. keeping this unresolved.
There was a problem hiding this comment.
hi @niehen6174 , opened the rfc here: #43411
| return self.adapter.unpack(output_batch.noise_pred, packed, x) | ||
|
|
||
| def forward(self, x, timestep, context, **kwargs): | ||
| batch = int(x.shape[0]) if torch.is_tensor(x) else 1 |
There was a problem hiding this comment.
is checking shape[0] == batch_size sufficient to distinguish batched inputs from shared conditions?
There was a problem hiding this comment.
Checked this. I think this is sufficient for now, since we do not support models like Trellis2 (those which share tensors). Pls tell me your thoughts on long-term maintenance, or a better way to generalise this.
bd935f2 to
f467ad9
Compare
Rename _row to _slice_batch_row. Recurse into dicts so ControlNet's control residuals are split per row, and reject tensors whose leading dim is neither 1 nor the batch instead of passing them through. transformer_options stays shared except its per-chunk entries (cond_or_uncond, uuids) and sigmas, which ComfyUI sets to one chunk's timestep, so row i takes sigmas[i % len].
f467ad9 to
a7d520e
Compare
|
Superseded by #43492 (rebuilt on current main; same 4 commits). Closing. |
Motivation
Fixes #43111.
ComfyUI puts CFG cond and uncond into one
diffusion_modelcall whenever their shapes allow it and memory permits (for Flux, T5 is fixed length, so that is the normal case), and a latent withbatch_size > 1is batched too. The ComfyUI executor crashed on the first such call:Fixing that line alone would not help, because the worker's comfyui path is per-sample:
ComfyUIPassThroughSchedulertakesreq.timestepsas the step schedule, and Flux'srequire_text_seq_lensexpects one seq len per row while the adapters send one per request.Depends on #43109, please merge that first (the first two commits here are #43109). Once rows go through the per-run conditioning cache one by one, main's old key makes two rows with the same text but different reference latents (Qwen-Image-Edit) share a key, and the second row silently runs on the first row's reference image.
test_batched_rows_with_same_text_keep_their_own_referencefails with this change on top of main's key and passes on #43109.Modifications
SGLDiffusionExecutor.forward: whenxhas batch > 1, run each row as its own request through the existing single-sample path andtorch.catthe outputs._rowslices tensors whose leading dim equals the batch (recursing into plain lists/tuples such asref_latents) and passes everything else through (transformer_options, shared refs). MiniMax-H3 passesxas[video, audio]and is not split, matching ComfyUI's native H3, which only supports batch 1.xare not split; same text with per-row reference latents keeps each row's own reference through the real cache path.Limitation: a batched call now costs B worker round trips instead of one. True batching on the worker would need per-row timesteps and seq lens in the comfyui request contract; that is a bigger change and could be a follow-up.
Accuracy Tests
CPU end-to-end: real ComfyUI KSampler (ComfyUI master 58b176f) with the plugin's
FluxExecutor+SGLDModelPatcher, worker half in-process (realbind_comfyui_session->FluxPipelineConfig.prepare_pos_cond_kwargs-> the DiT'stime_text_embed), and a noise_pred that depends on each row's conditioning. The reference is the same run with ComfyUI batching disabled (or each latent row sampled alone):GPU: not run. I set up a 2-GPU sanity run (FLUX.1-dev in comfyui mode: a 2-row batched call through the real
FluxExecutor, compared against each row run alone, plus the plugin'stest_flux_pipeline.py), but no 2x L40S container was scheduled within 10 minutes, so I stopped it. Happy to run it if someone can trigger GPU CI.Unit tests (CPU):
Speed Tests and Profiling
Batch-1 calls are unchanged. Batched calls (CFG > 1, batch_size > 1) used to crash; they now make one worker request per row.
Checklist
CI States
Latest PR Test (Base): ❌ Run #37953314216
Latest PR Test (Extra): ❌ Run #37953313899
Latest PR Test (AMD ROCm 10): ❌ Run #37953314129