huggingface / huggingface/diffusers

visualcloze model/pipeline review

Đang mở
#13,623 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Python
Star
34.5k
Fork
7.3k
Merge trung bình
3 ngày 3 giờ
Pull request đã merge (30 ngày)
91

Mô tả

# `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)
```

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Bắt đầu với các tệp pipeline VisualCloze bị ảnh hưởng trong src/diffusers/pipelines/visualcloze và các đường dẫn tiện ích cũng như tài liệu được nêu trong issue. Trước tiên, chạy các bài kiểm thử visualcloze, ghi nhận lỗi collection của torch đã được báo cáo, sau đó tách riêng các kiểm tra về generator, embeddings, serialization, preprocessing, giá trị trả về, slow-test và liên kết. Được xem là hoàn tất khi mỗi hành vi được báo cáo đều có một bài kiểm thử tập trung hoặc một chỉnh sửa tài liệu.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
python, pytorch
Lĩnh vực
documentation, machine-learning, testing-qa
Loại issue
Lỗi
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
45/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.