huggingface / huggingface/diffusers

chronoedit model/pipeline review

Aperta
#13,620 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
Python
Stelle
34.5k
Fork
7.3k
Merge medio
3g 3h
PR unite (30g)
91

Descrizione

# `chronoedit` model/pipeline review

Commit tested: `0f1abc4ae8b0eb2a3b40e82a310507281144c423`

Review performed against the repository review rules.

Duplicate search: checked GitHub Issues/PRs for `chronoedit`, `ChronoEditPipeline`, `ChronoEditTransformer3DModel`, `pipeline_chronoedit`, `transformer_chronoedit`, temporal reasoning + `model_outputs`, `num_videos_per_prompt` + `image_embeds`, and slow-test coverage. Existing related items found: https://github.com/huggingface/diffusers/issues/12661, https://github.com/huggingface/diffusers/pull/12660, https://github.com/huggingface/diffusers/pull/12679, https://github.com/huggingface/diffusers/pull/13347. None duplicate Issues 1-6 below; PR #13347 is related to Issue 7's missing model-test coverage.

## Issue 1: Temporal reasoning crashes with the default scheduler

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py#L666-L675

Problem:
`enable_temporal_reasoning=True` unconditionally accesses `self.scheduler.model_outputs` and `self.scheduler.last_sample`. The pipeline imports/types/tests `FlowMatchEulerDiscreteScheduler`, which does not define those UniPC-style fields.

Impact:
The documented temporal reasoning path can fail before completing the first denoising step when used with the scheduler family the pipeline itself advertises.

Reproduction:
```python
from diffusers import FlowMatchEulerDiscreteScheduler

scheduler = FlowMatchEulerDiscreteScheduler(shift=7.0)
print(hasattr(scheduler, "model_outputs")) # False

# Same field accessed by ChronoEditPipeline when temporal reasoning truncates latents.
len(scheduler.model_outputs)
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/schedulers/scheduling_unipc_multistep.py#L279-L288

Suggested fix:
```python
if hasattr(self.scheduler, "model_outputs"):
for j, model_output in enumerate(self.scheduler.model_outputs):
if model_output is not None and latents.shape[-3] != model_output.shape[-3]:
self.scheduler.model_outputs[j] = model_output[:, :, [0, -1]]
if getattr(self.scheduler, "last_sample", None) is not None:
self.scheduler.last_sample = self.scheduler.last_sample[:, :, [0, -1]]
```

## Issue 2: `num_videos_per_prompt > 1` leaves image embeddings under-batched

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py#L632-L635

Problem:
Prompt embeddings are expanded to `batch_size * num_videos_per_prompt`, but image embeddings are repeated only by `batch_size`. With `num_videos_per_prompt=2`, the transformer receives prompt batch 2 and image batch 1.

Impact:
Multi-video generation per prompt crashes or conditions samples with mismatched image context.

Reproduction:
```python
import torch
from diffusers import ChronoEditTransformer3DModel

m = ChronoEditTransformer3DModel(
patch_size=(1, 2, 2), num_attention_heads=2, attention_head_dim=12,
in_channels=36, out_channels=16, text_dim=32, ffn_dim=32,
num_layers=1, image_dim=4, rope_max_seq_len=32,
)
hidden = torch.randn(2, 36, 1, 2, 2)
text = torch.randn(2, 16, 32)
image = torch.randn(1, 257, 4) # current pipeline batch after repeat(batch_size=1)
m(hidden, torch.tensor([1, 1]), text, image)
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/wan/pipeline_wan_animate.py#L975-L979

Suggested fix:
```python
if image_embeds.shape[0] == 1:
image_embeds = image_embeds.repeat(batch_size * num_videos_per_prompt, 1, 1)
else:
image_embeds = image_embeds.repeat_interleave(num_videos_per_prompt, dim=0)
```

## Issue 3: `image_embeds` is accepted without `image`, but `image` is still required

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py#L333-L343
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py#L641-L655

Problem:
`check_inputs` allows `image=None` when `image_embeds` is provided, but `__call__` always preprocesses `image` and uses it to build VAE conditioning latents.

Impact:
Users trying to skip only the CLIP image encoder get a later, less actionable processor error. The API contract is misleading because `image_embeds` alone is insufficient.

Reproduction:
```python
import torch
from diffusers import ChronoEditPipeline

pipe = object.__new__(ChronoEditPipeline)
pipe._callback_tensor_inputs = ["latents", "prompt_embeds", "negative_prompt_embeds"]

ChronoEditPipeline.check_inputs(
pipe, prompt="x", negative_prompt=None, image=None,
image_embeds=torch.zeros(1, 257, 1280), height=16, width=16,
)
print("validation accepted image_embeds without image")
```

