Compare commits

...
Author SHA1 Message Date
hsliuandClaude Opus 4.7 f852de7336 Address review feedback on precheck-pr skill
- Fix base-branch detection: use BASE_SHA, fallback to origin/main ref
- Add union instruction for multi-category PRs in Step 3
- Enforce [Model] prefix includes model identifier
- Scope Cache-DiT check: blocking for top-level pipelines, warning for subclass
- Recognize deferred accuracy/perf claims in PR body
- Add import hygiene carve-out for closures/factory functions

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: hsliu <liuhongsheng4@huawei.com>
2026-06-08 15:24:14 +08:00
hsliuandClaude Opus 4.7 138dc3b74f [Doc] Rephrase precheck-pr wording in docs and PR template
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: hsliu <liuhongsheng4@huawei.com>
2026-06-06 22:44:41 +08:00
hsliuandClaude Opus 4.7 fb472ecfc7 [Doc] Fix review comments on precheck-pr skill
- Fix NPU skill name in readme to match declared name
- Fix base-branch detection to use explicit fallback
- Fix title format reference to point to docs/contributing/README.md
- Move /precheck-pr prompt to top of PR template

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: hsliu <liuhongsheng4@huawei.com>
2026-06-06 22:36:19 +08:00
hsliuandClaude Opus 4.7 975d79e44b [Doc] Document precheck-pr skill in PR template and contributing guide
Add precheck-pr reference to PULL_REQUEST_TEMPLATE.md BEFORE SUBMITTING
section, and add a new "Pre-Check with Claude Code" subsection to the
contributing guide.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: hsliu <liuhongsheng4@huawei.com>
2026-06-06 22:30:28 +08:00
hsliuandClaude Opus 4.7 c3434dc21b [Doc] Add precheck-pr Claude Code skill
Self-check a branch before creating a PR — validates PR title format
against project conventions, catches dead code, verifies accuracy/perf
claims, and confirms merge readiness. Two modes: quick (~3 min) and
full (~10 min). Covers Bug Fix, Performance, New Model, Diffusion Model,
and General PR types.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: hsliu <liuhongsheng4@huawei.com>
2026-06-06 22:21:20 +08:00
5 changed files with 255 additions and 1 deletions

No files matched your search

