Skip to content

fix(qwen-image): drop the txt_seq_lens kwarg removed by diffusers - #190

Merged
Rockdu merged 2 commits into
mainfrom
fix/qwen-image-drop-txt-seq-lens
Aug 18, 2026
Merged

fix(qwen-image): drop the txt_seq_lens kwarg removed by diffusers#190
Rockdu merged 2 commits into
mainfrom
fix/qwen-image-drop-txt-seq-lens

Conversation

@Rockdu

@Rockdu Rockdu commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

What

Stop passing txt_seq_lens into QwenImageTransformer2DModel.forward. It stays a
per-sample input — collate_cond_for_sample_batch still reads it to derive the text
mask and the padding width — it just no longer travels in the collated dict that the
trainer splats into the model.

Why

txt_seq_lens was removed from both QwenImageTransformer2DModel.forward and
QwenEmbedRope.forward after diffusers 0.39. Under the pin in #189
(f53d5520, post-0.39 main) the qwen-image training forward raises
TypeError: forward() got an unexpected keyword argument 'txt_seq_lens'.

Numerically a no-op on 0.38

Removing the kwarg cannot move a single bit on the current pin:

  • In 0.38 the parameter only triggered a deprecate() warning in
    QwenImageTransformer2DModel.forward — nothing read it.
  • The RoPE length comes from compute_text_seq_len_from_mask (identical in 0.38 and
    f53d5520), passed to pos_embed as max_txt_seq_len=text_seq_len. That path never
    consults txt_seq_lens.
  • QwenEmbedRope.forward would fall back to max(txt_seq_lens) only when
    max_txt_seq_len is None, which the transformer never does.

Verified bitwise under #189's diffusers pin

tests/e2e/short/test_qwenimage_pickscore_grpo_5xGPU.py run on this branch inside the
pinned-diffusers image, compared against the standard recorded on 0.38:

image radixark/miles_diffusion:test-pr-189
diffusers 0.40.0.dev0 @ f53d552036a0d1bd5570782a39cd40cfabf112bc
sglang 7605529
hardware 5x H200
command --num-rollout 2 (the test's own args, unmodified)
metric series points vs standard
rollout/reward/raw_num_samples 2 identical
rollout/reward/raw_mean 2 identical
rollout/reward/raw_median 2 identical
rollout/reward/raw_std 2 identical
train/log_prob_old_idx_0 4 identical
train/log_prob_new_idx_0 4 identical
train/log_prob_mean_abs_diff 4 identical
train/model_output_mean_abs_diff 4 identical
train/model_output_rel_max 4 identical
train/grad_norm 4 identical
[e2e-metrics] PASSED: 10 metric series match test_qwenimage_pickscore_grpo_5xGPU.json

32/32 points bit-for-bit, so the 311 commits between v0.38.0 and f53d5520 leave the
qwen-image train and rollout numerics untouched. The e2e is the guard for this change,
hence no new unit test.

Not covered here

test_wan22_pickscore_grpo_17xGPU_single_node_4xGPU_proxy still OOMs under #189's pin.
That failure is memory headroom, not numerics: at the OOM the trainer needed 1.31 GiB
with 1.25 GiB free while a rollout engine still held 25.3 GiB and the colocated
pickscore reward 4.7 GiB on the same device. The wan-side diffusers delta is a fp32
rope buffer pin (~0.5 MiB) and one added hidden_states.contiguous() (~32 MiB after
the ulysses-4 split) — enough to tip a device already at 99% occupancy, and tracked
separately.

@Rockdu
Rockdu marked this pull request as ready for review August 18, 2026 12:38
@Rockdu Rockdu added the run-ci-e2e Run e2e metric-regression tests on this PR label Aug 18, 2026
@zhihengy

Copy link
Copy Markdown
Collaborator

LGTM :)

@Rockdu
Rockdu merged commit 8decc4d into main Aug 18, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-ci-e2e Run e2e metric-regression tests on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants