Repository navigation
feat: Support text generation with Qwen3-VL (CORE-276) - #14298
Conversation
📝 WalkthroughWalkthroughThis PR adds Qwen3-VL, a multimodal text encoder combining a LLaMA-based backbone with vision processing and DeepStack visual feature injection. The implementation extends the model registry, adds position_ids and deepstack input support throughout the LLaMA forward and generation paths, introduces a vision feature merger for per-layer injection, unifies text and vision processing in a Qwen3VL model class, and provides CLIP-compatible wrappers and tokenizers that replace image placeholders with actual embedding data during inference. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
comfy/text_encoders/llama.py (1)
773-786:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInject DeepStack before the decoder layer.
These features are added only after
layer(...)has already consumedxand aftercurrent_kvhas been produced. That shifts the conditioning by one layer and leaves the cached K/V for the affected layers missing the injected visual state during decode.Suggested fix
for i, layer in enumerate(self.layers): + if deepstack_embeds is not None and i < len(deepstack_embeds): + x[visual_pos_masks] = x[visual_pos_masks] + deepstack_embeds[i].to(x) + if all_intermediate is not None: if only_layers is None or (i in only_layers): all_intermediate.append(x.unsqueeze(1).clone()) @@ if current_kv is not None: next_key_values.append(current_kv) - - # DeepStack: add per-layer visual features into the first len() decoder layers at image positions (Qwen3-VL) - if deepstack_embeds is not None and i < len(deepstack_embeds): - x[visual_pos_masks] = x[visual_pos_masks] + deepstack_embeds[i].to(x)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@comfy/text_encoders/llama.py` around lines 773 - 786, The DeepStack visual embeddings are being added after calling layer(...) which causes the layer to compute attention keys/values without the visual state and shifts conditioning by one layer; move the block that applies deepstack_embeds (using deepstack_embeds, visual_pos_masks, and x) to just before calling layer(...) so the layer receives x already augmented, and ensure past_kv/current_kv/next_key_values usage remains unchanged so cached K/Vs include the visual features during decode (adjust the order around the layer(...) invocation and past_key_value handling accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy/text_encoders/qwen_vl.py`:
- Around line 91-114: The function qwen2vl_mrope_position_ids incorrectly
assigns compact post-image MRoPE indices across every remaining slot (including
padding); update it to accept an attention_mask (or compute one from
embeds_info) and only advance/assign the post-image position_ids for real tokens
(where attention_mask == 1). Specifically, change the post-image assignment that
writes to position_ids[:, end:] to iterate or mask by attention_mask[:, end:],
compute the compacted indices only for non-padded token positions (preserving
their relative order), and leave padded positions unchanged so later real tokens
keep the correct smaller MRoPE indices; keep all other logic (start, end,
offset, grid-derived ranges) in qwen2vl_mrope_position_ids. Ensure parameter
list (seq_len, device) is updated to include attention_mask and update callers
accordingly.
In `@comfy/text_encoders/qwen3vl.py`:
- Line 6: The module currently imports Qwen2Tokenizer at top-level which forces
transformers to be imported on module import; move the import into
Qwen3VLSDTokenizer.__init__ so it’s loaded lazily (follow the same pattern used
in qwen35.py): remove the top-level "from transformers import Qwen2Tokenizer"
and instead perform the import inside Qwen3VLSDTokenizer.__init__ (e.g., "from
transformers import Qwen2Tokenizer" or equivalent try/except within the
constructor) and assign or use Qwen2Tokenizer there so the transformers
dependency is only imported when an instance is constructed.
---
Outside diff comments:
In `@comfy/text_encoders/llama.py`:
- Around line 773-786: The DeepStack visual embeddings are being added after
calling layer(...) which causes the layer to compute attention keys/values
without the visual state and shifts conditioning by one layer; move the block
that applies deepstack_embeds (using deepstack_embeds, visual_pos_masks, and x)
to just before calling layer(...) so the layer receives x already augmented, and
ensure past_kv/current_kv/next_key_values usage remains unchanged so cached K/Vs
include the visual features during decode (adjust the order around the
layer(...) invocation and past_key_value handling accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46c77a71-c1ef-43dc-93fe-272ed6075859
📒 Files selected for processing (5)
comfy/sd.pycomfy/text_encoders/llama.pycomfy/text_encoders/qwen35.pycomfy/text_encoders/qwen3vl.pycomfy/text_encoders/qwen_vl.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_extras/nodes_textgen.py`:
- Line 38: The max_length UI bound in io.Int.Input("max_length", default=512,
min=1, max=32768) must be validated against the model's actual context capacity
and interpreted as decode tokens vs total context to avoid runtime OOB errors;
update the node (comfy_extras/nodes_textgen.py) to query or accept the
encoder/model context limit (from comfy/text_encoders/qwen3vl.py or the model
metadata) and clamp the user-provided max_length so that prompt_length +
generated_tokens <= model_max_context, and add a clear validation/error message
when the requested decode tokens would exceed the model’s effective context
window (document that max_length is decode tokens, not total context, or compute
max_decode = model_max_context - prompt_length and enforce max_length <=
max_decode).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ca0bc6f-dc70-408b-8cc8-0965153b7225
📒 Files selected for processing (1)
comfy_extras/nodes_textgen.py
|
I confirm this is working for still images, video will need support in a separate PR in the future. |
Upstream merged native Qwen3-VL support (Comfy-Org#14298), adding comfy/text_encoders/qwen3vl.py plus helpers in qwen_vl.py / llama.py / qwen35.py. The JoyImage port previously shipped its own duplicate Qwen3-VL implementation (comfy/text_encoders/qwen3_vl.py); that duplication is now removed and the JoyImage text encoder rides on the upstream stack. - Delete comfy/text_encoders/qwen3_vl.py. - Rewrite comfy/text_encoders/joyimage.py to subclass upstream comfy.text_encoders.qwen3vl. The JoyImage checkpoint is a stock qwen3vl_8b, so only JoyImage-specific behavior is overridden: * Qwen3VL8B_JoyImage.forward builds the 3D MRoPE position ids and injects deepstack visual features on the conditioning path. Upstream Qwen3VL only does this inside generate() via build_image_inputs; SDClipModel.forward never passes those kwargs. The JoyImage node feeds an image through the encoder (clip.tokenize(prompt, images=[..])), so the override reuses build_image_inputs to reproduce the multimodal conditioning that Llama2_.forward already accepts kwargs for. * preprocess_embed keeps JoyImage's bicubic+clamp image preprocessing (process_qwen3vl_image) instead of upstream's bilinear path, to preserve validated DiT numerics. * JoyImageTokenizer keeps the JoyImage system-prompt templates, suppresses the Qwen3 <think> block, and raises on image-placeholder count mismatch. * JoyImageTEModel keeps the drop_idx=34 system-prompt strip and the pre-final-norm layer tap (layer="hidden", layer_idx=-1). - sd.py QWEN3VL_8B_JOYIMAGE branch: apply the same state-dict prefix remap the sibling QWEN3VL branch uses (model.language_model.->model., model.visual.->visual., lm_head.->model.lm_head.) so the checkpoint loads into the upstream Qwen3VL namespace, then use the module-level llama_detect. Detection ordering is preserved: the JoyImage discriminator is checked before the generic Qwen3-VL deepstack key. No changes to llama.py / qwen3vl.py / qwen_vl.py / qwen35.py.
|
The default template workflow for Flux2klein-KV doesn't work any longer - since I've updated today - if I use But it should work, despite the fact that the default Flux2klein-KV template asks for qwen_3_8b_fp8mixed.safetensors, which I've never used. Instead I've always used the qwen3-vl-8b_fp8_scaled.safetensors and a qwen3-8b-abliterated-v2-fp8.safetensors version worked too! Now, I assume because the changes were made for qwen3-vl text generation, only the vl-less CLIP qwen3-8b-abliterated-v2-fp8 still works with flux-2-klein-9b-kv-fp8 - but the image quality is slightly worse and Black Forest Labs uses qwen3vl. Using qwen3vl_8b_bf16 or qwen3vl_8b_fp8 is impossible. KV - OK 
KV - NOT OK 
[INFO] Model Flux2 prepared for dynamic VRAM loading. 9364MB Staged. 0 patches attached. Force pre-loaded 185 weights: 59 KB. For KV I can use 4 images without OOM on VRAM 8 GB (64GB system) - still possible with that vl-less CLIP model - that's good! Also good: I can now use qwen3-vl-8b_fp8_scaled for generating text, captioning an image. The crash probably happens here:
in: comfy/ldm/flux/model.py and
in: comfy/ops.py |
Which template are you referring to exactly? |
|
This one: 
|
|
Okay I had old template pack and didn't see it, thanks.
Where are you seeing this though? I only see them using it for the prompt enhancer, not as text encoder. I can restore the ability to use the VL version as text encoder, I just don't think that's the intended way to use either klein model? |
I can't find it or I'm wrong about Black Forest Lab's use of the vl-Qwen3, sorry! But it actually worked for me and I've tried the vl-less clip months ago, when I first tested Flux2-klein-KV. But the results were bad (burned, spotty), so I replaced it with the vl-one and probably thought it was meant to be like this with the vl-Qwen 3 clip. I didn't use it for the older Flux2-klein 9b model. |
I can confirm it does work in the sense that it does generate results, didn't test further than that though, so I made a PR to allow it work like it used to, while still also allowing you to use it for text generation, so there's an use case where you can have workflow that does prompt enhancement or such with same model load. |
|
Great, thanks!!! |
|
@kijai do you think I can replace my current use of Ollama node to leverage Image to Text generation with your new node pack? 
|
Adds support to do text generation with image understanding using existing Qwen3-VL 4b and 8b models.
https://huggingface.co/Comfy-Org/Qwen3-VL
Video support is still missing from both 3 and 3.5, that would be separate PR to add for both at same time.
New file
comfy/text_encoders/qwen3vl.py— model, tokenizer, and TE wrappers (4B uses tied word embeddings, 8B has a separatelm_head).interleaved_mropeconfig flag (rope_dims=[24, 20, 20]).model.visual.deepstack_merger_listweights; 4B vs 8B split by merger output dim.Shared-code changes (additive,
None-safe defaults)BaseGenerate.generate— threadsposition_ids/deepstack_embedsthrough prefill and advances the MRoPE position during decode.Llama2_.forward— optional DeepStack injection at visual positions.qwen2vl_mrope_position_idshelper (qwen35 now reuses it).Compatibility
No behavioral changes to other models — the rope code is untouched, and the
generatechange is a no-op whenposition_idsisNone.