huggingface / huggingface/diffusers

ddpm model/pipeline review

Open
#13,649 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
34.5k
Forks
7.3k
Avg merge
3d 3h
Merged PRs (30d)
91

Description

# `ddpm` model/pipeline review

Commit tested: `0f1abc4ae8b0eb2a3b40e82a310507281144c423`

Review performed against the repository review rules.

Coverage status: fast DDPM tests exist, and a slow accelerator integration test exists. Local run: `2 passed, 1 skipped` for `tests/pipelines/ddpm/test_ddpm.py`; the skipped test is marked `@slow`.

## Issue 1: `DDPMPipeline` does not run latents on the offload execution device

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/ddpm/pipeline_ddpm.py#L111-L139

Problem:
`DDPMPipeline` declares `model_cpu_offload_seq = "unet"` but initializes `image` on `self.device`, not `self._execution_device`, and never calls `self.maybe_free_model_hooks()` before returning. Under `enable_model_cpu_offload()`, `self.device` remains CPU while the UNet forward is executed on the accelerator by accelerate hooks. That leaves the scheduler step operating on mismatched CPU/GPU tensors.

Duplicate search:
No duplicate found for `DDPMPipeline enable_model_cpu_offload`, `DDPMPipeline maybe_free_model_hooks`, or `DDPMPipeline _execution_device`.

Impact:
Users enabling CPU offload can hit device mismatch failures or leave the UNet resident on the accelerator after the call, defeating the offload contract.

Reproduction:
```python
import torch
from diffusers import DDPMPipeline, DDPMScheduler, UNet2DModel

if not torch.cuda.is_available():
raise SystemExit("CUDA/accelerator required")

unet = UNet2DModel(
block_out_channels=(4, 8),
layers_per_block=1,
norm_num_groups=4,
sample_size=8,
in_channels=3,
out_channels=3,
down_block_types=("DownBlock2D", "AttnDownBlock2D"),
up_block_types=("AttnUpBlock2D", "UpBlock2D"),
)
pipe = DDPMPipeline(unet=unet, scheduler=DDPMScheduler(num_train_timesteps=2))
pipe.set_progress_bar_config(disable=True)
pipe.enable_model_cpu_offload(device="cuda")

pipe(num_inference_steps=1, output_type="np")
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/ddim/pipeline_ddim.py#L150
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/pipeline_utils.py#L1284-L1294

Suggested fix:
```python
device = self._execution_device

if device.type == "mps":
image = randn_tensor(image_shape, generator=generator, dtype=self.unet.dtype)
image = image.to(device)
else:
image = randn_tensor(image_shape, generator=generator, device=device, dtype=self.unet.dtype)

# ...

if output_type == "pil":
image = self.numpy_to_pil(image)

self.maybe_free_model_hooks()
```

## Issue 2: Generator lists are not validated against `batch_size`

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/ddpm/pipeline_ddpm.py#L111-L116

Problem:
`DDPMPipeline.__call__` accepts `generator: torch.Generator | list[torch.Generator]`, but it does not validate list length. A one-element list for a larger batch is silently treated like a single generator, and other short lists can fail with a raw `IndexError`.

Duplicate search:
No duplicate found for `DDPMPipeline generator list` or `DDPMPipeline generator list length`.

Impact:
Per-sample seeding behaves inconsistently and invalid inputs do not produce the clear `ValueError` users get from related pipelines.

Reproduction:
```python
import torch
from diffusers import DDPMPipeline, DDPMScheduler, UNet2DModel

unet = UNet2DModel(
block_out_channels=(4, 8),
layers_per_block=1,
norm_num_groups=4,
sample_size=8,
in_channels=3,
out_channels=3,
down_block_types=("DownBlock2D", "AttnDownBlock2D"),
up_block_types=("AttnUpBlock2D", "UpBlock2D"),
)
pipe = DDPMPipeline(unet=unet, scheduler=DDPMScheduler(num_train_timesteps=2))
pipe.set_progress_bar_config(disable=True)

pipe(
batch_size=3,
generator=[torch.Generator(device="cpu").manual_seed(i) for i in range(2)],
num_inference_steps=1,
output_type="np",
)
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/ddim/pipeline_ddim.py#L144-L150
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/consistency_models/pipeline_consistency_models.py#L96-L100

Suggested fix:
```python
if isinstance(generator, list) and len(generator) != batch_size:
raise ValueError(
f"You have passed a list of generators of length {len(generator)}, but requested an effective batch"
f" size of {batch_size}. Make sure the batch size matches the length of the generators."
)
```

## Issue 3: Invalid `output_type` values silently return NumPy

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/ddpm/pipeline_ddpm.py#L131-L134

Problem:
Any `output_type` other than `"pil"` falls through to NumPy output. This includes typos and common values like `"pt"`, with no warning or error.

Duplicate search:
No duplicate found for `DDPMPipeline output_type` or `DDPMPipeline invalid output_type`.

Impact:
A misspelled output type can silently change behavior, making downstream code receive `np.ndarray` when the caller expected a different format or a validation error.

Reproduction:
```python
from diffusers import DDPMPipeline, DDPMScheduler, UNet2DModel

unet = UNet2DModel(
block_out_channels=(4, 8),
layers_per_block=1,
norm_num_groups=4,
sample_size=8,
in_channels=3,
out_channels=3,
down_block_types=("DownBlock2D", "AttnDownBlock2D"),
up_block_types=("AttnUpBlock2D", "UpBlock2D"),
)
pipe = DDPMPipeline(unet=unet, scheduler=DDPMScheduler(num_train_timesteps=2))
pipe.set_progress_bar_config(disable=True)

images = pipe(num_inference_steps=1, output_type="definitely-not-valid").images
print(type(images).__name__) # ndarray
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/consistency_models/pipeline_consistency_models.py#L111-L124
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/image_processor.py#L764-L779

Suggested fix:
```python
if output_type not in ["pt", "np", "pil"]:
raise ValueError("output_type must be one of ['pt', 'np', or 'pil'].")

image = (image / 2 + 0.5).clamp(0, 1)
if output_type == "pt":
pass
else:
image = image.cpu().permute(0, 2, 3, 1).numpy()
if output_type == "pil":
image = self.numpy_to_pil(image)
```

Contributor guide

Open the contributing guide

Research direction

Start with src/diffusers/pipelines/ddpm/pipeline_ddpm.py and compare the referenced DDIM and pipeline utility implementations. Run tests/pipelines/ddpm/test_ddpm.py, including the slow accelerator case, then add coverage for offload execution, generator-list length validation, and invalid output types. Done means the regressions are handled with clear validation and existing behavior remains covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, pytorch
Domain
machine-learning
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.