Feature/ideogram4 reference latent support#14567
Conversation
|
LGTM |
📝 WalkthroughWalkthroughThe Ideogram4 transformer model gains optional reference latent support. 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
comfy/ldm/ideogram4/model.py (1)
247-286: 🏗️ Heavy liftExtract the reference packing into a shared helper.
The conditional and image-only paths duplicate the same
ref_latents_methodparsing, tokenization, position-ID generation, andreference_image_num_tokenssetup. Centralizing this keeps future reference-position fixes from landing in only one path.Also applies to: 337-376
🤖 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/ldm/ideogram4/model.py` around lines 247 - 286, Extract the duplicate reference latents processing logic (which includes ref_latents_method parsing, offset calculation via index_ref_method and negative_ref_method conditionals, tokenization via _img_to_tokens, position ID generation via _image_position_ids, and list appending) into a shared helper method. This helper should accept the reference latent, method type, current indices/offsets, device, and other necessary parameters, then return the processed tokens, position IDs, and updated state. Replace both the loop at lines 247-286 and the duplicate logic at lines 337-376 with calls to this helper method to eliminate code duplication and ensure fixes apply consistently across both paths.
🤖 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.
Nitpick comments:
In `@comfy/ldm/ideogram4/model.py`:
- Around line 247-286: Extract the duplicate reference latents processing logic
(which includes ref_latents_method parsing, offset calculation via
index_ref_method and negative_ref_method conditionals, tokenization via
_img_to_tokens, position ID generation via _image_position_ids, and list
appending) into a shared helper method. This helper should accept the reference
latent, method type, current indices/offsets, device, and other necessary
parameters, then return the processed tokens, position IDs, and updated state.
Replace both the loop at lines 247-286 and the duplicate logic at lines 337-376
with calls to this helper method to eliminate code duplication and ensure fixes
apply consistently across both paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5614747c-1fec-46e9-b241-7500c29b7ad1
📒 Files selected for processing (2)
comfy/ldm/ideogram4/model.pycomfy/model_base.py
|
@comfyanonymous I need verification about the CodeRabbit nitpick if that should be addressed or ignored. |
|
Hi @silveroxides and thanks for submitting this. I wanted to test it but the workflow contains a bunch of custom nodes and some relatively complex logic. Would it be please possible to have a minimal version?
|
i thing you need ComfyUI_SamplingUtils, ComfyUI-CES [WIP]. The second one is in the manager "dev"; the first is on the author’s page |
|
I don't know what to do about the CES Conditioning formula. It is basically my method of not having to use the useless unconditional model by scheduling the negative conds with inverse positive conds for first step. Most other stuff in the workflow is nothing more than resolution selection or things that can be straight up deleted. ResolutionSelectionExtended is just an extended version of the core one. I have trouble reading the entire nodes 2.0 lauyout too since it obviously is not distinguishing missing nodes from missing models or missing images. |
|
Thanks @SvKora . ComfyUI-UtilsCollection is also an option as it acts as drop in for SamplingUtils. https://github.com/Clybius/ComfyUI-CES |
|
To put it blunt. The nodes present make up for what ComfyUI Core has been sorely lacking for a long time and it is very apparent in templates such as the Ideogram default one where math expressions and extract string from json complexity makes it look like a complete mess. But I guess I will have to make a simplified workflow because it feels like no one is able to take one apart and just figure stuff out anymore. |
|
I could not test it though cause I do not have the text encoder on my PC anymore. I only use my own int8 rowwise quants which still has not gotten support in ComfyUI Core. The one I used to do the workflow was temporary and now deleted. |
Thanks for providing the workflow. We receive many PRs so you will understand this helps. The passive aggressive tone does not. As far as |



This PR implements reference latent support for Ideogram 4.
Ideogram_4_reflatent.json