huggingface / huggingface/diffusers
visualcloze model/pipeline review
- Dominant language
- Python
- Stars
- 34.5k
- Forks
- 7.3k
- Avg merge
- 3d 3h
- Merged PRs (30d)
- 91
Description
# `visualcloze` model/pipeline review
Commit tested: `0f1abc4ae8b0eb2a3b40e82a310507281144c423`
Review performed against the repository review rules.
Duplicate check: searched GitHub issues and PRs for `VisualCloze`, `VisualClozeProcessor`, `prompt_embeds`, `latents`, `generator`, `resolution`, `return_dict`, `get_layout_prompt`, `_resize_and_crop`, `upsampling_strength`, and slow-test coverage. No open duplicate found. Related merged PR: https://github.com/huggingface/diffusers/pull/12121 fixed a prior multi-image `VisualClozeProcessor` AttributeError, but not the remaining resize issue below.
Test status: attempted `.venv\Scripts\python.exe -m pytest tests/pipelines/visualcloze -q`; collection failed because this `.venv` torch build lacks `torch._C._distributed_c10d`.
## Issue 1: Default `generator=None` crashes
Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/visualcloze/pipeline_visualcloze_generation.py#L650-L690
Problem:
`generator` defaults to `None`, but `prepare_latents()` treats every non-`torch.Generator` value as an indexable list and evaluates `generator[i]`. Calling the pipeline without an explicit generator crashes before latent prep.
Impact:
The documented default API is broken. Users must pass a generator even though the signature says it is optional.
Reproduction:
```python
import torch
from diffusers import VisualClozeGenerationPipeline
pipe = object.__new__(VisualClozeGenerationPipeline)
try:
pipe.prepare_latents([[torch.zeros(1, 3, 16, 16)]], [[torch.zeros(1, 1, 16, 16)]],
torch.tensor([1.0]), 1, torch.float32, torch.device("cpu"),
None, vae_scale_factor=8)
except Exception as e:
print(type(e).__name__, e)
```
Relevant precedent:
`FluxFillPipeline.prepare_latents` accepts `generator=None`.
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/flux/pipeline_flux_fill.py#L686-L733
Suggested fix:
```python
sample_generator = generator[i] if isinstance(generator, list) else generator
```
## Issue 2: Precomputed embeddings cannot be used, and `latents` is ignored
Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/visualcloze/pipeline_visualcloze_generation.py#L423-L431
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/visualcloze/pipeline_visualcloze_generation.py#L830-L881
Problem:
`check_inputs()` rejects text prompts with `prompt_embeds`, but then also raises when `task_prompt` is missing, making `prompt_embeds` unusable. Separately, `__call__` accepts `latents` but never forwards it to latent preparation.
Impact:
Documented pipeline controls for prompt reuse and deterministic latent reuse do not work.
Reproduction:
```python
import inspect
import torch
from diffusers import VisualClozeGenerationPipeline
pipe = object.__new__(VisualClozeGenerationPipeline)
pipe._callback_tensor_inputs = ["latents", "prompt_embeds"]
try:
pipe.check_inputs(None, None, None,
prompt_embeds=torch.zeros(1, 4, 8),
pooled_prompt_embeds=torch.zeros(1, 8))
except Exception as e:
print(type(e).__name__, e)
print("prepare_latents accepts latents:",
"latents" in inspect.signature(VisualClozeGenerationPipeline.prepare_latents).parameters)
```
Relevant precedent:
`FluxPipeline.encode_prompt` only encodes text when `prompt_embeds is None`.
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/flux/pipeline_flux.py#L358-L388
Suggested fix:
Implement the Flux-style prompt-embed branch, decouple image preprocessing from required text prompts, and add a `latents=None` parameter to `prepare_latents()` that returns supplied latents after dtype/device conversion.
## Issue 3: `resolution` is not serialized
Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/visualcloze/pipeline_visualcloze_generation.py#L157-L190
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/visualcloze/pipeline_visualcloze_combined.py#L127-L168
Problem:
`resolution` controls preprocessing, but neither pipeline registers it in config. The fast tests work around this by manually passing `resolution=32` after reload.
Impact:
Saved 512-resolution or tiny-test pipelines reload with the default 384 preprocessing resolution, changing behavior and potentially increasing memory use.
Reproduction:
```python
import json, tempfile
from diffusers import VisualClozeGenerationPipeline
pipe = VisualClozeGenerationPipeline(None, None, None, None, None, None, None, resolution=32)
with tempfile.TemporaryDirectory(ignore_cleanup_errors=True) as d:
pipe.save_pretrained(d)
print(json.load(open(f"{d}/model_index.json")).get("resolution"))
```
Relevant precedent:
Scalar pipeline config values are registered with `register_to_config`.
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/flux2/pipeline_flux2_klein.py#L198
Suggested fix:
```python
self.register_to_config(resolution=resolution)
self.resolution = resolution
```
## Issue 4: Layout prompt is a tuple, not a string
Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/visualcloze/visualcloze_utils.py#L183-L187
Problem:
A trailing comma makes `layout_instruction` a one-element tuple. It is later interpolated into the text prompt as `("A grid layout ...",)`.
Impact:
The text encoder receives tuple punctuation instead of the intended layout instruction string.
Reproduction:
```python
from diffusers.pipelines.visualcloze.visualcloze_utils import VisualClozeProcessor
processor = VisualClozeProcessor(resolution=64)
layout_prompt = processor.get_layout_prompt((2, 3))
print(type(layout_prompt).__name__, layout_prompt)
```
Relevant precedent:
Normal prompt assembly expects plain strings.
Suggested fix:
```python
layout_instruction = (
f"A grid layout with {size[0]} rows and {size[1]} columns, "
f"displaying {size[0] * size[1]} images arranged side by side."
)
return layout_instruction
```
## Issue 5: Multi-target preprocessing swaps resize width and height
Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/visualcloze/visualcloze_utils.py#L103-L112
Problem:
The multi-target branch computes `new_w` and `new_h`, then calls `_resize_and_crop(image, new_h, new_w)`. `_resize_and_crop` expects `(width, height)`.
Impact:
For non-square inputs with more than one target, target crops are transposed to the wrong aspect/size. PR #12121 touched this block for a previous AttributeError but did not fix this width/height swap.
Reproduction:
```python
from PIL import Image
from diffusers.pipelines.visualcloze.visualcloze_utils import VisualClozeProcessor
p = VisualClozeProcessor(resolution=64)
imgs = [
[Image.new("RGB", (128, 64)) for _ in range(3)],
[None, None, Image.new("RGB", (128, 64))],
]
_, sizes, pos = p.preprocess_image(imgs, vae_scale_factor=8)
print(sizes, pos)
```
Relevant precedent:
`VaeImageProcessor._resize_and_crop(image, width, height)` defines the required order.
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/image_processor.py#L429-L459
Suggested fix:
```python
processed_images[i][j] = self._resize_and_crop(processed_images[i][j], new_w, new_h)
```
## Issue 6: Combined pipeline returns the wrong tuple when upsampling is disabled
Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/visualcloze/pipeline_visualcloze_combined.py#L360-L382
Problem:
For `upsampling_strength == 0` and `return_dict=False`, the method returns `(generation_output,)` instead of `(generation_output.images,)`.
Impact:
Tuple-output users receive a nested `FluxPipelineOutput`, unlike other pipelines and unlike the method docstring.
Reproduction:
```python
from diffusers import VisualClozePipeline
from diffusers.pipelines.flux.pipeline_output import FluxPipelineOutput
class FakeGenerationPipe:
def __call__(self, **kwargs):
return FluxPipelineOutput(images=[["generated"]])
pipe = object.__new__(VisualClozePipeline)
pipe.generation_pipe = FakeGenerationPipe()
out = VisualClozePipeline.__call__(pipe, "task", "content", [[None]],
upsampling_strength=0, return_dict=False)
print(type(out[0]).__name__)
```
Relevant precedent:
Pipeline tuple returns normally expose the payload field directly.
Suggested fix:
```python
if upsampling_strength == 0:
if not return_dict:
return (generation_output.images,)
return generation_output
```
## Issue 7: No slow tests exist for VisualCloze
Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/visualcloze/test_pipeline_visualcloze_generation.py#L32-L317
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/visualcloze/test_pipeline_visualcloze_combined.py#L27-L352
Problem:
Only fast tests are present; there is no `@slow` coverage for the published checkpoints.
Impact:
Checkpoint-specific behavior is untested, including real 384/512 resolution handling, default generator behavior, multi-target tasks, and the two-stage combined pipeline.
Reproduction:
```python
from pathlib import Path
text = "\n".join(p.read_text(encoding="utf-8") for p in Path("tests/pipelines/visualcloze").glob("test_*.py"))
print("@slow" in text)
```
Relevant precedent:
Flux has slow pipeline tests for real checkpoints.
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/flux/test_pipeline_flux.py#L301-L325
Suggested fix:
Add slow tests for `VisualClozeGenerationPipeline` and `VisualClozePipeline` using `VisualCloze/VisualClozePipeline-384`, covering `generator=None`, `upsampling_strength=0`, `upsampling_strength>0`, multi-target inputs, and save/load resolution preservation.
## Issue 8: Docs link the 512 checkpoint to the 384 repo
Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/docs/source/en/api/pipelines/visualcloze.md#L34-L36
Problem:
The `VisualClozePipeline-512` link points to `VisualClozePipeline-384`.
Impact:
Users trying to load the 512-resolution checkpoint are sent to the wrong model page.
Reproduction:
```python
from pathlib import Path
for line in Path("docs/source/en/api/pipelines/visualcloze.md").read_text(encoding="utf-8").splitlines():
if "VisualClozePipeline-512" in line:
print(line)
```
Relevant precedent:
N/A.
Suggested fix:
```md
[VisualClozePipeline-512](https://huggingface.co/VisualCloze/VisualClozePipeline-512)
```
Contributor guide
Research direction
Start with the affected VisualCloze pipeline files in src/diffusers/pipelines/visualcloze and the utility and documentation paths named in the issue. Run the visualcloze tests first, noting the reported torch collection failure, then separate the generator, embeddings, serialization, preprocessing, return-value, slow-test, and link checks. Done means each reported behavior has a focused test or documentation correction.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, pytorch
- Domain
- documentation, machine-learning, testing-qa
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 45/100