huggingface / huggingface/diffusers
ddpm model/pipeline review
- 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
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