huggingface / huggingface/diffusers

skyreels_v2 model/pipeline review

Aperta
#13,609 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

# `skyreels_v2` model/pipeline review

Commit tested: `0f1abc4ae8b0eb2a3b40e82a310507281144c423`

Review performed against the repository review rules. Reviewed public imports/lazy loading, transformer config/attention/runtime paths, all SkyReels-V2 pipelines, docs, examples, and tests. Duplicate searches found an existing duplicate only for the optional `ftfy` issue: https://github.com/huggingface/diffusers/issues/13112 and PR https://github.com/huggingface/diffusers/pull/13113.

Top-level imports passed in `.venv`. Full pytest collection could not run because this `.venv` torch build lacks `torch._C._distributed_c10d`, which the shared test mixins import through FSDP.

## Issue 1: Diffusion-forcing schedules drop tail latent frames

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing.py#L477-L480
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing.py#L563-L569
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/models/transformers/transformer_skyreels_v2.py#L657-L671

Problem:
`generate_timestep_matrix()` floors `num_latent_frames // causal_block_size` and later expands back to full frames. If the latent frame count is not divisible by the block size, the remainder is omitted. The documented 720P setting `base_num_frames=121` gives 31 latent frames with temporal scale 4, and `causal_block_size=5` covers only 30.

Impact:
The final latent frame can remain at initial noise or be skipped by the update schedule. Direct transformer calls with `num_frame_per_block > 1` also build masks assuming exact divisibility.

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

num_latent_frames = (121 - 1) // 4 + 1
step_matrix, _, update_mask, intervals = SkyReelsV2DiffusionForcingPipeline.generate_timestep_matrix(
None, num_latent_frames, torch.arange(4), num_latent_frames, ar_step=0, causal_block_size=5
)
print(num_latent_frames, step_matrix.shape[1], update_mask.shape[1], intervals[-1])
# 31 30 30 (0, 30)
```

Relevant precedent:
The docs recommend the 121-frame 720P setting with causal block sizing here:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/docs/source/en/api/pipelines/skyreels_v2.md#L193-L197

Suggested fix:
Use ceil block counts and crop expanded tensors back to `num_latent_frames`. In the model mask, derive block IDs per actual frame instead of repeat-interleaving a floored block count:
```python
frame_ids = torch.arange(post_patch_num_frames, device=hidden_states.device)
block_ids = torch.div(frame_ids, self.config.num_frame_per_block, rounding_mode="floor")
causal_mask = block_ids.unsqueeze(0) <= block_ids.unsqueeze(1)
```

## Issue 2: Overlap noise uses global indices on local windows

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing.py#L875-L885
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing_i2v.py#L954-L964
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing_v2v.py#L962-L972

Problem:
`latent_model_input` is already sliced to `valid_interval_start:valid_interval_end`, but the overlap-noise branch slices it again with global indices. It also uses `torch.randn_like()` directly, bypassing the user-provided `generator`.

Impact:
Long-video overlap conditioning is noised on the wrong local frames, and `addnoise_condition > 0` is not reproducible from the pipeline generator.

Reproduction:
```python
import torch

prefix_video_latents_frames = 10
valid_interval_start = 5
valid_interval_end = 15
latent_model_input = torch.arange(valid_interval_start, valid_interval_end).view(1, 1, -1, 1, 1).float()

current = latent_model_input.clone()
current[:, :, valid_interval_start:prefix_video_latents_frames] = -1

expected = latent_model_input.clone()
expected[:, :, : prefix_video_latents_frames - valid_interval_start] = -1

print(current.flatten().tolist())
print(expected.flatten().tolist())
```

Relevant precedent:
Other diffusers latent preparation uses `randn_tensor(..., generator=generator)` for all user-visible randomness.

Suggested fix:
```python
local_prefix_end = min(prefix_video_latents_frames, valid_interval_end) - valid_interval_start
if addnoise_condition > 0 and local_prefix_end > 0:
noise_factor = 0.001 * addnoise_condition
prefix = latent_model_input[:, :, :local_prefix_end]
noise = randn_tensor(prefix.shape, generator=generator, device=prefix.device, dtype=prefix.dtype)
latent_model_input[:, :, :local_prefix_end] = prefix * (1.0 - noise_factor) + noise * noise_factor
timestep[:, :local_prefix_end] = addnoise_condition
```

## Issue 3: `causal_block_size` mutates serialized transformer config

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing.py#L781-L784
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/models/transformers/transformer_skyreels_v2.py#L765-L766

Problem:
A per-call pipeline option calls `self.transformer._set_ar_attention(causal_block_size)`, and `_set_ar_attention()` writes into the model config via `register_to_config`.

Impact:
One async call changes later calls that omit `causal_block_size`, and `save_pretrained()` will persist that call-time setting.

Reproduction:
```python
from diffusers import SkyReelsV2Transformer3DModel

m = SkyReelsV2Transformer3DModel(
patch_size=(1, 2, 2), num_attention_heads=2, attention_head_dim=12,
in_channels=4, out_channels=4, text_dim=16, freq_dim=256,
ffn_dim=32, num_layers=1, rope_max_seq_len=32,
)
print(m.config.num_frame_per_block)
m._set_ar_attention(5)
print(m.config.num_frame_per_block)
```

Relevant precedent:
Pipeline call arguments should not silently rewrite loadable model config.

Suggested fix:
Pass the block size as a runtime transformer forward argument, or temporarily restore the original config in a `try/finally` around the denoising loop.

## Issue 4: I2V input validation rejects valid image batches and accepts unusable embeds

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_i2v.py#L340-L341
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing_i2v.py#L319-L329
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing_i2v.py#L872-L872

Problem:
The I2V pipelines document `PipelineImageInput`, but reject a list of PIL images. The diffusion-forcing I2V pipeline also accepts `image_embeds` without `image`, then unconditionally preprocesses `image`.

Impact:
Documented batched image inputs fail early, while `image_embeds`-only calls pass validation and fail later.

Reproduction:
```python
import torch
from PIL import Image
from diffusers import SkyReelsV2ImageToVideoPipeline, SkyReelsV2DiffusionForcingImageToVideoPipeline

pipe = SkyReelsV2ImageToVideoPipeline.__new__(SkyReelsV2ImageToVideoPipeline)
pipe._callback_tensor_inputs = ["latents", "prompt_embeds", "negative_prompt_embeds"]
try:
pipe.check_inputs("a", None, [Image.new("RGB", (16, 16))], 16, 16, callback_on_step_end_tensor_inputs=["latents"])
except Exception as e:
print(type(e).__name__, e)

df_pipe = SkyReelsV2DiffusionForcingImageToVideoPipeline.__new__(SkyReelsV2DiffusionForcingImageToVideoPipeline)
df_pipe._callback_tensor_inputs = pipe._callback_tensor_inputs
df_pipe.check_inputs("a", None, None, 16, 16, image_embeds=torch.zeros(1, 1, 1), num_frames=9, base_num_frames=97)
print("image_embeds-only accepted")
```

Relevant precedent:
`PipelineImageInput` normally includes PIL images, tensors, and lists of them.

Suggested fix:
Allow list inputs in validation, and either remove `image_embeds` from the diffusion-forcing I2V public API or implement a real precomputed-conditioning path.

## Issue 5: Optional `ftfy` is still called unguarded

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2.py#L90-L92
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/src/diffusers/pipelines/skyreels_v2/pipeline_skyreels_v2_diffusion_forcing.py#L97-L100

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

Impact:
Prompt encoding crashes in environments where optional `ftfy` is not installed. This is already tracked in https://github.com/huggingface/diffusers/issues/13112 and PR https://github.com/huggingface/diffusers/pull/13113.

Reproduction:
```python
import diffusers.pipelines.skyreels_v2.pipeline_skyreels_v2 as m

m.__dict__.pop("ftfy", None)
m.prompt_clean("hello")
```

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: Test coverage gaps hide SkyReels-V2 regressions

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/skyreels_v2/test_skyreels_v2_df_image_to_video.py#L37-L146
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/skyreels_v2/test_skyreels_v2.py#L31-L109
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/tests/pipelines/skyreels_v2/test_skyreels_v2_df_video_to_video.py#L38-L118

Problem:
`test_skyreels_v2_df_image_to_video.py` defines `SkyReelsV2DiffusionForcingImageToVideoPipelineFastTests` twice, so unittest discovery only exposes the second class name. The target family also has no `@slow` tests, and the fast tests mostly assert shapes with `max_diff <= 1e10`.

Impact:
The image-only DF I2V fixture is shadowed, official checkpoint behavior is untested, and regressions in output quality/scheduling can pass.

Reproduction:
```python
from pathlib import Path
import ast

files = list(Path("tests/pipelines/skyreels_v2").glob("test_*.py"))
print({p.name: "@slow" in p.read_text() for p in files})

p = Path("tests/pipelines/skyreels_v2/test_skyreels_v2_df_image_to_video.py")
classes = [n.name for n in ast.parse(p.read_text()).body if isinstance(n, ast.ClassDef)]
print(classes)
```

Relevant precedent:
Slow pipeline tests usually load a real small/official checkpoint path and assert deterministic output slices, not only output shapes.

Suggested fix:
Rename the second DF I2V test class, add slow tests for each public pipeline variant, and replace `1e10` thresholds with deterministic tensor slices or small visual/numeric checks.

## Issue 7: Docs reference a non-existent 1.3B 720P DF checkpoint

Affected code:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/docs/source/en/api/pipelines/skyreels_v2.md#L221-L224
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/docs/source/en/api/pipelines/skyreels_v2.md#L284-L287

Problem:
The FLF2V and V2V examples use `Skywork/SkyReels-V2-DF-1.3B-720P-Diffusers`, but that repo does not exist. The supported list names `Skywork/SkyReels-V2-DF-1.3B-540P-Diffusers` and `Skywork/SkyReels-V2-DF-14B-720P-Diffusers`.

Impact:
Users copying the docs get a 404 before reaching pipeline execution.

Reproduction:
```python
from huggingface_hub import HfApi

api = HfApi()
api.model_info("Skywork/SkyReels-V2-DF-1.3B-720P-Diffusers")
```

Relevant precedent:
https://github.com/huggingface/diffusers/blob/0f1abc4ae8b0eb2a3b40e82a310507281144c423/docs/source/en/api/pipelines/skyreels_v2.md#L31-L39

Suggested fix:
Use `Skywork/SkyReels-V2-DF-14B-720P-Diffusers` for 720P examples, or change the example dimensions/base frames to the existing 1.3B 540P checkpoint.

Guida per i contributori

Apri la guida per i contributori

Direzione di ricerca

Start with the affected SkyReels-V2 pipeline and transformer files at commit 0f1abc4ae8b0eb2a3b40e82a310507281144c423, then run the supplied schedule, overlap-noise, config, validation, and optional-ftfy reproductions. Review the listed SkyReels-V2 tests and documentation examples for coverage and checkpoint references. Done means the reported regressions are covered by meaningful tests and the affected docs and inputs behave as documented.

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

Valutazione

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

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.