+97
View File
@@ -0,0 +1,97 @@
---
name: precheck-pr
description: Self-check your branch before creating a PR — catch dead code, verify accuracy/perf claims, validate PR title format, and confirm merge readiness. Use when the user says "precheck", "self review", "pre-submit check", or "check my PR before I open it." Never posts to GitHub.
---
# PR Pre-Check
Self-review your branch before creating a PR against `vllm-project/vllm-omni`. Two modes: **quick** catches showstoppers, **full** does a thorough maintainer-grade review. Never posts to GitHub; the report is for the contributor's terminal only.
## Mode Selection
| Mode | When | Time |
|------|------|------|
| **Quick** | About to push, final sanity check | ~3 min |
| **Full** | Ready for review, want maintainer-level scan | ~10 min |
Default to quick if unsure. Run full before marking a PR "ready for review."
## Workflow
### Step 1: Detect Base Branch
```bash
BASE_SHA=$(git merge-base HEAD origin/main 2>/dev/null \
|| git merge-base HEAD main 2>/dev/null \
|| echo origin/main)
echo "diffing against ${BASE_SHA}"
git diff --name-only ${BASE_SHA}...HEAD
```
### Step 2: Validate PR Title
Check the most recent commit message (or branch name if no commit yet) against the project convention. Valid prefixes:
| Prefix | Applies to |
|--------|-----------|
| `[Bugfix]` | Bug fixes |
| `[CI/Build]` | Build or CI improvements |
| `[Doc]` | Documentation changes |
| `[Model]` | New/improved models (include model name) |
| `[Frontend]` | Frontend changes (API server, OmniLLM class, etc.) |
| `[Kernel]` | CUDA/kernel changes |
| `[Core]` | Core logic changes (OmniProcessor, OmniARScheduler, etc.) |
| `[Hardware][Vendor]` | Hardware-specific (e.g., `[Hardware][Ascend]`) |
| `[Misc]` | Other changes (use sparingly) |
✗ if: missing prefix, wrong case (`[bugfix]`), or WIP/Draft in title.
⚠ if: `[Model]` prefix without the model identifier (e.g., `[Model] Add new model` — should be `[Model] Add <ModelName> ...`).
### Step 3: Categorize the PR
| Diff contains | PR type |
|---------------|---------|
| New files under `vllm_omni/model_executor/models/<name>/` | **New Model** |
| Changes to `vllm_omni/diffusion/` | **Diffusion Model** |
| `[Bugfix]` prefix or single-file fix | **Bug Fix** |
| Perf/benchmark/throughput claims in commit msg or diff | **Performance** |
| Everything else | **General** |
If multiple rows apply (e.g., a diffusion model is also a new model), union the checklists.
### Step 4: Run Checklist
Ask: "Quick mode or full mode?" Then walk the checklist for the detected PR type from [references/checklists.md](references/checklists.md). Each item produces ✓, ✗, or ⚠.
### Step 5: Print Report
```
Pre-check report for <branch>
Mode: quick | full
Type: <new-model | diffusion-model | bug-fix | perf | general>
Dimension Result
───────────────── ──────
PR title format ✓
PR desc integrity ✓
Registry/config ✓
Dead code ⚠ 2 warnings
Accuracy ✓
Benchmark ✗ missing software versions
Verdict: 1 blocking | 2 warnings | recommend fixing ✗ before PR
```
**Severity:**
| Mark | Meaning |
|------|---------|
| ✗ | Blocking — fix before opening PR |
| ⚠ | Warning — consider fixing |
| ✓ | Pass |
| — | Skipped (not applicable) |
### Stop Here
Do not post comments, open PRs, or modify files. The report is for the contributor's terminal only.
@@ -0,0 +1,135 @@
# Pre-Check Checklists
Each item is a concrete check to run against the diff. ✗ = fix before PR. ⚠ = consider fixing.
---
## All PRs: Title Format (check first)
PR title must follow the project convention documented in `docs/contributing/README.md`:
| Prefix | Applies to |
|--------|-----------|
| `[Bugfix]` | Bug fixes |
| `[CI/Build]` | Build or CI improvements |
| `[Doc]` | Documentation changes |
| `[Model]` | New/improved models (include model name) |
| `[Frontend]` | Frontend changes (API server, OmniLLM class, etc.) |
| `[Kernel]` | CUDA/kernel changes |
| `[Core]` | Core logic changes (OmniProcessor, OmniARScheduler, etc.) |
| `[Hardware][Vendor]` | Hardware-specific (e.g., `[Hardware][Ascend]`) |
| `[Misc]` | Other changes (use sparingly) |
**Examples:**
```
✓ [Model] Add MiniCPM-o 4.5 support
✓ [Bugfix] Fix Ovis image text encoder dtype
✓ [Core] Qwen3-Omni performance optimization
✓ [Hardware][Ascend] Fix VoxCPM2 on Ascend
✗ Fix Ovis image text encoder dtype ← missing prefix
✗ [bugfix] Fix Ovis image text encoder ← wrong case
✗ [WIP] Add model ← WIP in title
```
- [ ] **Title has valid prefix** — ✗ if missing prefix, wrong case, or WIP/Draft in title. For `[Model]` PRs, the title MUST also include the model identifier (e.g., `[Model] Add <ModelName> ...`). ⚠ if missing the model name.
---
## Bug Fix PRs
### Quick
- [ ] **PR body has 4 sections:** what broke (error message), repro steps, root cause, fix description
- [ ] **Fix is minimal:** diff touches only files directly related to the bug. No unrelated refactors or style changes.
- [ ] **Repro is runnable:** the PR body includes a copy-paste command that reproduces the bug
- [ ] **Branch is rebased:** `git merge-base HEAD origin/main` is recent, no merge conflicts
### Full (adds)
- [ ] **Regression test exists:** `git diff --name-only` includes at least one `tests/` file
- [ ] **Regression test reproduces the original error:** the test would fail on main, pass on this branch
- [ ] **Fix matches root cause exactly:** no "fix the symptom + something else" — if the root cause is dtype, the fix is dtype, not dtype + reformatting
- [ ] **No silent failure risk:** no bare `except: pass`, no `try: ... except: return None`, no empty fallback added
- [ ] **Upstream pattern match:** the fix follows the same pattern as existing code for similar cases (grep for analogous `from_pretrained` calls, etc.)
- [ ] **Environment documented:** torch/transformers/vllm versions listed if the bug is version-dependent
---
## Performance PRs
### Quick
- [ ] **Before/after numbers in PR body:** at minimum, one metric (RTF, RPS, latency, VRAM) with main vs PR values
- [ ] **Hardware specified:** GPU model, count, VRAM. "L4" or "H20" is sufficient. "tested on GPU" is not.
- [ ] **Concurrency scaling:** if the optimization claims better throughput, numbers at 2+ concurrency levels
- [ ] **No unexplained regressions:** if any metric goes down at any concurrency level, explain why
- [ ] **Branch is rebased:** `git merge-base HEAD origin/main` is recent, no merge conflicts
### Full (adds)
- [ ] **Software versions:** torch, CUDA, vllm, vllm-omni versions in PR body
- [ ] **Warmup stated:** "1 warmup + 3 measured" or equivalent
- [ ] **Stddev or range:** not just a single best run. "average of 2 rounds" is minimum.
- [ ] **VRAM measured:** peak GPU memory before and after
- [ ] **Benchmark script checked in:** under `tests/` or `examples/`, runnable by reviewer
- [ ] **Exact command line in PR:** not pseudocode, not "run the benchmark"
- [ ] **Pytest summary included:** `36 passed, 3 skipped, 17 warnings in 2897.66s` — proves the whole suite passes
- [ ] **Config changes documented:** if `max_num_seqs`, `gpu_memory_utilization`, or deploy YAML changed, old and new values stated with reasoning
---
## New Model PRs
### Quick
- [ ] **PR body file list matches `git diff --name-only`:** every file path in the description exists in the diff
- [ ] **Registry entries match claimed count:** count `_OMNI_MODELS` or `_DIFFUSION_MODELS` entries added vs claimed in body
- [ ] **Pipeline config `custom_process_input_func` paths resolve:** each referenced module.function exists
- [ ] **`__init__.py` exports match registry:** each registered class appears in `__init__.py` and `__all__`
- [ ] **Shell script modes match README:** `run_curl.sh` supports every mode the README documents
- [ ] **Branch is rebased:** `git merge-base HEAD origin/main` is recent, no merge conflicts
### Full (adds)
- [ ] **Dead code scan (5 patterns):**
1. Dead `forward()` — defined but never called through the pipeline path
2. Dead factory/builder functions — instantiated nowhere
3. Dead wrappers — thin wrappers with no callers
4. Dead branch guards — `if version > X` where X is always satisfied
5. Unused parameters — `__init__` args never accessed
- [ ] **Copy-paste detection:** no string constant defined in 3+ files, no cross-module validation duplication, no near-identical shape coercion fns
- [ ] **Import hygiene:** no re-export-only imports, no module-level side effects without docs, no `import os` in function body when already at top. Function-body imports are acceptable when the import is only reachable from a closure or factory return value (i.e., `import` would otherwise execute at module load even though the user may never trigger that path). Either lift to top-of-file or annotate with `# noqa: PLC0415`.
- [ ] **Accuracy:** at least one test compares output against reference; audio = valid WAV at correct sample rate; determinism verified if seed claimed. ⚠ if missing and not explicitly deferred to a follow-up PR or RFC section; — if explicitly deferred (the deferral target SHOULD be linked in the PR body).
- [ ] **Performance:** hardware, software versions, warmup, and metrics (RTF, VRAM, RPS, latency) stated. ⚠ if missing and not explicitly deferred; — if explicitly deferred.
- [ ] **Benchmark settings:** model config (TP, PP, max_model_len, enforce_eager, quant), runtime config (batch, input/output spec), environment versions
- [ ] **Benchmark script checked in + exact command line + pytest summary**
---
## Diffusion Model PRs
Diffusion models live under `vllm_omni/diffusion/`. In addition to the New Model checklist above, verify:
- [ ] **Transformer adapter follows contract:** inherits from `nn.Module`, implements `load_weights()`, uses `vllm_omni.diffusion.attention.layer.Attention`
- [ ] **Pipeline has both offline and online paths:** `forward()` for batch generation, streaming path for online serving
- [ ] **Cache-DiT integration:** at least one acceleration (TeaCache, FBCache, etc.) is wired up. ✗ for new **top-level** pipelines. For **subclass** pipelines that inherit `__call__` from a parent where Cache-DiT is already absent, downgrade to ⚠ and recommend filing a follow-up issue against the parent pipeline family.
- [ ] **Parallelism config:** TP/SP/USP/CFG-Parallel options exposed in pipeline config
- [ ] **Docs table updated:** model added to `docs/models/supported_models.md` with correct metadata
- [ ] **E2E test exists:** at least one test exercises the full generate path
---
## General PRs
### Quick
- [ ] **PR body matches diff:** description doesn't claim changes not in the diff
- [ ] **Branch is rebased:** `git merge-base HEAD origin/main` is recent, no merge conflicts
- [ ] **CI gates passing:** DCO, pre-commit, build — check `gh pr view --json statusCheckRollup` if a PR already exists
### Full (adds)
- [ ] **No new dead code in changed files:** scan for unreachable paths, unused imports
- [ ] **Test coverage:** if core code changed (`engine/`, `stages/`, `connectors/`), tests are added or existing tests cover the path
- [ ] **Import hygiene:** no redundant imports added
- [ ] **No unrelated changes:** diff scope matches PR description
+5
View File
@@ -22,7 +22,12 @@ include:
- `add-tts-model`: covers integration of new TTS models and related serving - `add-tts-model`: covers integration of new TTS models and related serving
workflows workflows
- `generate-release-note`: helps prepare release notes for repository changes - `generate-release-note`: helps prepare release notes for repository changes
- `precheck-pr`: self-check a branch before creating a PR — validates PR title
format, catches dead code, verifies accuracy/perf claims, and confirms merge
readiness
- `review-pr`: provides a structured workflow for reviewing pull requests - `review-pr`: provides a structured workflow for reviewing pull requests
- `vllm-omni-npu-model-runner-upgrade`: upgrades NPU model runners to align with the
latest vllm-ascend NPUModelRunner
## Maintenance Guidelines ## Maintenance Guidelines
+4 -1
View File
@@ -1,4 +1,6 @@
<!-- markdownlint-disable --> <!-- markdownlint-disable -->
**Before submitting:** run the precheck-pr skill with code agent for a self-check against project conventions — catches dead code, missing benchmarks, and title format issues.
PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED. PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.
## Purpose ## Purpose
@@ -18,4 +20,5 @@ PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTT
- [ ] (Optional) Release notes update. If your change is user-facing, please update the release notes draft. - [ ] (Optional) Release notes update. If your change is user-facing, please update the release notes draft.
</details> </details>
**BEFORE SUBMITTING, PLEASE READ <https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md>** (anything written below this line will be removed by GitHub Actions) **BEFORE SUBMITTING,** please read <https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md>
(anything written below this line will be removed by GitHub Actions)
+14
View File
@@ -116,6 +116,20 @@ Only specific types of PRs will be reviewed. The PR title is prefixed appropriat
!!! note !!! note
If the PR spans more than one category, please include all relevant prefixes. If the PR spans more than one category, please include all relevant prefixes.
### Pre-Check Before Submitting
Before submitting a PR, run the precheck-pr skill with code agent for a self-review against project conventions:
```
/precheck-pr
```
The skill offers two modes:
- **Quick (~3 min):** catches showstoppers — PR title format, missing benchmark claims, rebase status
- **Full (~10 min):** thorough maintainer-grade review — dead code scan, copy-paste detection, import hygiene
The precheck covers five PR types: Bug Fix, Performance, New Model, Diffusion Model, and General. Each type has a tailored checklist that validates evidence quality (repro steps, A/B benchmarks, registry entries, etc.). See the skill in `.claude/skills/precheck-pr/` for the full checklist.
### Local Test ### Local Test
Please run the L1 and L2 test cases locally first and attach the results before contacting us to add the "ready" label. Please refer to the [test instructions](./ci/test_guide.md) for running the test cases. Please run the L1 and L2 test cases locally first and attach the results before contacting us to add the "ready" label. Please refer to the [test instructions](./ci/test_guide.md) for running the test cases.