huggingface / huggingface/diffusers
chronoedit model/pipeline review
- Lenguaje dominante
- Python
- Estrellas
- 34.5k
- Forks
- 7.3k
- Merge medio
- 3 d 3 h
- PR fusionados (30 d)
- 91
Descripción
# `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.
Guía de contribución
Línea de trabajo
Comienza con src/diffusers/pipelines/chronoedit/pipeline_chronoedit.py y src/diffusers/models/transformers/transformer_chronoedit.py, luego ejecuta las comprobaciones específicas en tests/pipelines/chronoedit/test_chronoedit.py; compara las implementaciones de Wan y UniPC citadas. Se considera terminado cuando las rutas de reproducción indicadas ya no fallen y se añadan el modelo ChronoEdit que falta y la cobertura de integración lenta bajo las pruebas citadas.
Escrito por el modelo de indexación a partir del texto del issue.
Evaluación
- Stack tecnológico
- python, pytorch
- Área
- machine-learning, testing-qa
- Tipo de issue
- Error
- Dificultad
- 4/5
- Tiempo estimado
- Más de una semana
- Estado de actividad
- Tranquilo
- Claridad
- Bastante claro
- Aptitud para principiantes
- 45/100