Relevant precedent:
`image_embeds` may skip CLIP encoding, but VAE conditioning still needs pixels.

Suggested fix:
```python
if image is None:
raise ValueError("`image` is required for VAE conditioning; `image_embeds` only skips CLIP image encoding.")
```

## Issue 4: `num_frames` is silently ignored unless temporal reasoning is enabled

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py#L590-L597

Problem:
When `enable_temporal_reasoning=False`, `num_frames` is overwritten to `5` before validation and latent preparation.

Impact:
The default `num_frames=81` and user-provided values do not describe the actual output shape in the default path.

Reproduction:
```python
num_frames = 9
enable_temporal_reasoning = False
num_frames = 5 if not enable_temporal_reasoning else num_frames
print(num_frames) # 5, not 9
```

Relevant precedent:
The `num_frames` docstring says it controls generated video length:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py#L511-L512

Suggested fix:
Either honor `num_frames` in both modes, or remove it from the non-temporal API path and raise if users pass a non-5 value while temporal reasoning is disabled.

## Issue 5: Optional `ftfy` guard is incomplete

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py#L44-L45
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py#L97-L100

Problem:
`ftfy` is imported only when available, but `basic_clean()` always calls `ftfy.fix_text`.

Impact:
A normal diffusers install with torch/transformers but without the test extra `ftfy` can import the pipeline and then fail during prompt encoding.

Reproduction:
```python
import diffusers.pipelines.chronoedit.pipeline_chronoedit as chrono

old_ftfy = getattr(chrono, "ftfy", None)
if hasattr(chrono, "ftfy"):
del chrono.ftfy
try:
chrono.basic_clean("hello & world")
finally:
if old_ftfy is not None:
chrono.ftfy = old_ftfy
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/wan/pipeline_wan.py#L78-L82

Suggested fix:
```python
def basic_clean(text):
if is_ftfy_available():
text = ftfy.fix_text(text)
text = html.unescape(html.unescape(text))
return text.strip()
```

## Issue 6: RoPE precompute still requests float64

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/models/transformers/transformer_chronoedit.py#L377-L392

Problem:
`ChronoEditRotaryPosEmbed` selects `torch.float64` on non-MPS systems. The helper later casts real cos/sin buffers to float32, so the float64 work is unnecessary and violates the review rule against unconditional float64 in models.

Impact:
This adds avoidable construction-time dtype work and keeps an NPU/MPS portability footgun in new model code.

Reproduction:
```python
import inspect
from diffusers.models.transformers import transformer_chronoedit

src = inspect.getsource(transformer_chronoedit.ChronoEditRotaryPosEmbed.__init__)
print("torch.float64" in src)
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/models/embeddings.py#L1170-L1178

Suggested fix:
```python
freqs_dtype = torch.float32
```

## Issue 7: Slow tests and dedicated model tests are missing

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/chronoedit/test_chronoedit.py#L43-L60
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/chronoedit/test_chronoedit.py#L166-L177

Problem:
The target has one fast pipeline test file, no `@slow` ChronoEdit integration test, and no checked-in `tests/models/transformers/test_models_transformer_chronoedit.py` at this commit. Several common pipeline tests are skipped, including batch-identical and fp16 save/load.

Impact:
The real checkpoint path, temporal reasoning, LoRA examples, model save/load behavior, attention processors, gradient checkpointing, and batch/image edge cases above are not covered.

Reproduction:
```python
from pathlib import Path

files = [str(p) for p in Path("tests").rglob("*chronoedit*.py")]
print(files)
print(any("@slow" in Path(p).read_text(encoding="utf-8") for p in files))
print(Path("tests/models/transformers/test_models_transformer_chronoedit.py").exists())
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/wan/test_wan.py#L185-L187
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/models/transformers/test_models_transformer_wan.py#L106-L132

Suggested fix:
Add a ChronoEdit transformer model test file following Wan's model mixins, and add at least one `@slow` pipeline integration test for `nvidia/ChronoEdit-14B-Diffusers`, including temporal reasoning and the documented LoRA/scheduler path.

Guida per i contributori

Apri la guida per i contributori

Direzione di ricerca

Start with src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py and src/diffusers/models/transformers/transformer_chronoedit.py, then run the targeted checks in tests/pipelines/chronoedit/test_chronoedit.py; compare the cited Wan and UniPC implementations. Done means the listed reproduction paths no longer fail and the missing ChronoEdit model and slow integration coverage is added under the cited tests.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
python, pytorch
Ambito
machine-learning, testing-qa
Tipo di issue
Bug
Difficoltà
4/5
Tempo stimato
Più di una settimana
Stato di attività
Tranquilla
Chiarezza
Abbastanza chiara
Idoneità per principianti
45/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.