diff --git a/.claude/skills/add-diffusion-model/SKILL.md b/.claude/skills/add-diffusion-model/SKILL.md new file mode 100644 index 000000000..353422e6e --- /dev/null +++ b/.claude/skills/add-diffusion-model/SKILL.md @@ -0,0 +1,308 @@ +--- +name: add-diffusion-model +description: Add a new diffusion model (text-to-image, text-to-video, image-to-video, text-to-audio, image editing) to vLLM-Omni. Use when integrating a new diffusion model, porting a diffusers pipeline or a custom model repo to vllm-omni, creating a new DiT transformer adapter, or adding diffusion model support. +--- + +# Adding a Diffusion Model to vLLM-Omni + +## Overview + +This skill guides you through adding a new diffusion model to vLLM-Omni. The model may come from HuggingFace Diffusers (structured pipeline) or from a private/custom repo. The workflow differs significantly depending on the source. + +## Prerequisites + +Before starting, determine: + +1. **Model category**: Text-to-Image, Text-to-Video, Image-to-Video, Image Editing, Text-to-Audio, or Omni +2. **Reference source**: Diffusers pipeline, custom repo, or a combination +3. **Model HuggingFace ID** or local checkpoint path +4. **Architecture**: Scheduler, text encoder, VAE, transformer/backbone + +## Step 0: Classify the Migration Path + +Check the model's HF repo for `model_index.json`. This determines your path: + +| Scenario | How to identify | Migration path | +|----------|----------------|----------------| +| **Already supported** | `_class_name` in `model_index.json` matches a key in `_DIFFUSION_MODELS` in `registry.py` | Skip to Step 5 (test) and Step 7 (docs) | +| **Diffusers-based** | Has standard `model_index.json` with `_diffusers_version`, subfolders for `transformer/`, `vae/`, etc. | Follow **Path A** below | +| **Custom/private repo** | No diffusers `model_index.json`, weights in non-standard format, custom model code in a separate git repo | Follow **Path B** below | +| **Hybrid** | Has some diffusers components (VAE) but custom transformer/fusion | Mix of Path A and Path B | + +## Path A: Diffusers-Based Model + +For models with a standard diffusers layout. See [references/transformer-adaptation.md](references/transformer-adaptation.md) for detailed code patterns. + +### A1. Analyze `model_index.json` + +Identify components: `transformer`, `scheduler`, `vae`, `text_encoder`, `tokenizer`. + +### A2. Create model directory + +``` +vllm_omni/diffusion/models/your_model_name/ +├── __init__.py +├── pipeline_your_model.py +└── your_model_transformer.py +``` + +### A3. Adapt transformer + +1. Copy from diffusers source. Remove mixins (`ModelMixin`, `ConfigMixin`, `AttentionModuleMixin`). +2. Replace attention with `vllm_omni.diffusion.attention.layer.Attention` (QKV shape: `[B, seq, heads, head_dim]`). +3. Add `od_config: OmniDiffusionConfig | None = None` to `__init__`. +4. Add `load_weights()` method mapping diffusers weight names to vllm-omni names. +5. Add class attributes: `_repeated_blocks`, `_layerwise_offload_blocks_attr`. + +### A4. Adapt pipeline + +Inherit from `nn.Module`. The key contract: + +```python +class YourPipeline(nn.Module): + def __init__(self, *, od_config: OmniDiffusionConfig, prefix: str = ""): + # Load VAE, text encoder, tokenizer via from_pretrained() + # Instantiate transformer (weights loaded later via weights_sources) + self.weights_sources = [ + DiffusersPipelineLoader.ComponentSource( + model_or_path=od_config.model, subfolder="transformer", + prefix="transformer.", fall_back_to_pt=True)] + + def forward(self, req: OmniDiffusionRequest) -> DiffusionOutput: + # Encode prompt → prepare latents → denoise loop → VAE decode + return DiffusionOutput(output=output) + + def load_weights(self, weights): + return AutoWeightsLoader(self).load_weights(weights) +``` + +Add post/pre-process functions in the same pipeline file. Register them in `registry.py`. + +### A5. Register, test, docs → continue at Step 4 below. + +--- + +## Path B: Custom/Private Repo Model + +For models without a diffusers pipeline — weights in custom format, model code in a private repo. Real examples: DreamID-Omni, BAGEL, HunyuanImage3. + +### B1. Understand the reference repo + +Study the original model's code to identify: +- **Model architecture files** (transformers, fusion modules, embeddings) +- **Weight format** (safetensors, `.pth`, custom checkpoint structure) +- **Weight loading helpers** (custom init functions, checkpoint loaders) +- **Pre/post-processing** (image/audio transforms, tokenization, VAE encode/decode) +- **External dependencies** (packages not on PyPI) +- **Config format** (JSON config files, hardcoded dicts) + +### B2. Decide what lives WHERE + +This is the key design decision for custom models. Follow these placement rules: + +| Code type | Where to place | Example | +|-----------|---------------|---------| +| **Pipeline orchestration** (init, forward, denoise loop) | `vllm_omni/diffusion/models//pipeline_.py` | Always required | +| **Custom transformer/backbone** (ported and adapted to vllm-omni) | `vllm_omni/diffusion/models//_transformer.py` or similar | `wan2_2.py`, `fusion.py`, `bagel_transformer.py` | +| **Custom sub-models** (VAE, fusion, autoencoder) | `vllm_omni/diffusion/models//` as separate files | `autoencoder.py`, `fusion.py` | +| **External dependency code** (original repo utilities) | **External repo**, installed via download script or pip | `dreamid_omni` package via git clone | +| **Hardcoded model configs** | Module-level dicts in pipeline file | `VIDEO_CONFIG`, `AUDIO_CONFIG` dicts | +| **Download/setup script** | `examples/offline_inference//download_.py` | `download_dreamid_omni.py` | +| **Custom `model_index.json`** | Generated by download script, placed at model root | Minimal: `{"_class_name": "YourPipeline", ...}` | + +### B3. Handle external dependencies + +If the model's code lives in a separate git repo: + +**Option 1: Import with graceful fallback** (recommended for models with external utils) + +```python +try: + from external_model.utils import init_vae, load_checkpoint +except ImportError: + raise ImportError( + "Failed to import from dependency 'external_model'. " + "Please run the download script first." + ) +``` + +**Option 2: Port the code directly** (preferred when feasible) + +Copy the essential model files into `vllm_omni/diffusion/models//` and adapt them. This avoids external dependencies. BAGEL does this — `autoencoder.py` and `bagel_transformer.py` are ported directly. + +**Decision criteria**: Port if the code is self-contained and won't diverge. Use external deps if the model repo is actively maintained and the code is complex. + +### B4. Handle custom weight loading + +Custom models have two patterns for weight loading: + +**Pattern 1: Bypass standard loader** (DreamID-Omni style) + +When the original model has complex custom init functions that load weights in `__init__`: + +```python +class CustomPipeline(nn.Module): + def __init__(self, *, od_config, prefix=""): + super().__init__() + model = od_config.model + # Load everything eagerly in __init__ using custom helpers + self.vae = custom_init_vae(model, device=self.device) + self.text_encoder = custom_init_text_encoder(model, device=self.device) + self.transformer = CustomFusionModel(CONFIG) + load_custom_checkpoint(self.transformer, + checkpoint_path=os.path.join(model, "model.safetensors")) + # NO weights_sources defined — bypasses standard loader + + def load_weights(self, weights): + pass # No-op — all weights loaded in __init__ +``` + +**Pattern 2: Use standard loader with custom `load_weights`** (BAGEL style) + +When weights are in safetensors format but need name remapping: + +```python +class CustomPipeline(nn.Module): + def __init__(self, *, od_config, prefix=""): + super().__init__() + # Instantiate model architecture without weights + self.bagel = BagelModel(config) + self.vae = AutoEncoder(ae_params) + + # Point loader at the safetensors in the model root + self.weights_sources = [ + DiffusersPipelineLoader.ComponentSource( + model_or_path=od_config.model, + subfolder=None, # weights at root, not in subfolder + prefix="", + fall_back_to_pt=False, + ) + ] + + def load_weights(self, weights): + # Custom name remapping for non-diffusers weight names + params = dict(self.named_parameters()) + loaded = set() + for name, tensor in weights: + # Remap original weight names to vllm-omni module names + name = self._remap_weight_name(name) + if name in params: + default_weight_loader(params[name], tensor) + loaded.add(name) + return loaded +``` + +### B5. Create the `model_index.json` + +Custom models need a `model_index.json` at the model root for vllm-omni to discover them. For custom models, this is minimal: + +```json +{ + "_class_name": "YourModelPipeline", + "custom_key": "path/to/custom_weights.safetensors" +} +``` + +The `_class_name` must match a key in `_DIFFUSION_MODELS` in `registry.py`. Additional keys are model-specific (accessed via `od_config.model_config`). + +If the model's weights come from multiple HF repos, write a **download script** that: +1. Downloads from each repo +2. Assembles into a single directory +3. Generates `model_index.json` +4. Installs any external dependencies (git clone + `.pth` file) + +Place at: `examples/offline_inference//download_.py` + +### B6. Handle multi-modal inputs + +If the model accepts images, audio, or other multi-modal inputs, implement the protocol classes from `vllm_omni/diffusion/models/interface.py`: + +```python +from vllm_omni.diffusion.models.interface import SupportImageInput, SupportAudioInput + +class MyPipeline(nn.Module, SupportImageInput, SupportAudioInput): + # Protocol markers — the engine uses these to enable proper input routing + pass +``` + +Preprocessing for custom models is typically done **inside `forward()`** rather than via registered pre-process functions, since the logic is often tightly coupled to the model. + +### B7. Continue at Step 4 below. + +--- + +## Common Steps (Both Paths) + +### Step 4: Register Model in registry.py + +Edit `vllm_omni/diffusion/registry.py`: + +```python +_DIFFUSION_MODELS = { + "YourModelPipeline": ("your_model_name", "pipeline_your_model", "YourModelPipeline"), +} +_DIFFUSION_POST_PROCESS_FUNCS = { + "YourModelPipeline": "get_your_model_post_process_func", # if applicable +} +_DIFFUSION_PRE_PROCESS_FUNCS = { + "YourModelPipeline": "get_your_model_pre_process_func", # if applicable +} +``` + +The registry key is the `_class_name` from `model_index.json`. The tuple is `(folder_name, module_file, class_name)`. + +Create `__init__.py` exporting the pipeline class and any factory functions. + +### Step 5: Run, Test, Debug + +Use the appropriate existing example script: + +| Category | Script | +|----------|--------| +| Text-to-Image | `examples/offline_inference/text_to_image/text_to_image.py` | +| Text-to-Video | `examples/offline_inference/text_to_video/text_to_video.py` | +| Image-to-Video | `examples/offline_inference/image_to_video/image_to_video.py` | +| Image-to-Image | `examples/offline_inference/image_to_image/image_edit.py` | +| Text-to-Audio | `examples/offline_inference/text_to_audio/text_to_audio.py` | + +For custom/Omni models that don't fit these categories, create a dedicated example script. + +**Validation**: No errors, output is meaningful, quality matches reference implementation. + +See [references/troubleshooting.md](references/troubleshooting.md) for common errors. + +### Step 6: Add Example Scripts + +For Omni or custom models, create: +- `examples/offline_inference/your_model_name/` — offline script + README +- `examples/online_serving/your_model_name/` — server script + client +- Download script if weights require assembly from multiple sources + +### Step 7: Update Documentation + +Required updates: +1. `docs/user_guide/diffusion/parallelism_acceleration.md` — parallelism support table +2. `docs/user_guide/diffusion/teacache.md` — if TeaCache supported +3. `docs/user_guide/diffusion/cache_dit_acceleration.md` — if Cache-DiT supported +4. `examples/offline_inference/xxx/README.md` — offline example docs +5. `examples/online_serve/xxx/README.md` — online serve docs + +### Step 8: Add E2E Tests (Recommended) + +Create `tests/e2e/online_serving/test_your_model_expansion.py`. + +## Iterative Development Tips + +1. **Start minimal**: Basic generation first, no parallelism/caching +2. **Use `--enforce-eager`**: Disable torch.compile during debugging +3. **Use small models**: Test with smaller variants first +4. **Check tensor shapes**: Most errors are reshape mismatches in attention +5. **Add parallelism incrementally**: TP → SP → CFG parallel +6. **For custom models**: Get the model running with the original code first, then progressively replace components with vllm-omni equivalents + +## Reference Files + +- [Transformer Adaptation](references/transformer-adaptation.md) — porting transformers from diffusers +- [Custom Model Patterns](references/custom-model-patterns.md) — patterns for non-diffusers models +- [Parallelism Patterns](references/parallelism-patterns.md) — TP, SP, CFG parallel +- [Troubleshooting](references/troubleshooting.md) — common errors and fixes diff --git a/.claude/skills/add-diffusion-model/references/custom-model-patterns.md b/.claude/skills/add-diffusion-model/references/custom-model-patterns.md new file mode 100644 index 000000000..2434e0b5d --- /dev/null +++ b/.claude/skills/add-diffusion-model/references/custom-model-patterns.md @@ -0,0 +1,273 @@ +# Custom Model Patterns Reference + +Patterns for adding models that don't come from the standard diffusers pipeline format. + +## Directory Structure Comparison + +### Diffusers-based model (e.g., Wan2.2) + +``` +vllm_omni/diffusion/models/wan2_2/ +├── __init__.py # Exports pipeline + transformer + helpers +├── pipeline_wan2_2.py # Pipeline: loads components via from_pretrained() +├── pipeline_wan2_2_i2v.py # Variant pipeline for image-to-video +└── wan2_2_transformer.py # Transformer: ported from diffusers, uses Attention layer +``` + +The transformer is loaded separately via `weights_sources` + `load_weights()`. Non-transformer components (VAE, text encoder) are loaded in `__init__` via `from_pretrained()`. + +### Custom model with external deps (e.g., DreamID-Omni) + +``` +vllm_omni/diffusion/models/dreamid_omni/ +├── __init__.py # Exports pipeline only +├── pipeline_dreamid_omni.py # Pipeline: loads ALL weights in __init__ via custom helpers +├── fusion.py # Custom fusion architecture (video + audio cross-attention) +└── wan2_2.py # Re-implemented Wan backbone with split API + +examples/offline_inference/x_to_video_audio/ +└── download_dreamid_omni.py # Downloads weights from 3 HF repos + clones code repo +``` + +All weights loaded eagerly in `__init__`. `load_weights()` is a no-op. External dependency (`dreamid_omni` package) imported with try/except. + +### Custom model with ported code (e.g., BAGEL) + +``` +vllm_omni/diffusion/models/bagel/ +├── __init__.py +├── pipeline_bagel.py # Pipeline: instantiates models, uses weights_sources +├── bagel_transformer.py # Full LLM backbone (Qwen2-MoT) ported into vllm-omni +└── autoencoder.py # Custom VAE ported from original repo +``` + +Model code is fully ported (no external dependency). Uses `weights_sources` and `load_weights()` with custom name remapping to handle non-diffusers safetensors format. + +## Weight Loading Patterns + +### Pattern 1: Standard diffusers flow (Wan2.2, Z-Image, FLUX) + +``` +init → create transformer (empty) → set weights_sources → [loader calls load_weights()] +``` + +- `weights_sources` points to safetensors in HF subfolder (e.g., `transformer/`) +- `load_weights()` receives `(name, tensor)` pairs from the loader +- Name remapping handles diffusers→vllm-omni differences (QKV fusion, Sequential index removal) + +### Pattern 2: Custom safetensors at root (BAGEL) + +``` +init → create all models (empty) → set weights_sources(subfolder=None) → [loader calls load_weights()] +``` + +- `weights_sources` points to **root** of model directory, not a subfolder +- Weights have non-diffusers names (e.g., `bagel.language_model.model.layers.0.self_attn.q_proj.weight`) +- `load_weights()` does heavy name normalization + +```python +self.weights_sources = [ + DiffusersPipelineLoader.ComponentSource( + model_or_path=od_config.model, + subfolder=None, # root directory + prefix="", # no prefix stripping + fall_back_to_pt=False, + ) +] +``` + +### Pattern 3: Fully custom loading (DreamID-Omni) + +``` +init → load ALL weights eagerly via custom helpers → load_weights() = no-op +``` + +- No `weights_sources` attribute — standard loader finds nothing to iterate +- Custom init functions (e.g., `init_wan_vae_2_2()`, `load_fusion_checkpoint()`) handle downloading and loading +- `load_weights()` is `pass` +- Weights may come from multiple HF repos in different formats (`.pth`, `.safetensors`) + +Use this when: +- The original model has complex, well-tested loading code you don't want to rewrite +- Weights span multiple HF repos +- Weight format is non-standard (e.g., a single `.pth` file, not sharded safetensors) + +## model_index.json for Custom Models + +Standard diffusers `model_index.json`: +```json +{ + "_class_name": "WanPipeline", + "_diffusers_version": "0.35.0.dev0", + "scheduler": ["diffusers", "UniPCMultistepScheduler"], + "transformer": ["diffusers", "WanTransformer3DModel"], + "vae": ["diffusers", "AutoencoderKLWan"] +} +``` + +Custom model `model_index.json` (minimal): +```json +{ + "_class_name": "DreamIDOmniPipeline", + "fusion": "DreamID-Omni/dreamid_omni.safetensors" +} +``` + +The only **required** field is `_class_name` — it must match a key in `_DIFFUSION_MODELS` in `registry.py`. Other fields are model-specific and accessible via `od_config.model_config` dict. + +## External Dependency Management + +### Git clone + .pth injection (DreamID-Omni pattern) + +```python +def download_dependency(): + CACHE_DIR.mkdir(parents=True, exist_ok=True) + with open(LOCK_FILE, "w") as f: + fcntl.flock(f, fcntl.LOCK_EX) + if not DEPENDENCY_DIR.exists(): + subprocess.run([ + "git", "clone", "--depth", "1", + REPO_URL, "--branch", BRANCH, + str(DEPENDENCY_DIR) + ], check=True) + fcntl.flock(f, fcntl.LOCK_UN) + + # Add to Python path via .pth file + site_packages = Path(site.getsitepackages()[0]) + pth_file = site_packages / "vllm_omni_dependency.pth" + pth_file.write_text(str(DEPENDENCY_DIR)) +``` + +### Direct port (BAGEL pattern) + +Copy essential files from the original repo into `vllm_omni/diffusion/models//`. Adapt imports to use vllm-omni utilities. Benefits: no external dependency, no git clone step. Drawback: must maintain the ported code. + +## Multi-Modal Input/Output Protocols + +Custom models that handle images, audio, or video I/O should implement protocol classes: + +```python +from vllm_omni.diffusion.models.interface import ( + SupportImageInput, # Model accepts image input + SupportAudioInput, # Model accepts audio input + SupportAudioOutput, # Model produces audio output +) + +class MyPipeline(nn.Module, SupportImageInput, SupportAudioInput, SupportAudioOutput): + pass # Protocol markers enable proper engine routing +``` + +The engine checks `isinstance(pipeline, SupportImageInput)` at startup to configure input validation and warmup behavior. + +## Hardcoded Config vs Config Files + +Diffusers models use `config.json` in each subfolder. Custom models often use: + +**Module-level config dicts** (DreamID-Omni): +```python +VIDEO_CONFIG = { + "patch_size": [1, 2, 2], "model_type": "ti2v", + "dim": 3072, "ffn_dim": 14336, "num_heads": 24, "num_layers": 30, ... +} +``` + +**Loaded from custom JSON** (BAGEL): +```python +cfg_path = os.path.join(model_path, "config.json") +with open(cfg_path) as f: + bagel_cfg = json.load(f) +vae_cfg = bagel_cfg.get("vae_config", {}) +``` + +## Custom Architecture Patterns + +### Split forward API (DreamID-Omni) + +When a fusion model needs to interleave blocks from two backbones: + +```python +class WanModel(nn.Module): + def prepare_transformer_block_kwargs(self, x, t, context, ...): + # Patch embed, time embed, text embed, RoPE + return x, e, kwargs + + def post_transformer_block_out(self, x, grid_sizes, e): + # Output projection, unpatchify + return output + + def forward(self, *args, **kwargs): + raise NotImplementedError # Fusion model handles block iteration +``` + +The `FusionModel` then iterates blocks in lock-step: +```python +for video_block, audio_block in zip(self.video_model.blocks, self.audio_model.blocks): + video_out = video_block(video_hidden, ...) + audio_out = audio_block(audio_hidden, ...) + # Cross-attend between modalities + video_out = cross_attention(video_out, audio_out) + audio_out = cross_attention(audio_out, video_out) +``` + +### LLM-as-denoiser (BAGEL) + +When the backbone is a language model that also does diffusion: + +```python +class BagelModel(nn.Module): + def __init__(self): + self.language_model = Qwen2MoTForCausalLM(config) + self.vit_model = SiglipVisionModel(vit_config) +``` + +The LLM processes both text tokens and latent image tokens in a single forward pass, using KV caching for the text portion. + +## Pre/Post Processing for Custom Models + +Custom models typically handle pre/post processing **inside `forward()`** rather than via registered functions, because the logic is tightly coupled: + +```python +def forward(self, req: OmniDiffusionRequest) -> DiffusionOutput: + # Inline preprocessing + image = self._load_and_resize_image(req.prompts[0].get("multi_modal_data", {}).get("image")) + image_latent = self._vae_encode(image) + + # ... denoising loop ... + + # Inline postprocessing + pil_image = self._decode_to_pil(latents) + return DiffusionOutput(output=[pil_image]) +``` + +If pre/post functions are not registered in `_DIFFUSION_PRE_PROCESS_FUNCS` / `_DIFFUSION_POST_PROCESS_FUNCS`, the engine simply skips those steps. + +## Download Script Template + +```python +# examples/offline_inference//download_.py +from huggingface_hub import snapshot_download +import json, os + +def main(output_dir): + # Download model weights from HF + snapshot_download(repo_id="org/model-weights", local_dir=os.path.join(output_dir, "weights")) + + # Download additional components if from separate repos + snapshot_download(repo_id="org/vae-weights", local_dir=os.path.join(output_dir, "vae"), + allow_patterns=["*.safetensors"]) + + # Generate model_index.json + config = {"_class_name": "YourPipeline", "custom_key": "weights/model.safetensors"} + with open(os.path.join(output_dir, "model_index.json"), "w") as f: + json.dump(config, f, indent=2) + + # Install external code dependency (if needed) + download_dependency() + +if __name__ == "__main__": + import argparse + parser = argparse.ArgumentParser() + parser.add_argument("--output-dir", default="./your_model") + args = parser.parse_args() + main(args.output_dir) +``` diff --git a/.claude/skills/add-diffusion-model/references/parallelism-patterns.md b/.claude/skills/add-diffusion-model/references/parallelism-patterns.md new file mode 100644 index 000000000..c6f32d1fa --- /dev/null +++ b/.claude/skills/add-diffusion-model/references/parallelism-patterns.md @@ -0,0 +1,114 @@ +# Parallelism Patterns Reference + +## Tensor Parallelism (TP) + +Replace standard `nn.Linear` with vLLM's parallel linear layers: + +| Pattern | vLLM Layer | When to Use | +|---------|-----------|-------------| +| Fan-out (first in FFN) | `ColumnParallelLinear` | Projection that splits output across ranks | +| Fan-in (second in FFN) | `RowParallelLinear` | Projection that gathers across ranks | +| QKV projection | `QKVParallelLinear` | Fused Q/K/V for self-attention | +| Single Q or K or V | `ColumnParallelLinear` | Separate projections (cross-attention) | +| Attention output | `RowParallelLinear` | Output projection after attention | + +```python +from vllm.model_executor.layers.linear import ( + ColumnParallelLinear, + RowParallelLinear, + QKVParallelLinear, +) + +class TPFeedForward(nn.Module): + def __init__(self, dim, ffn_dim): + super().__init__() + self.fc1 = ColumnParallelLinear(dim, ffn_dim) + self.fc2 = RowParallelLinear(ffn_dim, dim) + + def forward(self, x): + x, _ = self.fc1(x) + x = torch.nn.functional.gelu(x) + x, _ = self.fc2(x) + return x +``` + +**TP constraints**: `hidden_dim`, `num_heads`, and `num_kv_heads` must be divisible by `tp_size`. + +### RMSNorm with TP + +When RMSNorm sits between TP-sharded dimensions, use `DistributedRMSNorm` from the Wan2.2 implementation pattern — it computes global RMS via all-reduce across TP ranks. + +## CFG Parallelism + +Inherit `CFGParallelMixin` in your pipeline and implement `predict_noise()`: + +```python +from vllm_omni.diffusion.distributed.cfg_parallel.cfg_parallel import CFGParallelMixin + +class MyPipeline(nn.Module, CFGParallelMixin): + def predict_noise(self, model, latent_model_input, t, prompt_embeds, **kwargs): + return model(latent_model_input, t, prompt_embeds, **kwargs) + + def forward(self, req): + # In the denoising loop: + noise_pred = self.predict_noise_maybe_with_cfg( + model=self.transformer, + sample=latents, + timestep=t, + prompt_embeds=prompt_embeds, + guidance_scale=guidance_scale, + do_cfg=guidance_scale > 1.0, + ) + latents = self.scheduler_step_maybe_with_cfg( + self.scheduler, noise_pred, t, latents + ) +``` + +## Sequence Parallelism (SP) + +SP is applied non-intrusively via the `_sp_plan` dict on the transformer class. The framework applies hooks at module boundaries to shard/gather sequences. + +```python +from vllm_omni.diffusion.distributed.sp_plan import ( + SequenceParallelInput, + SequenceParallelOutput, +) + +class MyTransformer(nn.Module): + _sp_plan = { + # Split hidden_states input on dim=1 before first block + "blocks.0": SequenceParallelInput(split_dim=1), + # Gather output on dim=1 after final projection + "proj_out": SequenceParallelOutput(gather_dim=1), + } +``` + +For RoPE that needs splitting, add an entry for the RoPE module: + +```python +_sp_plan = { + "rope": SequenceParallelInput(split_dim=1, split_output=True, auto_pad=True), + "blocks.0": SequenceParallelInput(split_dim=1), + "proj_out": SequenceParallelOutput(gather_dim=1), +} +``` + +The `auto_pad=True` flag handles variable sequence lengths by padding to be divisible by SP degree and creating attention masks accordingly. + +## VAE Patch Parallelism + +If using `DistributedAutoencoderKLWan` or similar distributed VAE, the framework handles spatial sharding automatically. Set `vae_patch_parallel_size` in the parallel config. + +## HSDP (Hybrid Sharded Data Parallel) + +HSDP uses PyTorch FSDP2 to shard transformer weights. No code changes needed in the model — the loader handles it. Set `use_hsdp=True` in `DiffusionParallelConfig`. + +## Adding Parallelism Incrementally + +Recommended order: +1. **Basic single-GPU**: Get generation working first +2. **Tensor Parallelism**: Replace Linear layers, update `load_weights` for QKV fusion +3. **CFG Parallel**: Add `CFGParallelMixin`, implement `predict_noise` +4. **Sequence Parallelism**: Add `_sp_plan` to transformer +5. **HSDP**: Usually works out-of-box after TP is done +6. **VAE Patch Parallel**: Switch to distributed VAE class diff --git a/.claude/skills/add-diffusion-model/references/transformer-adaptation.md b/.claude/skills/add-diffusion-model/references/transformer-adaptation.md new file mode 100644 index 000000000..6e344b6a6 --- /dev/null +++ b/.claude/skills/add-diffusion-model/references/transformer-adaptation.md @@ -0,0 +1,218 @@ +# Transformer Adaptation Reference + +## Adapting a Diffusers Transformer to vLLM-Omni + +### Step-by-step Checklist + +1. Copy the transformer class from diffusers source +2. Remove all mixin classes — inherit only from `nn.Module` +3. Replace attention dispatch with `vllm_omni.diffusion.attention.layer.Attention` +4. Replace logger with `vllm.logger.init_logger` +5. Add `od_config: OmniDiffusionConfig | None = None` to `__init__` +6. Remove training-only code (gradient checkpointing, dropout) +7. Add `load_weights()` method for weight loading from safetensors +8. Add class-level attributes for acceleration features + +### Mixin Removal + +Remove these diffusers mixins (and their imports): + +```python +# Remove all of these: +from diffusers.models.modeling_utils import ModelMixin +from diffusers.configuration_utils import ConfigMixin, register_to_config +from diffusers.models.attention_processor import AttentionModuleMixin +from diffusers.loaders import PeftAdapterMixin, FromOriginalModelMixin + +# Replace: +class MyTransformer(ModelMixin, ConfigMixin, AttentionModuleMixin): +# With: +class MyTransformer(nn.Module): +``` + +Also remove `@register_to_config` decorators from `__init__`. + +### Attention Replacement + +The vLLM-Omni `Attention` layer wraps backend selection (FlashAttention, SDPA, SageAttn, etc.) and supports sequence parallelism hooks. + +**QKV tensor shape must be `[batch, seq_len, num_heads, head_dim]`.** + +#### Self-Attention Pattern + +```python +from vllm_omni.diffusion.attention.layer import Attention +from vllm_omni.diffusion.attention.backends.abstract import AttentionMetadata + +class SelfAttentionBlock(nn.Module): + def __init__(self, dim, num_heads): + super().__init__() + self.num_heads = num_heads + self.head_dim = dim // num_heads + + self.to_q = nn.Linear(dim, dim) + self.to_k = nn.Linear(dim, dim) + self.to_v = nn.Linear(dim, dim) + self.to_out = nn.Linear(dim, dim) + + self.attn = Attention( + num_heads=num_heads, + head_size=self.head_dim, + softmax_scale=1.0 / (self.head_dim ** 0.5), + causal=False, + num_kv_heads=num_heads, + ) + + def forward(self, x, attn_mask=None): + B, S, _ = x.shape + q = self.to_q(x).view(B, S, self.num_heads, self.head_dim) + k = self.to_k(x).view(B, S, self.num_heads, self.head_dim) + v = self.to_v(x).view(B, S, self.num_heads, self.head_dim) + + attn_metadata = AttentionMetadata(attn_mask=attn_mask) + out = self.attn(q, k, v, attn_metadata=attn_metadata) + out = out.reshape(B, S, -1) + return self.to_out(out) +``` + +#### Fused QKV with TP (Advanced) + +For tensor parallelism, use vLLM's parallel linear layers: + +```python +from vllm.model_executor.layers.linear import ( + QKVParallelLinear, RowParallelLinear +) + +class TPSelfAttention(nn.Module): + def __init__(self, dim, num_heads): + super().__init__() + self.num_heads = num_heads + self.head_dim = dim // num_heads + + self.to_qkv = QKVParallelLinear( + hidden_size=dim, + head_size=self.head_dim, + total_num_heads=num_heads, + total_num_kv_heads=num_heads, + ) + self.to_out = RowParallelLinear(dim, dim) + + self.attn = Attention( + num_heads=num_heads, + head_size=self.head_dim, + softmax_scale=1.0 / (self.head_dim ** 0.5), + causal=False, + num_kv_heads=num_heads, + ) +``` + +### Logger Replacement + +```python +# Replace: +from diffusers.utils import logging +logger = logging.get_logger(__name__) + +# With: +from vllm.logger import init_logger +logger = init_logger(__name__) +``` + +### Custom Layers from vLLM-Omni + +Available utility layers: + +```python +from vllm.model_executor.layers.layernorm import RMSNorm +from vllm_omni.diffusion.layers.rope import RotaryEmbedding +from vllm_omni.diffusion.layers.adalayernorm import AdaLayerNorm +``` + +### Config Support + +```python +from vllm_omni.diffusion.data import OmniDiffusionConfig + +class MyTransformer(nn.Module): + def __init__(self, *, od_config=None, num_layers=28, hidden_size=3072, **kwargs): + super().__init__() + self.od_config = od_config + self.parallel_config = od_config.parallel_config if od_config else None + # ... build layers +``` + +The transformer config values come from `model_index.json` → `config.json` in the transformer subfolder. The pipeline uses `get_transformer_config_kwargs(od_config.tf_model_config, TransformerClass)` to filter config keys to match the `__init__` signature. + +### Weight Loading + +The `load_weights` method receives an iterable of `(name, tensor)` from safetensors files, with the prefix (e.g., `"transformer."`) already stripped by the loader. + +```python +from vllm.model_executor.model_loader.weight_utils import default_weight_loader + +class MyTransformer(nn.Module): + def load_weights(self, weights): + params = dict(self.named_parameters()) + loaded = set() + for name, tensor in weights: + # Optional: remap names from diffusers to vllm-omni naming + # e.g., "ff.net.0.proj" -> "ff.net_0.proj" + + if name in params: + param = params[name] + if hasattr(param, "weight_loader"): + param.weight_loader(param, tensor) + else: + default_weight_loader(param, tensor) + loaded.add(name) + return loaded +``` + +#### QKV Fusion in load_weights + +If you fused separate Q/K/V into a `QKVParallelLinear`, you need to map diffusers' separate weight names: + +```python +stacked_params_mapping = [ + ("to_qkv", "to_q", "q"), + ("to_qkv", "to_k", "k"), + ("to_qkv", "to_v", "v"), +] + +def load_weights(self, weights): + params = dict(self.named_parameters()) + loaded = set() + for name, tensor in weights: + for fused_name, orig_name, shard_id in stacked_params_mapping: + if orig_name in name: + name = name.replace(orig_name, fused_name) + param = params[name] + param.weight_loader(param, tensor, shard_id) + loaded.add(name) + break + else: + # Normal loading + ... + return loaded +``` + +### Class-Level Attributes for Features + +```python +class MyTransformer(nn.Module): + # torch.compile: list block class names that repeat and can be compiled + _repeated_blocks = ["MyTransformerBlock"] + + # CPU offload: attribute name of the nn.ModuleList containing blocks + _layerwise_offload_blocks_attr = "blocks" + + # LoRA: mapping of fused param names to original param names + packed_modules_mapping = {"to_qkv": ["to_q", "to_k", "to_v"]} + + # Sequence parallelism plan (advanced — add after basic impl works) + _sp_plan = { + "blocks.0": SequenceParallelInput(split_dim=1), + "proj_out": SequenceParallelOutput(gather_dim=1), + } +``` diff --git a/.claude/skills/add-diffusion-model/references/troubleshooting.md b/.claude/skills/add-diffusion-model/references/troubleshooting.md new file mode 100644 index 000000000..4c63cdff9 --- /dev/null +++ b/.claude/skills/add-diffusion-model/references/troubleshooting.md @@ -0,0 +1,103 @@ +# Troubleshooting Reference + +## Common Errors When Adding a Diffusion Model + +### ImportError / ModuleNotFoundError + +**Cause**: Missing or incorrect registration. + +**Fix checklist**: +1. Model registered in `vllm_omni/diffusion/registry.py` `_DIFFUSION_MODELS` dict +2. `__init__.py` exports the pipeline class +3. Pipeline file exists at the correct path: `vllm_omni/diffusion/models/{folder}/{file}.py` +4. Class name in registry matches the actual class name in the file + +### Shape Mismatch in Attention + +**Symptom**: `RuntimeError: shape mismatch` or `expected 4D tensor` + +**Cause**: QKV tensors not reshaped to `[batch, seq_len, num_heads, head_dim]`. + +**Fix**: Before calling `self.attn(q, k, v, ...)`, ensure: +```python +q = q.view(batch, seq_len, self.num_heads, self.head_dim) +k = k.view(batch, kv_seq_len, self.num_kv_heads, self.head_dim) +v = v.view(batch, kv_seq_len, self.num_kv_heads, self.head_dim) +``` + +After attention, reshape back: +```python +out = out.reshape(batch, seq_len, -1) +``` + +### Weight Loading Failures + +**Symptom**: `RuntimeError: size mismatch for parameter ...` or missing keys + +**Debugging**: +1. Print diffusers weight names: `safetensors.safe_open(path, "pt").keys()` +2. Print model parameter names: `dict(model.named_parameters()).keys()` +3. Compare and add name remappings in `load_weights()` + +**Common remappings needed**: +- `ff.net.0.proj` → `ff.net_0.proj` (PyTorch Sequential indexing) +- `.to_out.0.` → `.to_out.` (Sequential unwrapping) +- `scale_shift_table` → moved to a wrapper module + +### Black/Blank/Noisy Output + +**Possible causes**: +1. **Wrong latent normalization**: Check VAE expects latents scaled by `vae.config.scaling_factor` +2. **Wrong scheduler**: Using the wrong scheduler class or wrong `flow_shift` +3. **Missing CFG**: Some models require `guidance_scale > 1.0` with negative prompt +4. **Wrong timestep format**: Some schedulers expect float, others expect int/long +5. **Missing post-processing**: Raw VAE output may need denormalization + +**Quick test**: Run with diffusers directly using the same seed and compare latents at each step. + +### OOM (Out of Memory) + +**Solutions** (in order of preference): +1. `--enforce-eager` to disable torch.compile (saves compile memory) +2. `--enable-cpu-offload` for model-level offload +3. `--enable-layerwise-offload` for block-level offload (better for large models) +4. `--vae-use-slicing --vae-use-tiling` for VAE memory reduction +5. Reduce resolution: `--height 480 --width 832` +6. Use TP: `--tensor-parallel-size 2` + +### Different Output vs Diffusers Reference + +**Common causes**: +1. **Attention backend difference**: FlashAttention vs SDPA may produce slightly different results. Set `DIFFUSION_ATTENTION_BACKEND=TORCH_SDPA` to match diffusers +2. **Float precision**: vLLM-Omni may use bfloat16 where diffusers uses float32 for some operations +3. **Missing normalization**: Check all LayerNorm/RMSNorm are preserved +4. **Scheduler rounding**: Some schedulers have numerical sensitivity + +### Tensor Parallel Errors + +**Symptom**: `AssertionError: not divisible` or incorrect output with TP>1 + +**Fix**: +1. Verify `hidden_dim % tp_size == 0` and `num_heads % tp_size == 0` +2. Ensure `ColumnParallelLinear` / `RowParallelLinear` are used correctly +3. Check that norms between parallel layers use distributed norm if needed +4. Verify `load_weights` handles TP sharding for norm weights + +### Model Not Detected / Wrong Pipeline Class + +**Symptom**: `ValueError: Model class ... not found in diffusion model registry` + +**Cause**: The model's `model_index.json` has a `_class_name` for the pipeline that doesn't match registry keys. + +**Fix**: The registry key must match the diffusers pipeline class name from `model_index.json`. If using a different name, map it in the registry: +```python +"DiffusersPipelineClassName": ("your_folder", "your_file", "YourVllmClassName"), +``` + +## Debugging Workflow + +1. **Add verbose logging**: Use `logger.info()` to print tensor shapes at each stage +2. **Compare step-by-step**: Run diffusers and vllm-omni side by side, comparing tensors after each major operation +3. **Use small configs**: Reduce `num_inference_steps=2`, small resolution for fast iteration +4. **Test transformer isolation**: Feed the same input to both diffusers and vllm-omni transformers, compare outputs +5. **Binary search for bugs**: Comment out blocks/layers to isolate where divergence starts diff --git a/.claude/skills/add-tts-model/SKILL.md b/.claude/skills/add-tts-model/SKILL.md new file mode 100644 index 000000000..9546f187d --- /dev/null +++ b/.claude/skills/add-tts-model/SKILL.md @@ -0,0 +1,274 @@ +--- +name: add-tts-model +description: Integrate a new TTS model into vLLM-Omni from HuggingFace reference implementation through production-ready serving with streaming and CUDA graph acceleration. Use when adding a new TTS model, wiring stage separation, enabling online speech serving, or debugging TTS integration behavior. +--- + +# TTS Model Integration Workflow + +## Overview + +Integrating a new TTS model into vLLM-Omni follows a phased approach. Each phase builds on the previous one, and the model becomes usable at each stage with increasing capabilities. + +``` +HF Reference -> Stage Separation -> Online Serving -> Async Chunk -> CUDA Graph + (Phase 1) (Phase 2) (Phase 3) (Phase 4) (Phase 5) +``` + +## Phase 1: HuggingFace Reference + +**Goal**: Understand the reference implementation and verify it produces correct audio. + +### Steps + +1. **Run the reference model** end-to-end using the official HuggingFace / GitHub code +2. **Document the architecture**: + - What are the sub-models? (AR decoder, codec decoder, vocoder, etc.) + - What is the token vocabulary? (semantic codes, RVQ codebooks, special tokens) + - What is the output format? (sample rate, channels, codec type) +3. **Capture reference outputs** for comparison during integration +4. **Identify the config structure**: `config.json` fields, `model_type`, sub-model configs + +### Key Questions + +- How many codebooks? What are the codebook sizes? +- What special tokens exist? (`<|voice|>`, `<|audio_start|>`, `<|im_end|>`, etc.) +- What is the token-to-ID mapping for codec codes? +- What is the hop length / frame rate of the codec? +- Does the model support voice cloning? How? (reference audio encoding, speaker embeddings, etc.) + +### Deliverables + +- Working reference script that produces audio +- Architecture diagram / notes +- Token vocabulary mapping +- Reference audio samples for regression testing + +## Phase 2: Stage Separation (Offline Inference) + +**Goal**: Split the model into vLLM-Omni stages and get offline inference working. + +### Steps + +1. **Register the model** in `vllm_omni/model_executor/models/registry.py` +2. **Create config classes** (`configuration_.py`) with `model_type` registration +3. **Implement Stage 0** (AR model): + - Subclass appropriate base (e.g., wrap Qwen3 decoder layers) + - Implement `forward()` for autoregressive token generation + - Handle special token logic (start/stop tokens, codec token mapping) + - If dual-AR (like Fish Speech), implement Fast AR as a nested module +4. **Implement Stage 1** (Decoder): + - Load codec weights (may need lazy loading from separate checkpoint) + - Implement `forward()`: codec codes -> audio waveform + - Return `OmniOutput` with `multimodal_outputs` +5. **Create stage config YAML** defining both stages, memory allocation, and model paths +6. **Create stage input processor** for prompt building +7. **Write end2end.py** test script + +### Critical Parameters to Get Right + +| Parameter | Impact if Wrong | +|-----------|----------------| +| Hop length | Audio duration wrong, streaming noise | +| Token ID mapping | Garbage codes -> noise output | +| Codebook count/size | Shape mismatch crashes | +| Stop token | Generation never stops or stops too early | +| dtype / autocast | Numerical issues, silent quality degradation | +| Repetition penalty | Must match reference (often 1.0 for TTS) | + +### Debugging Priority (from experience) + +When audio output is wrong, check in this order: + +1. **RoPE / attention**: Are position encodings correct? Is the attention mask right? +2. **Normalization**: RMSNorm epsilon, layer norm placement (pre vs post) +3. **Hop length**: Product of all upsample rates in the codec decoder +4. **Token mapping**: Are codec IDs correctly offset from the vocabulary base? +5. **Sampling parameters**: Temperature, top_k, top_p, repetition_penalty +6. **Tensor layout**: Codebook-major vs frame-major ordering +7. **dtype**: Float32 for codec decoders (autocast can corrupt audio) + +### Deliverables + +- Model files in `vllm_omni/model_executor/models//` +- Stage config YAML +- Working `end2end.py` with correct audio output +- README.md in the example directory + +## Phase 3: Online Serving + +**Goal**: Expose the model via `/v1/audio/speech` API endpoint. + +### Steps + +1. **Register in `serving_speech.py`**: + - Add model stage name to `_TTS_MODEL_STAGES` set + - Add model detection flag (e.g., `_is_fish_speech`) + - Implement prompt builder method (e.g., `_build_fish_speech_prompt()`) +2. **Handle model-specific parameters**: + - Voice cloning: `ref_audio` encoding and prompt injection + - `max_new_tokens` override in sampling params + - Model-specific default values +3. **Create client scripts**: `speech_client.py`, `run_server.sh` +4. **Test all response formats**: wav, mp3, flac, pcm +5. **Add Gradio demo**: Interactive web UI with streaming support + +### Voice Cloning Pattern + +If the model supports voice cloning via reference audio encoding: + +```python +# In serving_speech.py: +# 1. Resolve ref_audio (URL, base64, or file path) to audio bytes +# 2. Encode on CPU using the model's codec (e.g., DAC for Fish Speech) +# 3. Convert to token IDs and inject into the prompt as a system message +``` + +### Deliverables + +- Updated `serving_speech.py` with model-specific prompt builder +- Client scripts and server launcher +- Gradio demo with streaming and voice cloning UI +- Documentation (offline + online serving docs) + +## Phase 4: Async Chunk (Streaming) + +**Goal**: Enable inter-stage streaming so audio chunks are produced while AR generation continues. + +### Steps + +1. **Update stage config YAML**: + ```yaml + async_chunk: true + codec_chunk_frames: 25 # frames per chunk + codec_left_context_frames: 25 # overlap for smooth boundaries + ``` +2. **Implement chunk handling in Stage 1**: + - Accept partial input (chunk of codec codes) + - Handle left context for smooth audio boundaries + - Return partial audio in `OmniOutput` +3. **Test streaming**: + - Verify audio quality matches non-streaming output + - Check for artifacts at chunk boundaries + - Measure TTFA (time to first audio) +4. **Update online serving** to support `stream=true` with PCM output + +### Streaming Architecture + +``` +Stage 0 (AR) Stage 1 (Decoder) + | | + |-- chunk 0 (25 frames) ------> decode -> audio chunk 0 -> client + |-- chunk 1 (25 frames) ------> decode -> audio chunk 1 -> client + |-- chunk 2 (25 frames) ------> decode -> audio chunk 2 -> client + ... +``` + +### Key Considerations + +- **Left context overlap**: Prevents audible artifacts at chunk boundaries +- **Hop length matters**: `context_audio_samples = context_frames * hop_length` +- **First chunk latency**: Can use larger initial chunk for better quality, then smaller chunks + +### Deliverables + +- Updated stage config with async_chunk enabled +- Smooth streaming audio without boundary artifacts +- TTFA metrics + +## Phase 5: CUDA Graph Acceleration + +**Goal**: Capture the AR loop as a CUDA graph for significant speedup. + +### Steps + +1. **Identify the hot loop**: The AR decoding loop that runs N steps per token +2. **Create static buffers**: + - KV caches with fixed max sequence length + - Pre-built causal masks and position tensors per step + - Static input/output tensors +3. **Implement graph capture**: + - Warm up with real data + - Capture the forward pass + - Replay with updated inputs +4. **Handle constraints**: + - Use `torch.argmax` instead of `torch.multinomial` (graph-safe) + - Fixed batch size (fall back to eager for other sizes) + - No dynamic control flow inside the graph + +### Example: Code Predictor CUDA Graph (Qwen3-TTS) + +```python +class CodePredictorGraph: + """Captures the 16-step code predictor AR loop as a single CUDA graph.""" + + def setup_graph(self, device): + # Pre-allocate static KV caches [1, kv_heads, 16, head_dim] + # Pre-build causal masks and positions for each step + # Warm up -> capture -> store graph + + def run_graph(self, initial_input): + # Copy input to static buffer + # Replay captured graph + # Read output from static buffer +``` + +### Performance Expectations + +Based on Qwen3-TTS code predictor experience: +- **3-5x speedup** for the graphed component +- Only effective for fixed batch sizes (typically batch_size=1) +- Falls back to eager mode for unsupported configurations + +### Deliverables + +- CUDA graph implementation for the AR hot loop +- Benchmark script comparing eager vs graph performance +- Documentation of constraints and fallback behavior + +## Integration Checklist + +Use this checklist when integrating a new TTS model: + +### Phase 1: HF Reference +- [ ] Reference model runs and produces correct audio +- [ ] Architecture documented (stages, codebooks, tokens, sample rate) +- [ ] Reference audio samples saved for comparison + +### Phase 2: Stage Separation +- [ ] Model registered in `registry.py` +- [ ] Config classes created with `model_type` registration +- [ ] Stage 0 (AR) implemented and generates correct tokens +- [ ] Stage 1 (Decoder) produces correct audio from tokens +- [ ] Stage config YAML created +- [ ] `end2end.py` produces audio matching reference quality +- [ ] README.md written + +### Phase 3: Online Serving +- [ ] Model added to `serving_speech.py` +- [ ] Prompt builder handles text input correctly +- [ ] Voice cloning works (if supported) +- [ ] All response formats work (wav, mp3, flac, pcm) +- [ ] Client scripts and server launcher created +- [ ] Gradio demo working +- [ ] Documentation added (offline + online docs, nav, supported models) + +### Phase 4: Async Chunk +- [ ] Stage config updated with `async_chunk: true` +- [ ] Stage 1 handles partial chunks correctly +- [ ] No audio artifacts at chunk boundaries +- [ ] Streaming via API (`stream=true`) works +- [ ] TTFA measured and acceptable + +### Phase 5: CUDA Graph +- [ ] Hot loop identified and profiled +- [ ] Static buffers allocated +- [ ] Graph captured and replays correctly +- [ ] Benchmark shows meaningful speedup +- [ ] Fallback to eager works for unsupported configs + +## References + +- [TTS audio skill](../vllm-omni-audio-tts/SKILL.md) -- supported models and usage +- [Fish Speech integration](../vllm-omni-audio-tts/references/fish-speech.md) -- complete example of Phases 1-3 +- [Qwen3-TTS reference](../vllm-omni-audio-tts/references/qwen-tts.md) -- complete example of all 5 phases +- [Adding a TTS model (developer guide)](https://github.com/vllm-project/vllm-omni/blob/main/docs/contributing/model/adding_tts_model.md) diff --git a/.claude/skills/readme.md b/.claude/skills/readme.md new file mode 100644 index 000000000..b66f2ecd1 --- /dev/null +++ b/.claude/skills/readme.md @@ -0,0 +1,34 @@ +# Claude Skills for vLLM-Omni + +This directory contains Claude Code skills maintained for the `vllm-omni` +repository. These skills capture repeatable workflows for common contributor +tasks such as model integration, pull request review, and release note +generation. + +## Directory Structure + +Each skill lives in its own directory under `.claude/skills/`. A skill may +include: + +- `SKILL.md`: the main workflow and operating instructions +- `references/`: focused reference material used by the skill +- `scripts/`: small helper scripts used by the skill + +## Available Skills + +- `add-diffusion-model`: guides integration of a new diffusion model into + `vllm-omni` +- `add-omni-model`: covers addition of new omni-modality model support +- `add-tts-model`: covers integration of new TTS models and related serving + workflows +- `generate-release-note`: helps prepare release notes for repository changes +- `review-pr`: provides a structured workflow for reviewing pull requests + +## Maintenance Guidelines + +- Keep skill names short and task-oriented. +- Prefer repository-local paths, commands, and examples. +- Avoid hardcoding fast-changing support matrices unless the skill is actively + maintained alongside those changes. +- Treat skills as contributor tooling: optimize for clarity, actionability, and + low maintenance overhead. diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md new file mode 100644 index 000000000..6684be039 --- /dev/null +++ b/.claude/skills/review-pr/SKILL.md @@ -0,0 +1,357 @@ +--- +name: review-pr +description: Review PRs on vllm-project/vllm-omni by routing to the right domain skills, checking critical evidence, and focusing comments on blocking issues. Use when reviewing pull requests, triaging review depth, or checking tests, benchmarks, and breaking changes in vllm-omni. +--- + +# vLLM-Omni PR Review + +## Overview + +You are an adversarial reviewer. Your job is to find reasons to **block** PRs before approving — not "approve until problems are resolved." Assume blocking issues exist until proven otherwise. Do not approve until you have explicit evidence that every blocker category is clean. + +Use this skill as a router for `vllm-project/vllm-omni` pull request reviews. Keep the default context small, load only the references that match the diff, and prioritize high-confidence findings over coverage theater. + +## Priority Hierarchy Under Context Pressure + +If context is limited, prioritize: blocker scan → evidence → domain routing → verdict. + +Always run the blocker scan. Under context pressure, do a shallow scan of the most critical categories (Correctness, Security) and flag that the scan was incomplete. + +## Core Workflow + +### Step 0: Verify Review Gates First + +Check mergeability and required checks (DCO, pre-commit, mergeability). If failing, stop and ask the author to fix gates before proceeding. + +For gate commands, review submission, and comment style, see [references/review-execution.md](references/review-execution.md). + +### Step 0.5: Check PR Size for Large Changes + +For substantial changes (more than 1000 LOC OR more than 10 files changed): +- Ask the contributor to run L3 tests locally and paste results in PR description (highly recommended) + +Example: +> This PR is substantial (>1000 LOC / >10 files). Could you please run the [L3 tests](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/test_guide/#l3-level--l4-level) locally and paste the results here? + +Then continue with the workflow below. + +### Step 1: Gather Minimal Context + +Fetch: +- PR metadata and changed files +- The diff +- Linked issues for `[Bugfix]` and `[Feature]` PRs only when conventions are unclear +- Related PRs only when conventions or prior decisions are unclear + +Do not fetch broad extra context unless the diff leaves real ambiguity. + +### Step 2: Blocker Scan (Required First) + +Execute this scan before any other review activity. For each category, explicitly mark PASS or list blocking issues found. + +``` +BLOCKER scan: +| Category | Result | +|---------------------|-----------------------------------------| +| Correctness | PASS / ISSUES: (list) | +| Reliability/Safety | PASS / ISSUES: (list) | +| Breaking Changes | PASS / (check PR description first) | +| Test Coverage | PASS / (check PR desc for evidence) / needs tests | +| Documentation | PASS / ISSUES: (list) | +| Security | PASS / ISSUES: (list) | +``` + +**Blocker categories:** + +| Category | Flag These Patterns | +|----------|---------------------| +| **Correctness** | Silent exception swallows, uninitialized variables, off-by-one errors, logic inversions, missing returns | +| **Reliability/Safety** | Unclosed resources, race conditions, missing None checks, hardcoded timeouts, silent fallbacks | +| **Breaking Changes** | Signature changes without compat, removed public APIs, changed defaults, config removals | +| **Test Coverage** | Bug fix without regression test, new API without tests, performance claims without benchmarks | +| **Documentation** | New public API without docs, breaking changes without migration guide, new config without docs | +| **Security** | Hardcoded secrets, user input in eval/format strings, insecure deserialization | + +**Evidence standard:** Code inspection suffices for code-level blockers. For test coverage, require CI logs or PR description evidence. + +**Confidence threshold:** Flag obvious cases only. For suspicious but uncertain cases, add a non-blocking comment. + +**Special cases:** +| PR Type | Action | +|---------|--------| +| Doc-only PRs | Skip categories 1-4 and 6, proceed to 5 (Documentation) | +| Config-only PRs | Focus on Breaking Changes + Documentation | +| Test-only PRs | Focus on Correctness of test logic | +| Draft PRs | Do not block; add a single non-blocking comment: "Ready for full review when draft status removed. Preliminary scan available on request." | + +For detailed anti-patterns with code examples, see [references/blocker-patterns.md](references/blocker-patterns.md). + +**If blockers found:** +``` +BLOCKING ISSUES: +1. [Category] [Line/File] - [description] +2. ... +VERDICT: REQUEST_CHANGES (cannot approve until blockers resolved) +``` + +**If no blockers:** +List non-blocking suggestions and proceed to Step 3. + +### Step 3: Route to the Right Skill + +Use the title prefix and changed directories to decide whether a domain skill is required. + +| Signal | Action | +| ------------------------------------------- | ---------------------------- | +| `[Image]`, `[ImageGen]` | Use `vllm-omni-image-gen` | +| `[Video]`, `[VideoGen]` | Use `vllm-omni-video-gen` | +| `[Audio]`, `[TTS]` | Use `vllm-omni-audio-tts` | +| `[Multimodal]` | Use `vllm-omni-multimodal` | +| `[Distributed]` | Use `vllm-omni-distributed` | +| `[Quantization]` | Use `vllm-omni-quantization` | +| `[Performance]` | Use `vllm-omni-perf` | +| `[Hardware]` or backend-specific code | Use `vllm-omni-hardware` | +| `[API]` or `vllm_omni/entrypoints/` changes | Use `vllm-omni-api` | +| `[CI]` | Use `vllm-omni-cicd` | +| `[Model]` | Use `vllm-omni-contrib` | + +For multi-skill routing and hardware detection, see [references/review-routing.md](references/review-routing.md). + +### Step 4: Load Only the Relevant Review Reference + +Load targeted references based on the diff: + +| Diff Area | Load | +| ----------------------------------------------------------------------------------------- | ---------------------------------------------------------- | +| `vllm_omni/engine/`, `vllm_omni/stages/`, `vllm_omni/connectors/`, `vllm_omni/diffusion/` | [references/pitfalls.md](references/pitfalls.md) | +| Async, distributed coordination, validation, connector behavior | [references/code-patterns.md](references/code-patterns.md) | +| Scheduler, stage boundaries, execution model, critical paths | [references/architecture.md](references/architecture.md) | +| High-risk changes (core logic, configs/params, error handling, concurrency/distributed, I/O) or `[Feature]` / `[Bugfix]` PRs | [references/tests-docs-checklist.md](references/tests-docs-checklist.md) | + +Avoid loading all three by default. + +### Step 5: Ask for Concrete Validation Evidence + +When tests or benchmarks are missing **and PR description evidence is insufficient**, ask for specific evidence: + +| Change Type | Minimum Evidence to Request | +|-------------|-----------------------------| +| API behavior | Functional tests covering success + invalid input + response contract | +| Model execution | Inference correctness tests comparing outputs against baseline | +| Performance optimization | Benchmark showing before/after latency on stated hardware | +| New feature (performance-affecting) | Performance comparison test: baseline vs. with change (latency, throughput, VRAM) | +| Memory management | Peak memory measurement showing no regression | +| Bug fixes | Regression test that reproduces the original bug | + +For `[Feature]` PRs affecting performance or `[Performance]` PRs, use the checklist in [references/tests-docs-checklist.md](references/tests-docs-checklist.md) section 5. + +Be explicit in review comments. Treat "manual verification only" as insufficient unless automation is genuinely impossible. + +### Step 6: Final Verdict + +Use the review body to summarize: +- What was validated +- What still lacks evidence +- What must change before approval + +**Verdict format:** +``` +BLOCKER scan: +- Correctness: [PASS / ISSUES: (list)] +- Reliability/Safety: [PASS / ISSUES: (list)] +- Breaking Changes: [PASS / ISSUES: (list)] +- Test Coverage: [PASS / (check PR desc) / needs tests] +- Documentation: [PASS / ISSUES: (list)] +- Security: [PASS / ISSUES: (list)] + +OVERALL: [NO BLOCKERS / X BLOCKERS FOUND] + +VERDICT: [APPROVE / COMMENT / REQUEST_CHANGES] +``` + +For comment budget and phrasing, see [references/review-execution.md](references/review-execution.md). + +## Review Heuristics + +- Check PR description evidence before requesting tests +- Only flag missing tests when evidence is genuinely absent +- For [Bugfix] PRs, require a regression test unless automation is impossible +- For API-facing PRs, prefer contract tests over broad smoke tests +- Be suspicious of silent fallbacks, swallowed exceptions, device-specific assumptions +- Review critical paths first: engine, connectors, stages, API entrypoints +- Skip nits and style comments unless they hide a correctness issue + +## Scenario Coverage + +| Scenario | Blocker Scan | Domain Routing | Verdict | +|----------|--------------|----------------|---------| +| Standard code PR | Full 6-category scan | Route by prefix/diff | Standard format | +| Doc-only PR | Skip to Documentation only | Skip | Standard format | +| Config-only PR | Breaking Changes + Documentation | Skip | Standard format | +| Test-only PR | Correctness of test logic | Skip | Standard format | +| Draft PR | Run scan (non-blocking); offer preliminary feedback on request | Skip | COMMENT: "Ready for full review when draft removed" | +| Large PR (>1000 LOC) | Shallow scan + request L3 tests | Route by prefix/diff | Standard format | + +## When to Fetch More Context + +Fetch more context when: +- The diff snippet hides lifecycle or cleanup behavior +- A config key or API field is introduced without nearby validation +- A benchmark claim references unseen measurement code +- The PR appears to rely on prior design discussion + +Keep additional fetches narrow and tied to a specific uncertainty. + +## Diffusion Model PR Review + +> **Invoked when PR prefix is `[Model]`, `[New Model]`, `[Image]`, `[ImageGen]`, `[Video]`, `[VideoGen]`, or `[Diffusion]`.** + +All four dimensions below must pass before approving. Work through them in priority order. See [references/diffusion-checklist.md](references/diffusion-checklist.md) for full per-item criteria across all dimensions. + +### Priority Order + +1. **PR body evidence** — missing samples/metrics block the entire review +2. **Missing inference mode** — model must work offline and online +3. **Missing acceleration feature** — required for all new diffusion models +4. **Missing memory optimization** — required for all new diffusion models +5. **Missing documentation** — tables and examples must be updated +6. **Missing e2e test** — required for merge gate +7. **Missing offline inference test** — recommended; required only when no e2e test is present +8. **Combined feature test** — required when two or more acceleration/memory features are implemented + +--- + +### Dimension 1: PR Body Completeness + +Required: generation script, sample outputs, e2e latency, peak VRAM. Recommended: matching diffusers baseline for all four. + +**Comment when required items are missing:** + +``` +🔴 **PR Body Incomplete — Required Evidence Missing** + +The following items are required before this PR can be reviewed: + +- [ ] vLLM-Omni generation script (offline `Omni` or online `vllm serve`) +- [ ] Generated sample output (image / video / audio) +- [ ] vLLM-Omni e2e latency (hardware: GPU model, count; resolution; steps) +- [ ] vLLM-Omni peak VRAM usage (GB) + +Please update the PR description with this information. +``` + +**Comment when diffusers baseline is absent:** + +``` +💡 **Recommended: diffusers Baseline Comparison** + +Adding a diffusers comparison would strengthen this PR: + +- diffusers generation script (same prompt, same resolution/steps) +- diffusers sample output +- diffusers e2e latency vs vLLM-Omni latency +- diffusers peak VRAM vs vLLM-Omni VRAM +``` + +--- + +### Dimension 2: Code Review + +```bash +# Detect leftover diffusers mixins +gh pr diff --repo vllm-project/vllm-omni \ + | grep '^\+' | grep -E 'DiffusionPipelineMixin|SchedulerMixin|ConfigMixin' + +# Check acceleration features +gh pr diff --repo vllm-project/vllm-omni \ + | grep -iE 'sequence_parallel|cfg_parallel|vae_patch_parallel|tensor_parallel|teacache|cache_dit' + +# Check memory optimizations +gh pr diff --repo vllm-project/vllm-omni \ + | grep -iE 'cpu_offload|offload_to_cpu|quantization|int8|fp8|bitsandbytes|vae_tiling' +``` + +**2.1 Inference modes** — both offline (`Omni`) and online (`vllm serve` / `AsyncOmni`) must be implemented. + +``` +🔴 Missing support. The model must work in both +Omni (offline) and vllm serve / AsyncOmni (online) modes before merging. +``` + +**2.2 Diffusers mixin cleanup** — flag any mixin still present in `+` lines. + +``` +🔴 Leftover diffusers mixin detected: `` in `:`. +Remove diffusers mixins and use vLLM-Omni's native abstractions instead. +``` + +**2.3 Acceleration features** — at least one of: sequence parallel, CFG parallel, VAE patch parallel, tensor parallel, TeaCache/step cache. + +``` +🔴 No acceleration feature detected. The model must support at least one of: +sequence parallel, CFG parallel, VAE patch parallel, tensor parallel, or step caching (TeaCache). +``` + +**2.4 Memory optimization** — at least one of: CPU offload, quantization (int8/fp8/bnb), VAE tiling. + +``` +🔴 No memory optimization feature detected. The model must support at least one of: +CPU offload (`--cpu-offload-gb`), quantization (int8/fp8/bnb), or VAE tiling. +``` + +--- + +### Dimension 3: Documentation + +```bash +gh pr view --repo vllm-project/vllm-omni \ + --json files --jq '.files[].path' | grep -E 'docs/|\.md$' +``` + +Required: model support table, feature support table, usage example doc (offline + online). + +``` +🔴 Documentation incomplete: +- [ ] Model support table not updated (docs/models/supported_models.md or equivalent) +- [ ] Feature support table not updated +- [ ] Usage example doc missing or not updated for +``` + +--- + +### Dimension 4: Test Coverage + +```bash +gh pr view --repo vllm-project/vllm-omni \ + --json files --jq '.files[].path' | grep -E '^tests/' +``` + +Required: e2e online serving test. Recommended: offline inference test. Required when 2+ features: combined feature test. + +``` +🔴 Missing e2e online serving test in `tests/e2e/online_serving/`. +Please add a test that: +1. Starts `vllm serve --omni` +2. Sends a generation request via the API +3. Asserts the response contains a valid image / video / audio output +``` + +``` +⚠️ Multiple features are implemented (e.g., + ) but no combined +feature test is present. Please add a test (or extend the e2e test) that enables both +features together and asserts output validity + reports latency + VRAM. +``` + +> See [Diffusion Checklist](references/diffusion-checklist.md) for full per-item criteria and the Quick Red Flags summary. + +--- + +## References + +- [Blocker Patterns](references/blocker-patterns.md) - Anti-patterns that block approval with code examples +- [Review Routing](references/review-routing.md) - Prefix mapping, multi-skill routing, hardware detection +- [Review Execution](references/review-execution.md) - Gate checks, commands, comment budget, review phrasing +- [Common Pitfalls](references/pitfalls.md) - MRO issues, connector state, async differences +- [Architecture](references/architecture.md) - System overview and critical paths +- [Code Patterns](references/code-patterns.md) - Async, distributed, cache, validation, error handling patterns +- [Diffusion PR Requirements](references/diffusion-pr-requirements.md) - PR body requirements for diffusion model contributions diff --git a/.claude/skills/review-pr/references/architecture.md b/.claude/skills/review-pr/references/architecture.md new file mode 100644 index 000000000..391382186 --- /dev/null +++ b/.claude/skills/review-pr/references/architecture.md @@ -0,0 +1,192 @@ +# vLLM-Omni Architecture + +## Overview + +vLLM-Omni extends vLLM for omni-modal (multi-modal) model serving, supporting text, image, video, and audio generation. It provides a unified inference engine with OpenAI-compatible APIs for both LLM and diffusion models. + +**Key Differentiators from vLLM:** +- Multi-stage pipeline architecture (e.g., Thinker → Talker → Code2Wav) +- Diffusion Transformer (DiT) model support +- Inter-stage communication via shared memory connectors +- Stage-level concurrency control + +--- + +## Five-Layer Architecture + +``` +┌─────────────────────────────────────────────────────┐ +│ 1. User Interface Layer │ +│ OpenAI-compatible API, CLI, Python SDK │ +├─────────────────────────────────────────────────────┤ +│ 2. Orchestration Layer │ +│ Omni, AsyncOmni - coordinates stages │ +├─────────────────────────────────────────────────────┤ +│ 3. Engine Layer │ +│ ModelExecutor, Scheduler, KV cache management │ +├─────────────────────────────────────────────────────┤ +│ 4. Execution Layer │ +│ Workers (GPUARWorker, GPUGenerationWorker) │ +├─────────────────────────────────────────────────────┤ +│ 5. Model Layer │ +│ OmniLLM, OmniDiffusion, model implementations │ +└─────────────────────────────────────────────────────┘ +``` + +--- + +## Key Directories + +| Directory | Purpose | Critical? | Review Focus | +|-----------|---------|-----------|--------------| +| `vllm_omni/entrypoints/` | API server, CLI | High | Input validation, error handling | +| `vllm_omni/engine/` | Engine, scheduler | **Critical** | Concurrency, state management | +| `vllm_omni/model_executor/` | Model execution | **Critical** | Weight loading, memory | +| `vllm_omni/diffusion/` | Diffusion support | High | Latent cache, generation | +| `vllm_omni/connectors/` | Inter-stage IPC | High | Shared memory, cleanup | +| `vllm_omni/stages/` | Stage definitions | High | Lifecycle, state | +| `vllm_omni/config/` | Configuration | Medium | Validation, defaults | +| `vllm_omni/utils/` | Utilities | Low | Test coverage | + +--- + +## Core Components + +### Entry Points + +**Synchronous:** `Omni` class +```python +from vllm_omni import Omni +llm = Omni(model="Qwen/Qwen2.5-Omni-7B") +outputs = llm.generate("Hello") +``` + +**Asynchronous:** `AsyncOmni` class +```python +from vllm_omni import AsyncOmni +llm = AsyncOmni(model="Qwen/Qwen2.5-Omni-7B") +outputs = await llm.generate("Hello") +``` + +### Stage Types + +| Type | Worker Classes | Use Case | +|------|---------------|----------| +| `llm` | `GPUARWorker`, `GPUGenerationWorker` | Text generation, multimodal understanding | +| `diffusion` | Diffusion-specific workers | Image/video generation | +| `audio` | Audio workers | TTS, audio synthesis | + +### Connectors + +| Connector | Use Case | Performance | +|-----------|----------|-------------| +| `OmniShmConnector` | Same-machine inter-process | Fastest | +| `OmniZmqConnector` | Distributed/multi-node | Network-capable | + +--- + +## Multi-Stage Pipeline + +### Example: Qwen-Omni Audio Generation + +``` +┌─────────┐ ┌─────────┐ ┌───────────┐ +│ Thinker │ ──▶ │ Talker │ ──▶ │ Code2Wav │ +│ (LLM) │ │ (LLM) │ │ (Audio) │ +└─────────┘ └─────────┘ └───────────┘ + │ │ │ + Text/Audio Text Code Audio Output + Understanding Generation Synthesis +``` + +### Configuration (YAML) + +```yaml +stages: + - name: thinker + type: llm + - name: talker + type: llm + - name: code2wav + type: audio +``` + +--- + +## Data Flow + +``` +Request → API Layer → AsyncOmni/Omni + │ + ▼ + Stage Coordinator + (YAML config) + │ + ┌──────────────┼──────────────┐ + ▼ ▼ ▼ + Stage 1 Stage 2 Stage N + (Worker) (Worker) (Worker) + │ │ │ + └──────────────┴──────────────┘ + │ + OmniConnector + (Shm/Zmq) + │ + ▼ + Response +``` + +--- + +## Memory Management + +### KV Cache (LLM Stages) +- Paged attention inherited from vLLM +- Block-based memory allocation +- Prefix caching for repeated contexts + +### Diffusion Latent Cache +- Intermediate latent storage +- Timestep-based scheduling +- Memory-pressure aware + +--- + +## Supported Models + +### Omni-Modal Models +- Qwen3-Omni +- Qwen2.5-Omni +- Qwen3-TTS (CustomVoice, VoiceDesign, Base) +- MiMo-Audio +- Bagel + +### Diffusion Models +- Z-Image +- Qwen-Image +- Wan2.2 (video) +- FLUX + +--- + +## Review Considerations + +### Critical Paths (High Impact) +- `vllm_omni/engine/` — scheduler changes affect all workloads +- `vllm_omni/model_executor/` — model loading bugs break inference +- `vllm_omni/connectors/` — communication bugs cause hangs/crashes + +### High-Risk Patterns +1. **Stage coordination changes** — can break multi-stage pipelines +2. **Memory management in connectors** — shared memory leaks +3. **Worker lifecycle changes** — affect tensor parallelism +4. **Input validation gaps** — engine crashes instead of 400 errors + +### Testing Requirements +| Component | Test Requirement | +|-----------|------------------| +| LLM stages | Actual model inference | +| Diffusion stages | Generation quality | +| Connectors | Load testing, memory leak check | +| Multi-stage | End-to-end pipeline | +| API endpoints | Input validation, error responses | diff --git a/.claude/skills/review-pr/references/blocker-patterns.md b/.claude/skills/review-pr/references/blocker-patterns.md new file mode 100644 index 000000000..11fe32470 --- /dev/null +++ b/.claude/skills/review-pr/references/blocker-patterns.md @@ -0,0 +1,525 @@ +# Blocker Patterns Library + +Each pattern includes: the anti-pattern, why it's dangerous, and required fix. + +Cross-references point to [code-patterns.md](code-patterns.md) for "how to fix" guidance. + +## Exception Handling + +### Pattern: Silent Swallow +```python +# BLOCKER: Silent exception swallow +try: + result = risky_operation() +except: + pass # or: except Exception: pass +``` + +**Why dangerous:** Hides all failures, makes debugging impossible. + +**Required fix:** At minimum, log the exception. Better: catch specific exceptions. + +**Cross-ref:** See [Error Handling Pattern](code-patterns.md#error-handling-pattern) for proper exception handling with logging. + +--- + +### Pattern: Broad Catch with Continue +```python +for item in items: + try: + process(item) + except Exception: + continue # silently skips +``` + +**Why dangerous:** Partial failure with no visibility. + +**Required fix:** Log failure, increment counter, or fail fast. + +**Cross-ref:** See [Error Handling Pattern](code-patterns.md#error-handling-pattern) for proper exception handling with logging. + +--- + +### Pattern: Uninitialized Variable +```python +result = obj.method().nested.value # potential AttributeError +``` + +**Why dangerous:** Crashes with `AttributeError` if any step returns `None`. + +**Required fix:** Add explicit None guards before accessing attributes. + +```python +if obj is not None and obj.method() is not None: + result = obj.method().nested.value +``` + +**Cross-ref:** See [None Safety](code-patterns.md#input-validation-pattern) for guard patterns. + +--- + +## Resource Management + +### Pattern: Unclosed Resource +```python +f = open(path) # no 'with' statement +content = f.read() +# f never closed +``` + +**Why dangerous:** Resource leaks, especially in long-running servers instances. + +**Required fix:** Use context managers (`with open(...) as f:`). + +**Cross-ref:** See [Connector Communication](code-patterns.md#connector-communication) for proper connector usage with context managers. + +--- + +### Pattern: Shared Mutable State +```python +shared_list = [] # modified by concurrent coroutines +async def append(item): + shared_list.append(item) +``` + +**Why dangerous:** Race conditions, corrupted state. + +**Required fix:** Use `asyncio.Lock` or thread-safe structures. + +**Cross-ref:** See [Distributed Execution Patterns](code-patterns.md#distributed-execution-patterns) for thread safety alternatives. + +--- + +## None Safety + +### Pattern: Unchecked Chain +```python +result = obj.method().nested.value # no None checks +``` + +**Why dangerous:** `AttributeError` if any step returns `None`. + +**Required fix:** Add explicit None guards before each access, or use `getattr()` with defaults. + +```python +# Option 1: Explicit None checks +if obj is not None: + method_result = obj.method() + if method_result is not None: + result = method_result.nested.value + +# Option 2: getattr with default +result = getattr(getattr(obj, "method", lambda: None)(), "nested", None) +``` + +**Cross-ref:** See [Input Validation Pattern](code-patterns.md#input-validation-pattern) for early validation guidance. + +--- + +### Pattern: Missing None Check +```python +def process(data): + if data is None: + return None + # Missing None check! + result = process(data) # Silent failure + return True +``` + +**Why dangerous:** Hides bugs, makes debugging impossible. + +**Required fix:** Add explicit None check and log a warning. Better: `return None`. + +**Cross-ref:** See [Input Validation Pattern](code-patterns.md#input-validation-pattern) for validation guidance. + +--- + +## Breaking Change Detection + +### Pattern: Function Signature Change +```python +# BLOCKER: No backward compatibility +# Old signature: def get_user(user_id: int): +# New signature adds a non-optional parameter: +def get_user(user_id: int, include_profile: bool): # Breaking change! + pass +``` + +**Why dangerous:** Breaks existing code that depends on this function. + +**Required fix:** Add deprecation path or maintain backward compatibility for 1-2 releases. Document migration. + +**Cross-ref:** See [API Endpoints](code-patterns.md#connector-communication) for API compatibility guidance. + +--- + +### Pattern: Removed Public API +```python +# BLOCKER: No deprecation path +def remove_feature(name): + pass +``` + +**Why dangerous:** Breaks existing code that depends on this function. +**required fix:** If removal is necessary, provide replacement. Otherwise, document breaking change and deprecation timeline. +**Cross-ref:** See [API Endpoints](code-patterns.md#connector-communication) for OpenAI compatibility guidance +--- + +### Pattern: Changed Default +```python +class Config: + def __init__(self): + self.default_value = "on" # Was "off" - breaking change! +``` + +**Why dangerous:** Breaks existing code that depends on this default. + +**Required fix:** Document the change clearly. If intentional, provide rollback code or migration script. + +**Cross-ref:** See [API Endpoints](code-patterns.md#connector-communication) for API compatibility guidance. + +--- + +## Memory Management + +### Pattern: Unbounded Latent Cache +```python +class DiffusionModel: + def generate(self, prompt): + self.latent_cache.append(create_latents()) # Never cleared! + return decode(self.latent_cache[-1]) +``` + +**Why dangerous:** Memory leaks accumulate across generations. +**required fix:** Explicitly clear latent cache after generation. Add memory-pressure handling. +**Cross-ref:** See [Memory Management in Diffusion](pitfalls.md#memory-management-in-diffusion) for latent cache lifecycle guidance +--- + +## Async Safety + +### Pattern: Blocking Call in Async +```python +# BLOCKER: Blocking call blocks event loop +async def process(self, request): + result = blocking_call() # Blocks event loop! + return result +``` + +**Why dangerous:** Blocks event loop, prevents concurrent requests. +**required fix:** Use `asyncio.to_thread()` or await pattern. +**Cross-ref:** See [Async Function Complexity](code-patterns.md#async-function-complexity) for async patterns +--- + +### Pattern: Hardcoded Timeout +```python +def connect(url): + response = requests.get(url, timeout=30.0) # Hardcoded - no flexibility! +``` + +**Why dangerous:** No flexibility, hides issues in testing. + +**Required fix:** Make timeout configurable or add timeout parameter to CLI flags. + +**Cross-ref:** See [Connector Communication](code-patterns.md#connector-communication) for proper connector usage. + +--- + +### Pattern: Hardcoded Retry Count +```python +MAX_RETRIES = 3 # No justification, no backoff + +for attempt in range(MAX_RETRIES): + try: + response = requests.get(url, timeout=5) + except TimeoutError: + pass # Silent retry, no logging +``` + +**Why dangerous:** Can cause infinite retry loops or delays without visibility. + +**Required fix:** Use exponential backoff with configurable max retries or circuit breaker pattern. + +**Cross-ref:** See [Stage Lifecycle](code-patterns.md#stage-lifecycle) for lifecycle management. + +--- + +### Pattern: Sequential Async in Loop +```python +# BLOCKER: No parallelism +results = [] +for r in requests: + result = await _process_single(r) # Sequential, slow + return results +``` + +**Why dangerous:** Poor throughput, no parallelism benefits. + +**Required fix:** Use `asyncio.gather()` with `return_exceptions=True` for parallel execution. + +**Cross-ref:** See [Async Function Complexity](code-patterns.md#async-function-complexity) for proper async patterns. + +--- + +### Pattern: Device-Specific Code +```python +# BLOCKER: No non-CUDA fallback +if torch.cuda.is_available(): + x = x.to(device) +else: + x.cpu() # Missing non-CUDA path! +``` + +**Why dangerous:** Crashes on non-CUDA systems. + +**Required fix:** Check `torch.cuda.is_available()` first and use appropriate fallback. + +**Cross-ref:** See [Distributed Execution Patterns](code-patterns.md#distributed-execution-patterns) for distributed patterns. + +--- + +## API Boundaries + +### Pattern: Unsafe Parameter Passthrough +```python +# BLOCKER: No validation before passing to API +user_input = request.json.get("model") +response = model.generate(request.json["prompt"]) +``` + +**Why dangerous:** Arbitrary code execution, model injection. + +**Required fix:** Validate all user inputs before passing to model. + +**Cross-ref:** See [Input Validation Pattern](code-patterns.md#input-validation-pattern) for validation guidance. + +--- + +### Pattern: Missing Response Validation +```python +# BLOCKER: No validation after calling API +async def create_item(self, request): + # Missing: validation + # Could return invalid responses + return {"error": "Internal server error"} +``` + +**Why dangerous:** API contract violations. + +**Required fix:** Validate responses match schema and handle errors explicitly. + +**Cross-ref:** See [Error Handling Pattern](code-patterns.md#error-handling-pattern) for proper error handling patterns. + +--- + +## Security + +### Pattern: Hardcoded Secrets +```python +# BLOCKER: Hardcoded credentials +API_KEY = "sk-abc123" +DATABASE_PASSWORD = "secret123" +``` + +**Why dangerous:** Credential exposure in logs, git history. + +**Required fix:** Use environment variables or secret management systems. + +**Cross-ref:** See [Logging Pattern](code-patterns.md#logging-pattern) for proper logging practices. + +--- + +### Pattern: Insecure Deserialization +```python +import pickle # DANGEROUS: loading untrusted data +data = pickle.loads(user_input) +``` + +**Why dangerous:** Remote code execution (RCE). + +**Required fix:** Never use `pickle.loads()` on untrusted data. Use safe deserialization libraries. + +**Cross-ref:** See [Error Handling Pattern](code-patterns.md#error-handling-pattern) for safe error handling patterns. + +--- + +### Pattern: SQL Injection +```python +# BLOCKER: No parameterized queries +cursor.execute(user_input) # Direct string interpolation +``` + +**Why dangerous:** Data breach. + +**Required fix:** Use parameterized queries (prepared statements, ORM). + +**Cross-ref:** See [Input Validation Pattern](code-patterns.md#input-validation-pattern) for validation guidance. + +--- + +### Pattern: Unsafe String Formatting with User Input +```python +# BLOCKER: User-controlled format string enables code injection +user_template = request.args.get("template") # e.g., "{user.__class__.__mro__}" +msg = user_template.format(user=current_user) # Can leak internal state! + +# BLOCKER: eval/exec with user input +result = eval(user_expression) # Remote code execution! +query = f"SELECT * FROM {user_table}" # SQL injection via string formatting +``` + +**Why dangerous:** `str.format()` with user-controlled templates can access object attributes. `eval()`/`exec()` execute arbitrary code. String formatting into SQL/HTML enables injection. + +**Required fix:** +- For templates: use sandboxed template engines (Jinja2 with autoescape) or whitelist allowed placeholders +- For SQL: use parameterized queries, never string interpolation +- For code execution: never pass user input to `eval()`/`exec()` + +**Note:** Normal f-strings like `f"Hello, {name}!"` are safe - they don't execute code, just interpolate. + +**Cross-ref:** See [Input Validation Pattern](code-patterns.md#input-validation-pattern) for validation guidance. + +--- + +## Test Coverage + +### Pattern: Missing Regression Test +```python +# BLOCKER: Bug fix without regression test +def fix_bug(): + pass # Bug fixed +``` + +**Why dangerous:** Bug may resurface in future PRs. + +**Required fix:** Add a regression test that reproduces the original bug. + +**Cross-ref:** See [Test Coverage Requirements](code-patterns.md#test-coverage-requirements) for test coverage requirements. + +--- + +### Pattern: Weak Assertions +```python +# BLOCKER: Assertion too weak +def test_result(): + result = compute() + assertTrue(result) # Passes even if result is wrong! +``` + +**Why dangerous:** False confidence in test results. + +**Required fix:** Use meaningful assertions that verify actual behavior. + +**Cross-ref:** See [Test Coverage Requirements](code-patterns.md#test-coverage-requirements) for test coverage requirements. + +--- + +### Pattern: Mock Mismatch +```python +# Production +class Model(nn.Module, Mixin): + pass + +# Test mock - different Mro +class MockModel(Mixin): # Missing nn.Module + pass +``` + +**Why dangerous:** Tests pass but hide real bugs. + +**Required fix:** Ensure mocks match production inheritance hierarchy. + +**Cross-ref:** See [Test Mock Mismatches](pitfalls.md#test-mock-mismatches) for mock testing guidance. + +--- + +### Pattern: Only Happy Path Test +```python +# BLOCKER: No edge case tests +def process(self, data): + result = self._process(data) + # Happy path only + return result +``` + +**Why dangerous:** Edge cases not tested, bugs only surface in production. + +**Required fix:** Add tests for error handling, edge cases (empty, null, boundary). + +**Cross-ref:** See [Test Coverage Requirements](code-patterns.md#test-coverage-requirements) for test coverage requirements. + +--- + +### Pattern: No Performance Benchmark +```python +# BLOCKER: Performance optimization without benchmark data +def optimize(): + pass # Faster! + return data +``` + +**Why dangerous:** Claims without evidence. + +**Required fix:** Run benchmarks with `pytest` or manual timing, document results. + +**Cross-ref:** See [Test Coverage Requirements](code-patterns.md#test-coverage-requirements) for test coverage requirements. + +--- + +### Pattern: Missing Memory Measurement +```python +# BLOCKER: Memory optimization without memory measurement +pass +``` + +**Why dangerous:** Cannot verify memory savings. + +**Required fix:** Use memory profiling tools and report peak and delta. + +**Cross-ref:** See [Memory Management in Diffusion](pitfalls.md#memory-management-in-diffusion) for latent cache lifecycle guidance. + +--- + +### Pattern: No Distributed Test +```python +# BLOCKER: Distributed changes without distributed test +def test_distributed(self): + pass +``` + +**Why dangerous:** Distributed bugs only surface in distributed environments, not unit tests. + +**Required fix:** Test in distributed mode with `torchrun`. + +**Cross-ref:** See [Tensor Parallelism Edge Cases](pitfalls.md#tensor-parallelism-edge-cases) for distributed testing guidance. + +--- + +## Documentation + +### Pattern: Missing Migration Guide +```python +# BLOCKER: Breaking change without migration guide +def migrate_config(): + pass +``` + +**Why dangerous:** Users unable to upgrade. + +**Required fix:** Document breaking changes in RELEASE_NOTES.md. Provide migration instructions. + +**Cross-ref:** See [API Endpoints](code-patterns.md#connector-communication) for OpenAI compatibility guidance. + +--- + +### Pattern: Missing Usage Examples +```python +# BLOCKER: New feature without usage examples +def add_feature(): + pass +``` + +**Why dangerous:** Users can't understand how to use the feature. + +**Required fix:** Add usage examples, docstrings. + +**Cross-ref:** See [Comment Style](review-execution.md#comment-style) for review comment guidance. diff --git a/.claude/skills/review-pr/references/code-patterns.md b/.claude/skills/review-pr/references/code-patterns.md new file mode 100644 index 000000000..e79517990 --- /dev/null +++ b/.claude/skills/review-pr/references/code-patterns.md @@ -0,0 +1,234 @@ +# Code Patterns in vLLM-Omni + +## Async Function Complexity + +**Guidelines:** +- Each `await` is a potential failure point and state transition +- Consider splitting long async chains into smaller helpers +- Watch for race conditions in complex async flows +- Use `return_exceptions=True` for parallel operations + +**Good:** +```python +async def process_batch(requests: list[Request]) -> list[Response]: + validated = [_validate_request(r) for r in requests] + results = await asyncio.gather( + *[_process_single(r) for r in validated], + return_exceptions=True + ) + return [_handle_result(r) for r in results] +``` + +**Bad:** +```python +async def process_batch(requests: list[Request]) -> list[Response]: + results = [] + for r in requests: + results.append(await _process_single(r)) # Sequential, no error handling + return results +``` + +--- + +## Distributed Execution Patterns + +**When complexity is justified:** +- Tensor parallelism synchronization across workers +- Pipeline parallelism stage coordination +- Distributed KV cache management +- Multi-node communication patterns + +**Critical questions:** +- Is distributed complexity isolated from business logic? +- Are distributed failure modes handled? +- Is there a clear fallback for single-device execution? + +**Pattern:** +```python +def get_world_size() -> int: + """Returns 1 if not in distributed mode.""" + if not dist.is_initialized(): + return 1 + return dist.get_world_size() +``` + +--- + +## KV Cache Management + +**Guidelines:** +- Ensure clear separation between allocation, update, and cleanup +- Document invariants and assumptions +- Use type system to enforce valid states +- Always handle cleanup in error paths + +**Pattern:** +```python +class KVCache: + def allocate(self, num_blocks: int) -> None: + assert self._state == CacheState.UNINITIALIZED + self._blocks = [Block() for _ in range(num_blocks)] + self._state = CacheState.ALLOCATED + + def update(self, block_id: int, kv_data: Tensor) -> None: + assert self._state == CacheState.ALLOCATED + self._blocks[block_id].update(kv_data) + + def free(self) -> None: + self._blocks = [] + self._state = CacheState.UNINITIALIZED +``` + +--- + +## Input Validation Pattern + +**Guidelines:** +- Validate early (API layer, not engine) +- Return actionable error messages +- Use clean HTTP status codes (400 for client errors) + +**Pattern:** +```python +def _validate_request(self, request: Request) -> str | None: + """Returns error message or None if valid.""" + if not request.input: + return "Input cannot be empty" + + if request.param and request.param not in VALID_PARAMS: + return f"Invalid param '{request.param}'. Valid: {VALID_PARAMS}" + + return None + +def handle_request(self, request: Request) -> Response: + error = self._validate_request(request) + if error: + return Response(status_code=400, content={"error": error}) + return self._process(request) +``` + +--- + +## Connector Communication + +**Guidelines:** +- Use context managers for resource cleanup +- Handle timeouts explicitly +- Implement proper error propagation + +**Pattern:** +```python +async def send_to_next_stage(self, data: Tensor) -> None: + try: + async with self.connector.acquire(timeout=30.0) as channel: + await channel.send(data) + except TimeoutError: + logger.error("Stage communication timeout") + raise StageError("Downstream stage unavailable") + except Exception as e: + logger.error(f"Stage communication failed: {e}") + raise +``` + +--- + +## Stage Lifecycle + +**Guidelines:** +- Stages are configured at init time +- Runtime reconfiguration requires full teardown +- State must be properly managed across stage boundaries + +**Pattern:** +```python +class Stage: + def __init__(self, config: StageConfig): + self._validate_config(config) + self.config = config + self._state = StageState.INITIALIZED + + async def start(self) -> None: + assert self._state == StageState.INITIALIZED + await self._allocate_resources() + self._state = StageState.RUNNING + + async def stop(self) -> None: + if self._state == StageState.RUNNING: + await self._release_resources() + self._state = StageState.STOPPED +``` + +--- + +## Test Coverage Requirements + +| Code Type | Requirement | +|-----------|-------------| +| New features | Happy path + edge cases + error handling | +| Bug fixes | Regression test + edge cases around fix | +| Performance | Benchmarks with before/after measurements | +| Distributed | Must test in distributed mode | +| Quantization | Memory savings + quality impact measured | +| API endpoints | Input validation + error responses | + +--- + +## Code Quality Thresholds + +| Metric | Threshold | Action | +|--------|-----------|--------| +| Function length | 100+ lines | Review for split opportunities | +| Complexity | 11+ | Requires refactoring | +| Nesting depth | 4+ levels | Requires refactoring | +| Parameters | 7+ | Consider parameter object | + +--- + +## Error Handling Pattern + +**Guidelines:** +- Use specific exception types +- Include actionable context in messages +- Don't silently swallow errors + +**Pattern:** +```python +class ValidationError(Exception): + """Raised when input validation fails.""" + pass + +class EngineError(Exception): + """Raised when engine operation fails.""" + pass + +def process(self, request: Request) -> Response: + try: + return self._process_internal(request) + except ValidationError as e: + logger.warning(f"Validation failed: {e}") + return Response(status_code=400, content={"error": str(e)}) + except EngineError as e: + logger.error(f"Engine error: {e}") + return Response(status_code=500, content={"error": "Internal error"}) +``` + +--- + +## Logging Pattern + +**Guidelines:** +- Use appropriate log levels +- Include relevant context +- Avoid logging sensitive data + +```python +# Good +logger.info(f"Processing request {request_id} with model {model_name}") +logger.warning(f"Slow response time: {elapsed_ms}ms for {endpoint}") +logger.error(f"Failed to load model {model_path}: {e}") + +# Bad +logger.info("Processing request") # No context +print(f"Error: {e}") # Wrong logging method +logger.debug(f"User token: {token}") # Sensitive data +``` diff --git a/.claude/skills/review-pr/references/diffusion-checklist.md b/.claude/skills/review-pr/references/diffusion-checklist.md new file mode 100644 index 000000000..3405cf2d7 --- /dev/null +++ b/.claude/skills/review-pr/references/diffusion-checklist.md @@ -0,0 +1,252 @@ +# Diffusion Model Contribution Review Checklist + +Use this checklist as a structured pass after reading the PR diff and body. Each item maps to a dimension in the main SKILL.md workflow. + +--- + +## Dimension 1: PR Body Completeness + +### 1.1 Required Evidence (Table A) + +| Item | What to Check | Missing → Action | +|------|---------------|------------------| +| **vLLM-Omni generation script** | Runnable Python or bash snippet using `Omni` / `vllm serve` that produces output | Request script | +| **Generated sample outputs** | At least 1 image / video / audio sample attached or linked | Request sample | +| **vLLM-Omni e2e latency** | Wall-clock time from request to output, with GPU model, count, resolution, steps | Request measurement | +| **vLLM-Omni VRAM usage** | Peak VRAM in GB during generation, with resolution / steps | Request measurement | + +- [ ] **vLLM-Omni generation script present** + - Must use `Omni(model=...)` for offline, or `vllm serve ... --omni` for online + - Script must be runnable (no pseudocode, no placeholder ``) + - If only offline or only online is shown, note which is missing + +- [ ] **Sample output attached or linked** + - Image PR: at least 1 image (PNG/JPEG), ideally multiple prompts + - Video PR: at least 1 video clip or animated GIF + - Audio PR: audio file or spectrogram + waveform visualization + - Resolution and generation parameters must be visible (caption or table) + +- [ ] **vLLM-Omni e2e latency recorded** + - Wall-clock time from request submission to final output received + - Must include: GPU model, GPU count, resolution (H×W), `num_inference_steps` + - Single measurement is acceptable; batch=1 is the baseline + +- [ ] **vLLM-Omni peak VRAM recorded** + - Peak GPU memory in GB + - Measurement method noted (e.g., `torch.cuda.max_memory_allocated()`, `nvidia-smi`) + - Must match the hardware spec reported with latency + +### 1.2 Recommended Evidence (Table B) + +| Item | Why It Matters | +|------|----------------| +| **diffusers generation script** | Reproducible baseline for quality and speed comparison | +| **diffusers sample outputs** | Side-by-side quality comparison demonstrates parity | +| **diffusers e2e latency** | Quantifies vLLM-Omni speedup relative to reference | +| **diffusers VRAM usage** | Quantifies memory reduction or overhead | + +- [ ] **diffusers generation script** + - Same prompt, same resolution, same number of steps as vLLM-Omni script + - Uses `diffusers` library directly (not wrapped in vllm-omni) + +- [ ] **diffusers sample output** + - Side-by-side or clearly labeled comparison with vLLM-Omni output + - Same prompt used for both + +- [ ] **diffusers e2e latency** + - Recorded under same hardware conditions as vLLM-Omni measurement + - Speedup ratio (vLLM-Omni latency / diffusers latency) stated or calculable + +- [ ] **diffusers VRAM usage** + - Peak VRAM under same conditions + - Delta (VRAM_vllm_omni − VRAM_diffusers) highlighted if significant + +--- + +## Dimension 2: Code Checks + +### 2.1–2.2 Inference Mode Coverage (both required) + +| Check | Pass Condition | Flag | +|-------|----------------|------| +| **Offline inference** | `Omni` / `OmniLLM` integration exists; model can be instantiated and called | Missing offline path | +| **Online serving** | `vllm serve` or `AsyncOmni` handles the model; API routes return correct responses | Missing online path | + +### 2.1 Offline Inference + +- [ ] `Omni` or `OmniLLM` can instantiate the model (`model=""`) +- [ ] `.generate()` returns a valid output object with images/video/audio fields +- [ ] Default stage config YAML exists under `vllm_omni/configs/` or equivalent +- [ ] Model is registered in the model registry (import path resolvable) + +### 2.2 Online Serving + +- [ ] `AsyncOmni` or API server route handles the model +- [ ] `vllm serve --omni` startup does not error +- [ ] `/v1/chat/completions` or appropriate endpoint returns correct response schema +- [ ] Input validation rejects malformed requests with 400, not engine crash + +### 2.3 Diffusers Mixin Cleanup + +- [ ] No `DiffusionPipelineMixin` in added/modified code +- [ ] No `SchedulerMixin` in added/modified code +- [ ] No `ConfigMixin` (diffusers) in added/modified code +- [ ] No direct `diffusers.pipelines.*` import in production model code + - Exception: `diffusers` may appear in test files or benchmark scripts + +**Detection command:** +```bash +gh pr diff --repo vllm-project/vllm-omni \ + | grep -E '^\+' \ + | grep -E 'DiffusionPipelineMixin|SchedulerMixin|ConfigMixin|from diffusers\.pipelines' +``` + +### 2.4 Acceleration Features + +At least one of the following must be implemented and demonstrably working: + +| Feature | Detection Pattern | Notes | +|---------|------------------|-------| +| **Sequence Parallel** | `sequence_parallel`, `sp_group`, `ring_attn` | For transformer attention | +| **CFG Parallel** | `cfg_parallel`, `cfg_group`, dual-batch guidance | Splits positive/negative across devices | +| **VAE Patch Parallel** | `vae_patch_parallel`, `patch_parallel` | For high-res VAE decoding | +| **Tensor Parallel** | `tp_group`, `ColumnParallelLinear`, `RowParallelLinear` | Linear layer sharding | +| **TeaCache / Step Cache** | `teacache`, `cache_dit`, `cache_interval` | Skip redundant denoising steps | + +- [ ] At least one acceleration feature implemented +- [ ] Feature can be enabled via config or CLI flag (not hardcoded) +- [ ] PR body or docs show the speedup from enabling the feature + +### 2.5 Memory Optimization Features + +At least one of the following must be implemented: + +| Feature | Detection Pattern | Notes | +|---------|------------------|-------| +| **CPU Offload** | `cpu_offload`, `offload_to_cpu`, `--enable-cpu-offload` | Move weights to CPU when idle | +| **Quantization** | `quantization`, `int8`, `fp8`, `bnb`, `bitsandbytes` | Reduced precision weights | +| **VAE Tiling** | `vae_tiling`, `tile_size`, `tiled_decode` | Decode VAE in spatial tiles | + +- [ ] At least one memory optimization feature implemented +- [ ] Feature can be enabled via config or CLI flag +- [ ] PR body or docs show VRAM reduction from enabling the feature + +--- + +## Dimension 3: Documentation + +**Find doc files:** +```bash +gh pr view --repo vllm-project/vllm-omni \ + --json files --jq '.files[].path' | grep -E '\.md$|docs/' +``` + +| Doc Artifact | Required | What to Check | +|--------------|----------|---------------| +| **Model support table** | Yes | Row added: model name, architecture, HF model ID, modality, min VRAM | +| **Feature support table** | Yes | Row showing which acceleration and memory features are supported | +| **Usage example doc** | Yes | Runnable offline + online example for the new model | +| **Feature compatibility table** | Optional | If multiple features: matrix showing validated combinations | + +### 3.1 Model Support Table + +- [ ] Table updated in `docs/models/supported_models.md` (or equivalent) +- [ ] New row contains: Model Name, Architecture, HF Model ID, Modality, Min VRAM +- [ ] Modality correctly labeled (text-to-image / text-to-video / image-editing / etc.) + +### 3.2 Feature Support Table + +- [ ] Table updated showing which features apply to the new model +- [ ] Each column maps to a specific feature (Seq Parallel, CFG Parallel, VAE Patch, TP, CPU Offload, Quantization) +- [ ] Cells use consistent notation (✅ supported / ❌ not supported / 🔄 planned) + +### 3.3 Feature Compatibility Table (Optional) + +Only checked when model supports multiple features: + +- [ ] Compatibility matrix exists showing which feature combinations are validated +- [ ] Untested combinations marked clearly (e.g., `❓`) + +### 3.4 Usage Example Doc + +- [ ] Example doc added or updated for the new model +- [ ] Offline inference example present (with `Omni`) +- [ ] Online serving example present (with `vllm serve` + `curl`) +- [ ] At least one acceleration feature example shown +- [ ] At least one memory optimization example shown +- [ ] Generation parameters documented (resolution, steps, guidance scale) + +--- + +## Dimension 4: Test Coverage + +**Find test files:** +```bash +gh pr view --repo vllm-project/vllm-omni \ + --json files --jq '.files[].path' | grep '^tests/' +``` + +| Test Type | Location | Required | What to Verify | +|-----------|----------|----------|----------------| +| **e2e online serving test** | `tests/e2e/online_serving/` | Yes | Start server, send request, assert output shape / non-null | +| **Offline inference test** | `tests/` or `tests/models/` | No (if e2e test exists) | Instantiate `Omni`, call `.generate()`, assert output | +| **Acceleration / memory feature test** | Alongside above | Recommended | Enable each supported feature, verify output and speed | +| **Combined feature test** | Alongside above | Required if 2+ features | Enable multiple features together, assert output + VRAM + latency | + +### 4.1 e2e Online Serving Test (Required) + +Location: `tests/e2e/online_serving/test_.py` + +- [ ] Test file exists in `tests/e2e/online_serving/` +- [ ] Test starts the server (or uses a fixture that does) +- [ ] Test sends a generation request to the API endpoint +- [ ] Test asserts response is non-null and output has correct shape/type +- [ ] Test includes at least one functional scenario (basic generation) +- [ ] Test includes at least one negative case (invalid input → 400 error) + +### 4.2 Offline Inference Test (Recommended; required only when no e2e test is present) + +- [ ] Test instantiates `Omni(model="")` +- [ ] Test calls `.generate()` with at least one prompt +- [ ] Test asserts output is a valid image/video/audio +- [ ] Test covers at least one edge case (empty prompt, special characters, max resolution) + +### 4.3 Acceleration and Memory Feature Tests (Recommended) + +- [ ] Test enables each acceleration feature and asserts output validity +- [ ] Test enables each memory optimization and asserts output validity + VRAM within budget + +### 4.4 Combined Feature Test (Required when 2+ features implemented) + +Only checked when the PR implements **two or more** acceleration or memory features: + +- [ ] Test (or an extended e2e test case) enables multiple features simultaneously +- [ ] Output under combined config is asserted as valid (quality preserved) +- [ ] Latency under combined config is reported +- [ ] VRAM under combined config is reported +- [ ] Any quality degradation under combination is explicitly documented + +--- + +## Quick Red Flags Summary + +Flag immediately and post a blocking comment if any of these are true: + +| Red Flag | Severity | Action | +|----------|---------|--------| +| No generation script in PR body | 🔴 Blocking | Request before review proceeds | +| No sample output in PR body | 🔴 Blocking | Request before review proceeds | +| No latency or VRAM measurements | 🔴 Blocking | Request before review proceeds | +| Online serving not implemented | 🔴 Blocking | Must implement both modes | +| Offline inference not implemented | 🔴 Blocking | Must implement both modes | +| Diffusers mixin still in code | 🔴 Blocking | Must remove before merge | +| No acceleration feature | 🔴 Blocking | Must implement at least one | +| No memory optimization | 🔴 Blocking | Must implement at least one | +| No e2e online serving test | 🔴 Blocking | Must add test | +| No offline inference test (and no e2e test) | 🔴 Blocking | Must add at least one test | +| No offline inference test (e2e test exists) | 💡 Recommended | Suggest, do not block | +| Model support table not updated | 🔴 Blocking | Must update docs | +| Feature support table not updated | 🔴 Blocking | Must update docs | +| No diffusers comparison | 💡 Recommended | Suggest, do not block | +| No combined feature test (2+ features) | ⚠️ Conditional | Required when 2+ features implemented | +| No usage example doc | 🔴 Blocking | Must add example | diff --git a/.claude/skills/review-pr/references/diffusion-pr-requirements.md b/.claude/skills/review-pr/references/diffusion-pr-requirements.md new file mode 100644 index 000000000..8bfbe3d7e --- /dev/null +++ b/.claude/skills/review-pr/references/diffusion-pr-requirements.md @@ -0,0 +1,257 @@ +# PR Body Requirements for Diffusion Model Contributions + +This document defines the exact structure and content expected in the PR description when adding a new diffusion model to vLLM-Omni. Reviewers use this as the source of truth when evaluating PR body completeness (Dimension 1). + +--- + +## Complete PR Body Template + +Below is the canonical template. PRs that follow this template pass Dimension 1 automatically. + +````markdown +## Summary + +[One paragraph: what model is being added, architecture type (DiT / AR+DiT / etc.), +and which capabilities are introduced (text-to-image / video / editing / etc.)] + +**Model:** `org/model-id` +**Architecture:** DiT / AR+DiT / other +**Modality:** text-to-image / text-to-video / image-editing + +--- + +## vLLM-Omni Generation + +### Offline Inference + +```python +from vllm_omni.entrypoints.omni import Omni + +omni = Omni(model="org/model-id") +outputs = omni.generate("a cup of coffee on a wooden table, photorealistic") +outputs[0].request_output[0].images[0].save("output.png") +``` + +### Online Serving + +```bash +# Start server +vllm serve org/model-id --omni --port 8091 + +# Generate +curl -s http://localhost:8091/v1/chat/completions \ + -H "Content-Type: application/json" \ + -d '{ + "messages": [{"role": "user", "content": "a cup of coffee on a wooden table"}], + "extra_body": {"height": 1024, "width": 1024, "num_inference_steps": 50, "seed": 42} + }' | jq -r '.choices[0].message.content[0].image_url.url' \ + | cut -d',' -f2 | base64 -d > output.png +``` + +### Sample Outputs + +| Prompt | Output | +|--------|--------| +| "a cup of coffee on a wooden table, photorealistic" | ![sample1](./assets/sample1.png) | +| "a futuristic city at night, neon lights" | ![sample2](./assets/sample2.png) | + +### Performance Measurements + +**Hardware:** 1× NVIDIA A100 80GB +**Resolution:** 1024×1024 +**Steps:** 50 + +| Metric | Value | +|--------|-------| +| e2e latency | X.X s | +| Peak VRAM | XX.X GB | + +--- + +## diffusers Baseline (Recommended) + +### Script + +```python +from diffusers import StableDiffusionXLPipeline +import torch + +pipe = StableDiffusionXLPipeline.from_pretrained("org/model-id", torch_dtype=torch.float16) +pipe = pipe.to("cuda") +image = pipe("a cup of coffee on a wooden table, photorealistic", + num_inference_steps=50, height=1024, width=1024).images[0] +image.save("output_diffusers.png") +``` + +### Comparison + +| | vLLM-Omni | diffusers | +|-|-----------|-----------| +| e2e latency | X.X s | X.X s | +| Peak VRAM | XX.X GB | XX.X GB | +| Sample | ![vllm_omni](./assets/sample_vllm.png) | ![diffusers](./assets/sample_diffusers.png) | + +--- + +## Acceleration Features + +### Supported Features + +| Feature | Supported | CLI Flag / Config Key | +|---------|----------|----------------------| +| Sequence Parallel | ✅ | `--sequence-parallel-size N` | +| CFG Parallel | ❌ | — | +| VAE Patch Parallel | ✅ | `vae_patch_parallel: true` | +| Tensor Parallel | ✅ | `--tensor-parallel-size N` | +| TeaCache | ✅ | `--teacache-thresh 0.15` | + +### Acceleration Demo + +```bash +# Tensor Parallel across 2 GPUs +vllm serve org/model-id --omni --tensor-parallel-size 2 + +# TeaCache for 1.5× speedup +vllm serve org/model-id --omni --teacache-thresh 0.15 +``` + +| Config | Latency | VRAM/GPU | +|--------|---------|---------| +| Baseline (1 GPU) | X.X s | XX GB | +| TP=2 | X.X s | XX GB | +| TeaCache | X.X s | XX GB | +| TP=2 + TeaCache | X.X s | XX GB | + +--- + +## Memory Optimization Features + +### Supported Options + +| Feature | Supported | CLI Flag / Config Key | +|---------|----------|----------------------| +| CPU Offload | ✅ | `--enable-cpu-offload` | +| FP8 Quantization | ✅ | `--quantization fp8` | +| VAE Tiling | ✅ | `vae_tiling: true` | + +### Memory Reduction Demo + +```bash +# CPU offload (reduces VRAM from XX GB to YY GB) +vllm serve org/model-id --omni --enable-cpu-offload + +# FP8 quantization +vllm serve org/model-id --omni --quantization fp8 +``` + +| Config | Peak VRAM | Latency | +|--------|----------|---------| +| Baseline (BF16) | XX.X GB | X.X s | +| CPU Offload 10 GB | XX.X GB | X.X s | +| FP8 Quantization | XX.X GB | X.X s | + +--- + +## Combined Feature Validation + +```bash +# CPU offload + TeaCache + Tensor Parallel +vllm serve org/model-id --omni \ + --tensor-parallel-size 2 \ + --teacache-thresh 0.15 \ + --cpu-offload-gb 5 +``` + +| Config | Latency | VRAM/GPU | Quality Notes | +|--------|---------|---------|--------------| +| All features combined | X.X s | XX GB | No visible quality degradation | + +Sample output with combined config: ![combined](./assets/sample_combined.png) + +--- + +## Documentation Updates + +- [ ] `docs/models/supported_models.md` — model row added +- [ ] `docs/features/feature_support.md` — feature row added +- [ ] `docs/features/compatibility.md` — compatibility table updated (if applicable) +- [ ] `docs/models/.md` — usage example doc added + +--- + +## Test Coverage + +- [ ] `tests/e2e/online_serving/test__expansion.py` — online serving e2e test +- [ ] `tests/models/test_.py` — offline inference test +```` + +--- + +## Measurement Guidelines + +### How to Measure e2e Latency + +**Offline:** +```python +import time +from vllm_omni.entrypoints.omni import Omni + +omni = Omni(model="org/model-id") + +# Warm-up (1 run, not measured) +omni.generate("warm-up prompt") + +# Timed run +start = time.perf_counter() +outputs = omni.generate("a cup of coffee on a wooden table") +end = time.perf_counter() +print(f"e2e latency: {end - start:.2f}s") +``` + +**Online (using `curl` with timing):** +```bash +time curl -s http://localhost:8091/v1/chat/completions \ + -H "Content-Type: application/json" \ + -d '{"messages": [{"role": "user", "content": "a cup of coffee"}], ...}' +``` + +### How to Measure Peak VRAM + +```python +import torch +torch.cuda.reset_peak_memory_stats() + +# Run generation here + +peak_mb = torch.cuda.max_memory_allocated() / 1024**2 +print(f"Peak VRAM: {peak_mb / 1024:.1f} GB") +``` + +Or via `nvidia-smi`: +```bash +nvidia-smi --query-gpu=memory.used --format=csv,noheader,nounits -l 1 +``` + +### What "Same Conditions" Means for diffusers Comparison + +When comparing vLLM-Omni vs diffusers: +- Same GPU model and count +- Same prompt +- Same resolution (H × W) +- Same `num_inference_steps` +- Same `seed` +- Same data type (both BF16 or both FP16) +- Both measured after one warm-up run + +--- + +## Common PR Body Mistakes + +| Mistake | Correct Approach | +|---------|-----------------| +| Using a tiny resolution (256×256) for latency measurement | Use the model's recommended resolution (typically 512×512 minimum, 1024×1024 for flagship models) | +| Reporting latency without warm-up run | Always warm up the model first; measure on the second+ run | +| Showing only one sample image | Show 2–3 samples with diverse prompts to demonstrate quality | +| Only demonstrating offline mode | Include both offline (`Omni`) and online (`vllm serve`) | +| Reporting VRAM from `nvidia-smi` at idle | Measure peak VRAM during active generation | +| Listing features as "planned" in the support table | Only mark ✅ for features that are implemented and tested in this PR | diff --git a/.claude/skills/review-pr/references/pitfalls.md b/.claude/skills/review-pr/references/pitfalls.md new file mode 100644 index 000000000..30ee80e1a --- /dev/null +++ b/.claude/skills/review-pr/references/pitfalls.md @@ -0,0 +1,222 @@ +# Common Pitfalls in vLLM-Omni + +## Mixin + nn.Module MRO Issue + +**Problem:** +When a mixin class is listed **after** `nn.Module` in inheritance, the mixin's `__init__` will **not** be called because `nn.Module.__init__()` doesn't call `super().__init__()`. + +```python +# BROKEN - Mixin's __init__ won't be called +class MyModel(nn.Module, SomeMixin): + def __init__(self): + super().__init__() # Only calls nn.Module.__init__! + self.mixin_method() # CRASH: mixin attributes not initialized +``` + +**Solution - Lazy Initialization:** +```python +class SomeMixin: + @property + def _internal_state(self) -> set: + if not hasattr(self, '_internal_state_storage'): + self._internal_state_storage = set() + return self._internal_state_storage +``` + +**Red Flag in Tests:** +If test mocks inherit only from the mixin (not `nn.Module`), they won't catch this bug because the test's `super().__init__()` WILL call the mixin's `__init__`. + +**Review Action:** +- Check inheritance order when you see mixins +- If mixin has `__init__` that sets attributes, flag it +- Verify tests use realistic class hierarchy + +--- + +## Connector State Management + +**Problem:** +Connectors use shared memory. Improper cleanup leads to leaks and crashes. + +```python +# BROKEN - No cleanup on error +def send_data(self, data): + self.shm_buffer.write(data) + raise ValueError("oops") # Buffer never released! +``` + +**Solution - Context Managers:** +```python +def send_data(self, data): + with self.shm_buffer.acquire() as buf: + buf.write(data) + # Automatically released even on error +``` + +**Review Action:** +- Check error paths in connector code +- Look for `try/finally` or context managers +- Verify cleanup in all branches + +--- + +## Async vs Sync Path Differences + +**Problem:** +`AsyncOmni` and `Omni` have different code paths. Changes may work in one but not the other. + +```python +# Works in sync path +def process(self, request): + result = blocking_call() # OK in sync + return result + +# Breaks in async path +async def process(self, request): + result = blocking_call() # Blocks event loop! + return result +``` + +**Solution:** +```python +async def process(self, request): + result = await asyncio.to_thread(blocking_call) + return result +``` + +**Review Action:** +- Check if PR modifies shared code paths +- Verify both sync and async are tested +- Look for blocking calls in async code + +--- + +## Stage Configuration Validation + +**Problem:** +Stage configs are loaded at init time. Invalid configs may not fail until runtime. + +```python +# Config loaded but not validated +config = load_yaml("stages.yaml") +# ... much later ... +stage = create_stage(config["unknown_stage"]) # Crash! +``` + +**Solution - Eager Validation:** +```python +config = load_yaml("stages.yaml") +validate_stage_config(config) # Fail fast +``` + +**Review Action:** +- Check for config validation at load time +- Verify all required fields are checked +- Look for defaults that mask invalid configs + +--- + +## Memory Management in Diffusion + +**Problem:** +Diffusion models manage large latent caches. Memory leaks accumulate across generations. + +```python +# BROKEN - Latent cache grows unbounded +class DiffusionModel: + def generate(self, prompt): + self.latent_cache.append(create_latents()) # Never cleared! + return decode(self.latent_cache[-1]) +``` + +**Solution:** +```python +class DiffusionModel: + def generate(self, prompt): + latents = create_latents() + try: + return decode(latents) + finally: + del latents # Explicit cleanup +``` + +**Review Action:** +- Check latent cache lifecycle +- Verify cleanup in generation loops +- Look for memory-pressure handling + +--- + +## Input Validation in API Layer + +**Problem:** +Invalid requests that reach the engine can cause crashes instead of clean errors. + +```python +# BROKEN - Validation happens in engine +def create_speech(self, request): + # Missing validation here + return self.engine.generate(request) # Engine crashes! +``` + +**Solution - Validate Early:** +```python +def create_speech(self, request): + error = self._validate_request(request) + if error: + return BadRequest(error) # Clean 400 response + return self.engine.generate(request) +``` + +**Review Action:** +- Check validation before engine calls +- Verify all parameters are validated +- Ensure error messages are actionable + +--- + +## Tensor Parallelism Edge Cases + +**Problem:** +Code may work single-GPU but break with tensor parallelism. + +```python +# Works on single GPU +def forward(self, x): + return self.layer(x) + self.bias # bias not replicated! +``` + +**Solution:** +```python +def forward(self, x): + return self.layer(x) + self.bias.data # Explicitly access data +``` + +**Review Action:** +- Check if changes affect distributed execution +- Verify tensor parallel tests exist +- Look for rank-specific logic + +--- + +## Test Mock Mismatches + +**Problem:** +Tests may mock classes differently from production, hiding bugs. + +```python +# Production +class Model(nn.Module, Mixin): + pass + +# Test mock - different MRO! +class MockModel(Mixin): # Missing nn.Module + pass +``` + +This hides the MRO bug described above. + +**Review Action:** +- Compare mock inheritance to production +- Check if mocks skip critical base classes +- Verify mock behavior matches production diff --git a/.claude/skills/review-pr/references/review-execution.md b/.claude/skills/review-pr/references/review-execution.md new file mode 100644 index 000000000..e1b18057f --- /dev/null +++ b/.claude/skills/review-pr/references/review-execution.md @@ -0,0 +1,126 @@ +# Review Execution + +Use this file when you are actively running the review and need the gate checks, concrete `gh` commands, or comment-writing rules. + +## Review Gates + +Check these before deep review. If any fail, stop and post a gate-failure summary instead of doing a full review. + +| Check | Passing State | Action if Failed | +|-------|---------------|------------------| +| DCO | `SUCCESS` | Ask for signed commits with `git commit -s` | +| pre-commit | `SUCCESS` | Ask the author to run `pre-commit run --all-files` | +| mergeable | `MERGEABLE` | Ask the author to rebase and resolve conflicts | + +Command: + +```bash +gh pr view --repo vllm-project/vllm-omni --json mergeable,statusCheckRollup --jq '{mergeable, checks: [.statusCheckRollup[] | {name, conclusion}]}' +``` + +Gate-failure template: + +```text +Review blocked until the PR is mergeable and required checks pass. + +- DCO: +- pre-commit: +- mergeability: +``` + +## Minimal Fetch Sequence + +```bash +gh pr view --repo vllm-project/vllm-omni --json title,body,author,state,files,closingIssuesReferences +gh pr diff --repo vllm-project/vllm-omni +``` + +For linked issues: + +```bash +gh pr view --repo vllm-project/vllm-omni --json closingIssuesReferences --jq '.closingIssuesReferences[] | {number, title, body}' +gh issue view --repo vllm-project/vllm-omni --json title,body,labels,state,comments +``` + +For more code context: + +```bash +gh api repos/vllm-project/vllm-omni/contents/?ref= +gh search code --repo vllm-project/vllm-omni "class " +gh search code --repo vllm-project/vllm-omni "" --extension yaml +``` + +## Comment Budget + +Keep reviews selective. + +| PR Shape | Expected Inline Comments | +|----------|--------------------------| +| docs-only or tiny fix | 0 | +| medium bug fix | 1-3 | +| large feature or risky refactor | 3-5 | + +Budget rules: + +- Cap normal reviews at 5 inline comments +- Merge related issues into one comment +- Skip generic praise and low-confidence speculation +- If domain review already surfaced several issues, skip extra general comments + +## Comment Style + +Use short, direct comments tied to a concrete file and behavior. + +| PR State | Preferred Tone | +|----------|----------------| +| Draft | Ask focused questions and call out missing evidence | +| Ready for review | Request specific changes for blocking issues | +| Approved or already addressed | Comment only on newly found blockers | + +Banned patterns: + +- generic praise without evidence +- vague quality language like "solid" or "well structured" +- comments without a concrete failure mode or missing validation + +## Good Comment Shapes + +Missing test: + +```text +This bug fix still needs a regression test that reproduces the original failure. Without it, this path can regress silently in a future refactor. +``` + +Missing evidence: + +```text +The PR claims a latency reduction, but I do not see before/after measurements or the benchmark setup. Please include numbers for a representative workload. +``` + +MRO issue: + +```text +This mixin appears after `nn.Module` but still relies on `__init__` side effects. In that hierarchy the mixin initializer will not run, so the new attribute is not guaranteed to exist. +``` + +## Review Submission + +Use one review summary plus inline comments for the actual findings. + +```bash +gh api repos/vllm-project/vllm-omni/pulls//reviews --input - <", + "comments": [ + {"path": "", "line": , "body": ""} + ] +} +EOF +``` + +Summary checklist: + +- what you validated +- what still lacks tests or evidence +- whether the PR is blocked or only needs follow-up diff --git a/.claude/skills/review-pr/references/review-routing.md b/.claude/skills/review-pr/references/review-routing.md new file mode 100644 index 000000000..5a9d4abe4 --- /dev/null +++ b/.claude/skills/review-pr/references/review-routing.md @@ -0,0 +1,93 @@ +# Review Routing + +Use this file when the main review skill needs help deciding which domain skill, secondary skill, or optional specialized reviewer to involve. + +## Prefix Routing + +| PR Prefix | Primary Skill | Main Review Focus | +|-----------|---------------|-------------------| +| `[Image]`, `[ImageGen]` | `vllm-omni-image-gen` | Generation quality, diffusion settings, VRAM use | +| `[Video]`, `[VideoGen]` | `vllm-omni-video-gen` | Temporal consistency, throughput, frame pipeline | +| `[Audio]`, `[TTS]` | `vllm-omni-audio-tts` | Audio quality, latency, streaming behavior | +| `[Multimodal]` | `vllm-omni-multimodal` | Cross-modal correctness, shared processor behavior | +| `[Distributed]` | `vllm-omni-distributed` | Scaling, OmniConnector, multi-node invariants | +| `[Quantization]` | `vllm-omni-quantization` | Quality loss, loader compatibility, memory claims | +| `[Performance]` | `vllm-omni-perf` | Benchmarks, latency, throughput, cache tuning | +| `[Hardware]` | `vllm-omni-hardware` | Backend compatibility, device guards, platform assumptions | +| `[API]` | `vllm-omni-api` | OpenAI compatibility, request validation, response behavior | +| `[CI]` | `vllm-omni-cicd` | Pipeline correctness, deployment safety | +| `[Model]` | `vllm-omni-contrib` | Model integration patterns, tests, config wiring | +| `[Bug]`, `[Bugfix]` | none by default | Load [bug-test-coverage.md](bug-test-coverage.md) for regression reproduction and test coverage | +| `[Refactor]` | none by default | Behavior preservation and invariant drift | +| `[Feature]` | none by default | Acceptance criteria, docs, tests | + +## Multi-Skill Routing + +Use one primary skill and add a secondary skill only when the diff crosses a true subsystem boundary. + +| Scenario | Primary | Secondary | +|----------|---------|-----------| +| `[Performance]` + `[Distributed]` | `vllm-omni-perf` | `vllm-omni-distributed` | +| `[Performance]` + `[Model]` | `vllm-omni-perf` | `vllm-omni-contrib` | +| `[Model]` + `[Quantization]` | `vllm-omni-contrib` | `vllm-omni-quantization` | +| `[Feature]` + `[API]` | `vllm-omni-api` | none in most cases | +| `[Distributed]` + `[Hardware]` | `vllm-omni-distributed` | `vllm-omni-hardware` | + +## Hardware Detection + +Load `vllm-omni-hardware` when the diff shows backend-specific code even if the title prefix does not. + +Patterns worth flagging: + +- `is_npu` +- `is_cuda` +- `torch.cuda` +- `torch_npu` +- device-specific literals such as `cuda:0` + +Review concerns: + +- missing non-CUDA fallback paths +- platform-specific ops without guards +- hardcoded device assumptions +- memory handling that differs across platforms + +## Delegation Triggers + +Treat specialized reviewers as optional integrations. If the runtime has no `Agent(...)` or subagent tool, do the same checks manually and do not block on delegation. + +| Trigger | Optional reviewer if available | Manual review focus | +|---------|--------------------------------|---------------------| +| new or changed `try/except` blocks | `pr-review-toolkit:silent-failure-hunter` | swallowed exceptions, empty fallbacks, missing logs, bad retry behavior | +| new `@dataclass` or `TypedDict` | `pr-review-toolkit:type-design-analyzer` | invariants, missing validation, unsafe defaults, optional-vs-required fields | +| test files changed | `pr-review-toolkit:pr-test-analyzer` | coverage gaps, unrealistic mocks, missing regression cases, weak assertions | +| `vllm_omni/entrypoints/` changes | `code-reviewer` | input validation, abuse resistance, unsafe parameter passthrough, bad error surfaces | +| config or validation code changes | `code-reviewer` | validation completeness, backward compatibility, edge cases, default handling | + +Delegation limits: + +- Max 2 delegated reviewers per PR +- Skip delegation for docs-only PRs +- Skip delegation for small PRs unless the risk is unusually high +- Delegated findings still count against the total comment budget + +If no delegation tool exists: + +- Apply the manual focus column directly during review +- Keep the same 5-comment budget +- Do not mention missing reviewer tooling in the PR unless the user explicitly asks + +## Context Expansion Rules + +Fetch more context when one of these is true: + +- linked issue contains the actual acceptance criteria +- the diff changes shared infrastructure with prior art in adjacent PRs +- three lines of diff do not show cleanup, lifecycle, or invariant handling +- benchmark or quality claims reference data not included in the PR + +Keep expansion bounded: + +- 2-3 linked issues max +- 5 related PRs per important file max +- 3-5 code context fetches per review diff --git a/.claude/skills/review-pr/references/tests-docs-checklist.md b/.claude/skills/review-pr/references/tests-docs-checklist.md new file mode 100644 index 000000000..b429978ce --- /dev/null +++ b/.claude/skills/review-pr/references/tests-docs-checklist.md @@ -0,0 +1,110 @@ +## PR Tests & Docs Review Addendum + +This reference adds two structured outputs for `vllm-omni-review`: + +- Coverage matrix (change point → existing tests → gap → minimal fix) +- PR description checklist (environment + runtime estimate + fill-in template) + +### 1) Coverage Matrix (required for high-risk changes) + +Use this format. Keep it short and actionable. + +- Change point: + - What changed: + - Risk if broken: + - Where it lives (paths/modules): +- Existing tests: + - Unit tests: + - E2E tests (`tests/e2e/`): + - Example/gate tests (if any): +- Gaps: + - Missing behavior coverage: + - Missing failure/boundary case: + - Missing regression (for bugfix): +- Minimal add: + - Suggested test file(s): + - Suggested test cases (1–3 bullets): + - Suggested assertions (stable, deterministic): + +### 2) Style / Tag / Stability Checklist (quick pass) + +- Markers/labels follow repository CI docs and are justified (no marker sprawl). +- Assertions are stable (avoid external network/time/randomness). +- Numeric assertions avoid direct float `==` / `!=` and use tolerances/approx checks. +- Test names and failure messages are readable and help locate the issue quickly. + +### Optional References (use as needed) + +- CI test levels: `https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/CI_5levels/` +- Test markers: `https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/tests_markers/` +- Tests entry point: `https://github.com/vllm-project/vllm-omni/tree/main/tests` +- For anything else, open the most relevant pages under `vllm-omni/docs` based on the PR's change points. + +### 3) Docs Sync Checklist (examples ↔ docs) + +- If `examples/` README/docs were changed, ensure the corresponding `docs/` entry points are updated (avoid stale user-facing docs). If the repository uses MkDocs, run `mkdocs serve` to preview the docs site locally and verify the navigation/entry pages reflect the changes. +- If a new model/feature/parameter/default changed, ensure `vllm-omni/docs` includes: + - What it is / how to use + - Constraints and known limitations + - Minimal runnable example (when appropriate) + +### 4) PR Description Checklist (environment + runtime) + +Ensure the PR description includes: + +- Required hardware/environment: + - CPU/GPU type, VRAM, CUDA/driver (or ROCm/NPU/XPU), OS + - Python/vllm/vllm-omni version, key package versions (if relevant) +- What was run: + - Commands (exact) + - Expected outputs (brief) +- Runtime estimate: + - Rough duration (order of magnitude is enough) + +Fill-in template (2–3 lines): + +``` +Test env: , , , Python +Commands: ; +Runtime: ~ min +``` + +### 5) Performance Comparison Test Checklist + +**Required for:** `[Feature]` PRs that affect performance characteristics, and all `[Performance]` PRs. + +Before approving, verify the PR includes performance comparison tests that measure the change against baseline (without the change). + +**Minimum Requirements:** + +- [ ] **Before/after benchmark** — Quantitative comparison on same hardware +- [ ] **Consistent environment** — Same GPU, driver, model, batch size, resolution/steps for both runs +- [ ] **Key metrics reported:** + - [ ] Latency (e2e generation time) + - [ ] Throughput (requests/sec or tokens/sec, where applicable) + - [ ] Peak memory usage (VRAM in GB) +- [ ] **Methodology documented** — Commands, parameters, prompts/dataset used + +**Evidence format to request when missing:** + +``` +⚠️ **Performance Comparison Test Required** + +This PR includes a [feature/performance enhancement] that may affect performance. +Please provide benchmark comparison: + +| Metric | Baseline (w/o change) | With Change | Delta | +|------------------|----------------------|-------------|-------| +| Latency (e2e) | ms | ms | +/- % | +| Throughput | req/s | | +/- % | +| Peak VRAM | GB | GB | +/- % | + +Test environment: , , , +Commands: +``` + +**Exceptions:** +- Doc-only or config-only changes without performance impact +- Bug fixes that only correct logic without changing performance characteristics +- Changes gated behind a new flag that defaults to disabled (document expected perf when enabled) + diff --git a/.gitignore b/.gitignore index 7f101a784..c0ee96806 100644 --- a/.gitignore +++ b/.gitignore @@ -158,7 +158,19 @@ cython_debug/ # Claude CLAUDE.md -.claude/ +/.claude/* +!.claude/skills/ +!.claude/skills/readme.md +!.claude/skills/add-diffusion-model/ +!.claude/skills/add-diffusion-model/SKILL.md +!.claude/skills/add-diffusion-model/references/ +!.claude/skills/add-diffusion-model/references/*.md +!.claude/skills/add-tts-model/ +!.claude/skills/add-tts-model/SKILL.md +!.claude/skills/review-pr/ +!.claude/skills/review-pr/SKILL.md +!.claude/skills/review-pr/references/ +!.claude/skills/review-pr/references/*.md # Codex AGENTS.md