Compare commits
5
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
f852de7336 | ||
|
|
138dc3b74f | ||
|
|
fb472ecfc7 | ||
|
|
975d79e44b | ||
|
|
c3434dc21b |
No files matched your search
@@ -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
|
||||
@@ -22,7 +22,12 @@ include:
|
||||
- `add-tts-model`: covers integration of new TTS models and related serving
|
||||
workflows
|
||||
- `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
|
||||
- `vllm-omni-npu-model-runner-upgrade`: upgrades NPU model runners to align with the
|
||||
latest vllm-ascend NPUModelRunner
|
||||
|
||||
## Maintenance Guidelines
|
||||
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
<!-- 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.
|
||||
|
||||
## 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.
|
||||
</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)
|
||||
@@ -116,6 +116,20 @@ Only specific types of PRs will be reviewed. The PR title is prefixed appropriat
|
||||
!!! note
|
||||
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
|
||||
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.
|
||||
|
||||
|
||||
Reference in new issue
Block a user