From f663d120c78e225976d6cd07b2f93a5c5e13012d Mon Sep 17 00:00:00 2001 From: Hongsheng Liu Date: Sat, 8 Aug 2026 22:18:54 +0800 Subject: [PATCH] [Misc] Add vLLM-Omni PR review skill (#5871) Signed-off-by: Hongsheng Liu --- .claude/skills/review-pr/SKILL.md | 231 ++++++++++++++++++ .claude/skills/review-pr/agents/openai.yaml | 4 + .../checks/model-addition-checklist.md | 121 +++++++++ .../references/checks/perf-verification.md | 51 ++++ .../checks/test-quality-evaluation.md | 39 +++ .../references/checks/tests-docs-checklist.md | 48 ++++ .../references/checks/verification.md | 36 +++ .../delivery/maintainer-style-study.md | 105 ++++++++ .../references/delivery/review-requests.md | 99 ++++++++ .../references/features/communication.md | 24 ++ .../features/diffusion-acceleration.md | 26 ++ .../features/infrastructure-performance.md | 22 ++ .../features/runtime-stage-execution.md | 24 ++ .../references/modules/ar-runtime.md | 22 ++ .../references/modules/benchmarking.md | 24 ++ .../references/modules/cache-management.md | 22 ++ .../references/modules/configuration.md | 24 ++ .../review-pr/references/modules/diffusion.md | 33 +++ .../modules/engine-orchestration.md | 23 ++ .../references/modules/entrypoints.md | 25 ++ .../references/modules/error-contracts.md | 24 ++ .../references/modules/execution-platforms.md | 23 ++ .../modules/input-output-modality.md | 23 ++ .../references/modules/model-integration.md | 25 ++ .../references/modules/observability.md | 21 ++ .../references/modules/omni-connector.md | 24 ++ .../review-pr/references/modules/profiling.md | 21 ++ .../references/modules/quantization.md | 22 ++ .../references/modules/stage-runtime.md | 23 ++ .../references/process/design-contracts.md | 34 +++ .../references/process/general-checks.md | 129 ++++++++++ .../references/process/review-execution.md | 191 +++++++++++++++ .../references/process/review-routing.md | 76 ++++++ 33 files changed, 1639 insertions(+) create mode 100644 .claude/skills/review-pr/SKILL.md create mode 100644 .claude/skills/review-pr/agents/openai.yaml create mode 100644 .claude/skills/review-pr/references/checks/model-addition-checklist.md create mode 100644 .claude/skills/review-pr/references/checks/perf-verification.md create mode 100644 .claude/skills/review-pr/references/checks/test-quality-evaluation.md create mode 100644 .claude/skills/review-pr/references/checks/tests-docs-checklist.md create mode 100644 .claude/skills/review-pr/references/checks/verification.md create mode 100644 .claude/skills/review-pr/references/delivery/maintainer-style-study.md create mode 100644 .claude/skills/review-pr/references/delivery/review-requests.md create mode 100644 .claude/skills/review-pr/references/features/communication.md create mode 100644 .claude/skills/review-pr/references/features/diffusion-acceleration.md create mode 100644 .claude/skills/review-pr/references/features/infrastructure-performance.md create mode 100644 .claude/skills/review-pr/references/features/runtime-stage-execution.md create mode 100644 .claude/skills/review-pr/references/modules/ar-runtime.md create mode 100644 .claude/skills/review-pr/references/modules/benchmarking.md create mode 100644 .claude/skills/review-pr/references/modules/cache-management.md create mode 100644 .claude/skills/review-pr/references/modules/configuration.md create mode 100644 .claude/skills/review-pr/references/modules/diffusion.md create mode 100644 .claude/skills/review-pr/references/modules/engine-orchestration.md create mode 100644 .claude/skills/review-pr/references/modules/entrypoints.md create mode 100644 .claude/skills/review-pr/references/modules/error-contracts.md create mode 100644 .claude/skills/review-pr/references/modules/execution-platforms.md create mode 100644 .claude/skills/review-pr/references/modules/input-output-modality.md create mode 100644 .claude/skills/review-pr/references/modules/model-integration.md create mode 100644 .claude/skills/review-pr/references/modules/observability.md create mode 100644 .claude/skills/review-pr/references/modules/omni-connector.md create mode 100644 .claude/skills/review-pr/references/modules/profiling.md create mode 100644 .claude/skills/review-pr/references/modules/quantization.md create mode 100644 .claude/skills/review-pr/references/modules/stage-runtime.md create mode 100644 .claude/skills/review-pr/references/process/design-contracts.md create mode 100644 .claude/skills/review-pr/references/process/general-checks.md create mode 100644 .claude/skills/review-pr/references/process/review-execution.md create mode 100644 .claude/skills/review-pr/references/process/review-routing.md diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md new file mode 100644 index 000000000..005a06f87 --- /dev/null +++ b/.claude/skills/review-pr/SKILL.md @@ -0,0 +1,231 @@ +--- +name: review-pr +description: Review pull requests and local branches for vllm-project/vllm-omni with a frozen snapshot, module-design ownership, feature-design overlays, targeted validation, and concise evidence-backed findings. Use for default, detailed, or repeat maintainer reviews; checking correctness, compatibility, tests, benchmarks, model additions, distributed changes, or breaking behavior; and identifying or explicitly requesting the most relevant code-owner reviewers. Use precheck-pr instead for an author's pre-submit self-check. +--- + +# Review vLLM-Omni Pull Requests + +Review like a maintainer: direct, selective, and focused on issues that CI does +not prove. Prefer a few high-confidence findings over exhaustive commentary. +Zero findings is a valid result. + +## Quality contract + +Make every finding: + +- **Correct:** prove a reachable failure, not a suspicion. +- **Prioritized:** lead with merge blockers and high-impact defects. +- **Actionable:** identify the smallest safe fix direction. +- **Evidence-based:** cite code, tests, docs, CI, or measurements. +- **Concise:** avoid review templates and repeated summaries. +- **Calibrated:** match severity to user and maintainer impact. + +Do not report unrelated backlog, style already enforced by pre-commit, or a +missing test that would not protect changed behavior. + +## Select the input and depth + +Use `vllm-project/vllm-omni` as the base repository. Accept its forks and local +checkouts; use another skill for unrelated repositories. + +| Input | Review surface | +| --- | --- | +| PR number or URL | Frozen PR metadata, full diff, and relevant threads. | +| Local branch/worktree | Frozen target-base SHA through committed, staged, unstaged, and in-scope untracked changes. | +| Pre-filled context | Reuse supplied metadata; fetch only missing facts and the full diff. | + +Default to maintainer brevity. A detailed or audit request expands coverage and +lists `path:line` findings, but keeps the same confidence and severity bar. + +## Reference guide + +Load references in review order: process, one primary module contract, matching +feature designs, evidence checks, then delivery. Every file is linked directly +below; do not load unrelated module references. + +Each concise reference links to the maintained +[vLLM-Omni documentation](https://docs.vllm.ai/projects/vllm-omni/en/latest/). +For branch-specific behavior, inspect the matching `docs/` file in the reviewed +checkout first; use the published latest docs for current guidance and discovery. +If docs and live code disagree, verify the code/tests and report the drift. + +### Review process + +| Reference | Read when | +| --- | --- | +| [review-execution.md](references/process/review-execution.md) | Every review; freeze inputs, inspect safely, and deliver against the same snapshot. | +| [general-checks.md](references/process/general-checks.md) | Every review; apply repository-wide correctness and evidence rules. | +| [design-contracts.md](references/process/design-contracts.md) | Every production review; resolve branch-local module and feature design status. | +| [review-routing.md](references/process/review-routing.md) | After the diff census; select one primary module and conditional overlays. | + +### Primary module contract + +| Reference | Read when | +| --- | --- | +| [entrypoints.md](references/modules/entrypoints.md) | Offline/CLI/API ingress, validation, rendering, streaming, or sessions change. | +| [configuration.md](references/modules/configuration.md) | Config construction, deploy/stage schema, defaults, registry, or topology changes. | +| [input-output-modality.md](references/modules/input-output-modality.md) | Requests, messages, serialization, output types, accumulation, or completion change. | +| [error-contracts.md](references/modules/error-contracts.md) | Error classification, fatality, propagation, sanitization, or rendering changes. | +| [engine-orchestration.md](references/modules/engine-orchestration.md) | Cross-stage routing, request state, output ordering, RPC correlation, or terminal convergence changes. | +| [stage-runtime.md](references/modules/stage-runtime.md) | Placement, startup, readiness, replica identity, affinity, membership, or shutdown changes. | +| [omni-connector.md](references/modules/omni-connector.md) | Cross-stage/process/device/node transport or synchronization changes. | +| [model-integration.md](references/modules/model-integration.md) | Registration, preprocessing, loading, runners, or model-specific execution changes. | +| [ar-runtime.md](references/modules/ar-runtime.md) | AR scheduling, request/cache state, adapters, workers, or upstream vLLM semantics change. | +| [diffusion.md](references/modules/diffusion.md) | Diffusion runtime, models, batching, parallelism, or offload changes. | +| [execution-platforms.md](references/modules/execution-platforms.md) | Hardware selection, capabilities, vendor workers, kernels, or patches change. | +| [cache-management.md](references/modules/cache-management.md) | Cache identity, reuse, validity, reset, eviction, or teardown changes. | +| [quantization.md](references/modules/quantization.md) | Quantization selection, checkpoint metadata, layer mapping, precision, or constraints change. | +| [observability.md](references/modules/observability.md) | Metrics, logs, units, labels, correlation, or lifecycle changes. | +| [profiling.md](references/modules/profiling.md) | Profiling instrumentation, traces, start/stop lifecycle, or overhead changes. | +| [benchmarking.md](references/modules/benchmarking.md) | Benchmark workload, metric calculation, CLI, or result metadata changes. | + +### Feature-design overlays + +| Reference | Read when | +| --- | --- | +| [runtime-stage-execution.md](references/features/runtime-stage-execution.md) | Disaggregated inference, async chunk/output/materialization, or prefix caching changes. | +| [communication.md](references/features/communication.md) | A concrete OmniConnector backend or its deployment contract changes. | +| [diffusion-acceleration.md](references/features/diffusion-acceleration.md) | Diffusion parallelism, attention, quantization, cache, batching, or offload changes. | +| [infrastructure-performance.md](references/features/infrastructure-performance.md) | Metrics infrastructure or documented speech optimization stacks change. | + +### Evidence and quality checks + +| Reference | Read when | +| --- | --- | +| [model-addition-checklist.md](references/checks/model-addition-checklist.md) | A model, architecture, loader, processor, registry, or stage config is added. | +| [perf-verification.md](references/checks/perf-verification.md) | The PR makes a latency, throughput, memory, or quality claim. | +| [test-quality-evaluation.md](references/checks/test-quality-evaluation.md) | Tests change, are absent for risky code, or may not exercise production behavior. | +| [tests-docs-checklist.md](references/checks/tests-docs-checklist.md) | Coverage, CI markers, examples, user docs, or PR evidence need review. | +| [verification.md](references/checks/verification.md) | Hardware, a server, or a runnable affected path is available for active verification. | + +### Delivery and reviewer coordination + +| Reference | Read when | +| --- | --- | +| [maintainer-style-study.md](references/delivery/maintainer-style-study.md) | Findings are ready for concise maintainer-style delivery. | +| [review-requests.md](references/delivery/review-requests.md) | The user asks to identify, suggest, request, or ping code-owner reviewers. | + +## Workflow + +### 1. Freeze and report the snapshot + +Pin the base and head before reading source or running validation. Within 60 seconds, +report the pinned head, CI, mergeability, and preliminary findings in the host +conversation. Do not wait for CI or post this update to GitHub. + +If the target changes while fetching, discard the evidence and retry once. If +it changes again, report the churn and wait for a stable target. + +For a trusted PR head, materialize the pinned head in an isolated detached +worktree. A worktree freezes identity but is not a security sandbox. Treat fork +heads as untrusted unless the user and environment policy explicitly establish +otherwise: execute them only in a disposable, secret-free sandbox with restricted +filesystem, network, and resources; without one, use static SHA-addressed reads +and CI evidence only. For a local review, freeze the committed, index, worktree, +and NUL-safe in-scope untracked contents. Follow +[review-execution.md](references/process/review-execution.md) for trust gates, +state fingerprints, and byte-for-byte staleness checks. + +### 2. Build the diff census + +Group files into production code, tests, docs, configuration, build/CI, and +generated artifacts. Map each changed production file and test group to the PR +goal. Compare the title/body claims with the actual diff; use linked issues only +when they define the contract or reproduction. + +Mark unrelated scope and unexplained generated artifacts. Do not infer behavior +from the PR description without tracing the live code. + +### 3. Route from the live behavior + +Trace each claimed behavior through the changed producer to its live consumer, +then use [design-contracts.md](references/process/design-contracts.md) and +[review-routing.md](references/process/review-routing.md) to select one primary +module contract, a second only for a real documented cross-boundary call path, +and every matching feature-design and evidence overlay. Treat titles and paths +as hints; live behavior and the frozen head's current design metadata are +authoritative. For docs-, tests-, or CI-only changes, route to the production +contract they protect or use only the applicable evidence checks. + +### 4. Run the blocker scan + +Apply every category in [general-checks.md](references/process/general-checks.md) before +lower-priority comments. + +For each changed value or behavior, trace: + +```text +public ingress -> validation/defaulting -> producer -> transformations + -> stage/worker/connector boundary -> final consumer -> terminal cleanup +``` + +Cover every applicable offline/online, streaming/non-streaming, sync/async, +feature-on/off, topology, and compatibility path. Search bounded callers and +sibling implementations rather than assuming the changed hunk is the only path. + +### 5. Apply module and feature contracts + +Apply the reference set selected in step 3 and any matching repo-local skill. +Read the exact module and feature pages in the frozen head, including status, +ownership boundary, dependencies, candidate invariants, safe-change guide, and +promotion gate. Candidate or draft rules are questions, not blockers, unless +current code, tests, or policy enforce them. Inspect both sides of any config, +registry, serialization, connector, cache, or stage boundary. + +When a diff adds or expands a helper, class, fallback, compatibility branch, or +public behavior, run a subtraction pass: remove out-of-scope behavior and check +whether each new abstraction can be deleted, merged, moved, or inlined. + +### 6. Verify the changed path + +Before each validation group, verify the frozen SHA plus the tracked, index, +untracked, and ignored-file fingerprint, or recreate a pristine snapshot. On a +trusted head or inside the required sandbox, run an import/version preflight, +then the narrowest relevant tests and low-cost static checks. Bind every result +to the head SHA, snapshot fingerprint, and environment fingerprint. Never run +imports, tests, builds, hooks, or repo-configurable tooling from an untrusted +head on the reviewer host. + +- Treat CI as status evidence; inspect only the first overlapping failure. +- For docs-only changes, use diff hygiene, links/build checks, and bounded live + contract verification instead of dependency setup or pytest. +- For hardware-dependent paths, run available static/CPU checks and name the + exact GPU/NPU gap. Never simulate device evidence. +- For performance or accuracy claims, require comparable base/head runs with + the same environment, workload, warmup, repetitions, and quality criteria. + +Stop when each changed semantic path has a supported finding or an explicit +no-issue conclusion. Do not search further only to increase confidence. + +### 7. Consolidate and deliver + +Verify each finding against the current diff, deduplicate by root cause, and +order by severity. + +Re-read the remote head and reverify or recreate the pristine validation +snapshot immediately before delivery. If either changed, mark the review stale +and restart from the new snapshot. + +Return findings first. Use +[maintainer-style-study.md](references/delivery/maintainer-style-study.md) to keep them +direct and brief. Each finding must include an exact `path:line`, trigger or +call path, current behavior, impact, and smallest fix direction. If there are +no findings, say so briefly and name material validation gaps. + +Keep the review read-only unless the user explicitly authorizes posting. Do not +submit `APPROVE`, `COMMENT`, or `REQUEST_CHANGES`, add labels, edit code, or push +commits as an implied part of review. + +### 8. Optionally request focused owner reviews + +Only when the user asks to identify or request reviewers, read +[review-requests.md](references/delivery/review-requests.md). Rank path-matched +CODEOWNERS with the frozen module page's owners or required reviewers and +documented governance expertise; propose one to three focused reviewers with an +explicit contract rationale. + +Identifying or suggesting reviewers is read-only. Requesting reviewers or +posting `@mention` comments changes external state and requires explicit user +authorization. When authorized, recheck the head, deduplicate existing +requests, and post at most one consolidated comment. Do not infer this +permission from a request to review the code. diff --git a/.claude/skills/review-pr/agents/openai.yaml b/.claude/skills/review-pr/agents/openai.yaml new file mode 100644 index 000000000..57da62eff --- /dev/null +++ b/.claude/skills/review-pr/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "vLLM-Omni PR Review" + short_description: "Review vLLM-Omni PRs and route owner requests" + default_prompt: "Use $review-pr to review this vLLM-Omni pull request with owner-specific rules and evidence-backed findings." diff --git a/.claude/skills/review-pr/references/checks/model-addition-checklist.md b/.claude/skills/review-pr/references/checks/model-addition-checklist.md new file mode 100644 index 000000000..b60fc936e --- /dev/null +++ b/.claude/skills/review-pr/references/checks/model-addition-checklist.md @@ -0,0 +1,121 @@ +# Model Addition Checklist + +Read this reference when a PR adds or expands a model architecture, pipeline, +loader, processor, registry entry, or stage configuration. + +Official docs: [model contribution guides](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/model/), +[adding an omni model](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/model/adding_omni_model/), +[adding a diffusion model](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/model/adding_diffusion_model/), +[adding a TTS model](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/model/adding_tts_model/), +[supported models](https://docs.vllm.ai/projects/vllm-omni/en/latest/models/supported_models/), +[diffusion features](https://docs.vllm.ai/projects/vllm-omni/en/latest/user_guide/diffusion_features/), +and [feature compatibility](https://docs.vllm.ai/projects/vllm-omni/en/latest/user_guide/feature_compatibility/). + +## Match claims to the diff + +- Reconcile every claimed architecture, modality, variant, feature, and example + with changed files and registry entries. +- Resolve config model types, architecture names, callable paths, exports, and + dependency extras against live symbols. +- Check sibling generations sharing an architecture name are disambiguated by + a stable config predicate rather than import order or an accidental default. + +## Close the integration chain + +Trace: + +```text +public model id -> config/pipeline selection -> registry -> loader/processor + -> stage inputs -> model execution -> stage/public output +``` + +Verify optional dependencies fail with an actionable message, weight names and +dtypes map correctly, and each advertised serving mode reaches the production +dispatcher. Confirm outputs are non-empty and valid for their modality, shape, +sample rate, or response schema. + +## Keep support documentation truthful + +- Build an evidence-backed inventory of the new model's exact architecture and + checkpoint identifiers, modalities and tasks, hardware backends, serving + modes, and supported acceleration, parallelism, cache, offload, and + quantization features. Treat untested support as unknown, not supported. +- Update `docs/models/supported_models.md` with the architecture, model family, + example checkpoints, and only the backends validated by the PR. +- For a diffusion model, add it to the applicable ImageGen, VideoGen, or + AudioGen table in `docs/user_guide/diffusion_features.md`. Fill every feature + column according to its legend and document partial support or constraints. +- Update `docs/user_guide/feature_compatibility.md` when the model changes valid + feature combinations, configuration constraints, examples, or limitations, + and update any other branch-local support table affected by the claims. +- Cross-check every documented support mark against the registry, configuration, + production-path tests, and PR evidence; do not copy a sibling model's support + claims without validation. + +## Ship the model recipe + +- Require every new model PR to add or update one model-family recipe under + `recipes//` and its row in the `recipes/README.md` Available Recipes + table. Follow [`recipes/TEMPLATE.md`](../../../../../recipes/TEMPLATE.md) and + the layout rules in + [`recipes/README.md`](../../../../../recipes/README.md); the + external recipes repository is a structural reference, not a substitute. +- Include only tested task, serving-mode, and hardware configurations. Keep the + exact model identifiers, flags, commands, verification, feature support, + constraints, and limitations consistent with code, tests, examples, and the + support documentation. +- Cover each validated platform in its own hardware section. Use one recipe per + model family by default; justify a split when configurations cannot be kept + clear in one file, and do not add placeholder sections for untested platforms. + +## Compare the reference implementation + +Require every new model PR to include a head-versus-reference accuracy or +quality and performance comparison: + +- Name the canonical implementation and pin its repository revision, package + version, configuration, and checkpoint. If it cannot run, explain why and use + the closest runnable reference; do not silently omit the comparison. +- Match inputs or dataset, seed, output settings, precision or quantization, + hardware, software, warmup, repetitions, synchronization, and timing and + memory scope. Include exact commands and environment details for both sides. +- Compare accuracy or output quality with a task-appropriate metric and explicit + tolerance. When no reliable metric exists, provide paired fixed-input outputs, + artifacts, and evaluation criteria, and explain expected numerical or + qualitative differences. +- Report end-to-end latency, task-appropriate throughput, peak device memory, + run-to-run variability, and head/reference ratios at equivalent output quality. +- Prefer a timing split for preprocessing or encoding, each model stage, + inter-stage transfer or communication, decoding or postprocessing, and output + assembly. Define the boundaries, account for overlap, reconcile the split with + end-to-end time, and explain any unavailable stage measurement. + +Use [perf-verification.md](perf-verification.md) for the comparison protocol and +repository benchmark and profiling links. + +## Remove accidental surface + +Search bounded call sites for: + +- inference-dead training `forward()` paths, unused factories/wrappers, never-set + branch keys, and immediately discarded parameters; +- duplicate payload strings, validation, or shape coercion across stages; +- compatibility aliases in brand-new code without an existing caller; +- private symbols re-exported as public API or module-level side effects. + +Keep code only when it has a distinct live caller, invariant, or compatibility +contract. Prefer one typed producer-consumer schema to repeated string keys. + +## Require proportionate evidence + +- Run a representative production-path inference and assert output content, not + only process survival. +- Require profiling or A/B tables when the PR makes performance, memory, + precision, or quality claims, or when a suspected hot-path/device bug needs + that evidence. Do not impose fixed utilization or regression thresholds that + the repository does not define. +- Require focused registry/config/loading tests, a representative E2E path, and + user-facing model/usage documentation. + +Use [diffusion.md](../modules/diffusion.md) for diffusion models and +[perf-verification.md](perf-verification.md) for quantitative claims. diff --git a/.claude/skills/review-pr/references/checks/perf-verification.md b/.claude/skills/review-pr/references/checks/perf-verification.md new file mode 100644 index 000000000..bbfe5a147 --- /dev/null +++ b/.claude/skills/review-pr/references/checks/perf-verification.md @@ -0,0 +1,51 @@ +# Performance and Accuracy Verification + +Activate this reference when the PR adds a model or claims or intentionally +changes latency, throughput, memory, scaling, precision, or output quality. + +Official docs: [profiling](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/profiling/), +[serving benchmarks](https://docs.vllm.ai/projects/vllm-omni/en/latest/cli/bench/serve/), +and [metrics](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/metrics/). + +## Comparable A/B contract + +Compare frozen base and head for an existing path. For a new model absent from +base, compare head with a pinned canonical reference implementation. Use the same: + +- hardware, driver, software versions, model/checkpoint, and dependencies; +- input or dataset, seed, precision, batch/concurrency, topology, and feature + flags; +- warmup, measured repetitions, synchronization, timing scope, and memory + collection method. + +Keep exact commands and report variability rather than a cherry-picked run. +Use the repository's benchmark when it covers the claim; otherwise use the +smallest reproducible workload that reaches the changed production path. + +## Evidence table + +| Dimension | Typical evidence | +| --- | --- | +| Latency | End-to-end latency and, when available, TTFT plus a defined per-stage and transfer-time split. | +| Throughput/scaling | Requests, tokens, frames, or audio duration per second across the claimed concurrency/topology. | +| Memory | Peak allocated/reserved device memory and OOM boundary if relevant. | +| Quality/accuracy | Repository metric or known-good output comparison with an explicit tolerance; paired samples when no metric exists. | + +Every optimization needs correctness or quality evidence. A faster result on a +different workload, precision, seed, or topology is not a valid comparison. + +## Reviewer verification ladder + +1. Run base/head A/B on suitable hardware when affordable. +2. If only one side can run, verify the script and request a comparable pair. +3. If hardware or assets are unavailable, audit methodology, code path, and + contributor evidence; name the exact unverified claim. + +Classify discrepancies before reporting: implementation regression, benchmark +bug, environmental drift, noise, or unsupported claim. Explain material +regressions against the PR's stated goal or repository contract; do not invent +universal percentage thresholds. + +Report claimed and measured values together, bind them to the frozen SHAs and +environment, and keep unavailable hardware as a validation gap rather than a +fabricated pass. diff --git a/.claude/skills/review-pr/references/checks/test-quality-evaluation.md b/.claude/skills/review-pr/references/checks/test-quality-evaluation.md new file mode 100644 index 000000000..fb1427f01 --- /dev/null +++ b/.claude/skills/review-pr/references/checks/test-quality-evaluation.md @@ -0,0 +1,39 @@ +# Test Quality Evaluation + +Read this reference when tests change, a risky production path has no test, or +a test-only PR may not prove the intended behavior. + +Official docs: [test system overview](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/test_system_overview/) +and [test writing guide](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/test_writing_guide/). + +## Static proof check + +For each changed semantic path, ask whether the test would fail if that behavior +were reverted or broken. Check: + +- assertions pin the contract, not only non-null output, logs, or “no crash”; +- the production dispatcher, registry, connector, or scheduler is reached + instead of the changed behavior being mocked away; +- fakes preserve relevant types, MRO, shapes, devices, async behavior, and + lifecycle transitions; +- seeds, ordering, timing, synchronization, external services, and numeric + tolerances are deterministic or explicitly controlled; +- normal, invalid, boundary, feature-off, failure/cancellation, and regression + paths are covered where the diff changes them; +- run-level and domain markers place hardware/model tests in the intended CI + lane without silently skipping the contract. + +Map source symbols to tests with bounded `rg` searches; do not assume the test +tree mirrors production paths. + +## Runtime check + +Run the narrowest affected tests supported by the environment. Record skipped +hardware/model cases as gaps. Classify failures as code, test, infrastructure, +or flaky before turning them into findings; one passing rerun does not erase a +flakiness signal. + +Keep grades or matrices internal. Report only concrete code bugs or the one or +two test defects that leave changed behavior materially unprotected. Use +[tests-docs-checklist.md](tests-docs-checklist.md) for CI and documentation +coverage. diff --git a/.claude/skills/review-pr/references/checks/tests-docs-checklist.md b/.claude/skills/review-pr/references/checks/tests-docs-checklist.md new file mode 100644 index 000000000..e03a4896b --- /dev/null +++ b/.claude/skills/review-pr/references/checks/tests-docs-checklist.md @@ -0,0 +1,48 @@ +# Tests and Documentation Checklist + +Use this reference to connect changed behavior to coverage, CI selection, +examples, documentation, and reproducible PR evidence. + +Official docs: [test writing guide](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/test_writing_guide/), +[test execution guide](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/test_execution_guide/), +and [documentation guide](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/DOCS_GUIDE/). + +## Coverage packet + +For each high-risk change, record a compact internal packet: + +```text +change and failure risk -> existing unit/E2E coverage -> uncovered boundary + -> smallest test and stable assertion that closes the gap +``` + +Prefer a regression test that fails on the frozen base for bug fixes. Check +numeric tolerances, deterministic inputs, realistic mocks, hardware/domain +markers, and Buildkite/CI wiring using the documentation present on the reviewed +branch. A skipped test is not a pass. + +## Documentation sync + +Require user-facing updates when the diff changes a model, feature, CLI/API, +config key, default, compatibility behavior, or supported platform. Verify: + +- the actual docs navigation and support tables used by the branch; +- exact identifiers, defaults, constraints, known limitations, and migration; +- a minimal runnable example for newly exposed behavior; +- examples and docs agree on commands, parameters, output schema, and supported + modes; +- links and the bounded live contract build or resolve correctly. + +Do not request docs for internal behavior with no user-visible contract. + +## PR evidence + +For contributor-run tests or benchmarks, require enough provenance to evaluate +the claim: hardware, relevant software versions, exact commands, inputs/model, +result, and approximate runtime. Performance or accuracy evidence must compare +base and head under equivalent conditions; use +[perf-verification.md](perf-verification.md). + +Turn a missing test or document into a finding only when it protects or explains +changed behavior. Name the smallest useful addition rather than requesting a +broad coverage expansion. diff --git a/.claude/skills/review-pr/references/checks/verification.md b/.claude/skills/review-pr/references/checks/verification.md new file mode 100644 index 000000000..956abd61d --- /dev/null +++ b/.claude/skills/review-pr/references/checks/verification.md @@ -0,0 +1,36 @@ +# Active Verification + +Read this reference when a runnable affected path and suitable local or remote +hardware are available. Never claim device evidence from static analysis or a +simulated backend. + +Official docs: [installation](https://docs.vllm.ai/projects/vllm-omni/en/latest/getting_started/installation/) +and [test execution guide](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/test_execution_guide/). + +## Choose the narrowest level + +| Available environment | Verification | +| --- | --- | +| Matching accelerator, model/assets, and server | Targeted unit/integration test plus one representative E2E request. | +| Matching accelerator without serving setup | Targeted model/runtime test and output or metric inspection. | +| CPU/static environment only | Import/version preflight, focused CPU tests, config/registry resolution, and static contract checks. | +| Missing dependencies or inaccessible hardware | Record the exact gap and evaluate contributor/CI evidence. | + +## Execute safely + +1. Bind the run to the frozen head SHA and record Python, platform, dependency, + model, device, and relevant environment fingerprints. +2. Run the smallest affected unit test before an E2E workload. +3. Exercise the changed public or production-dispatch path with bounded inputs. +4. Compare actual output, metrics, cleanup, and exit behavior with the PR claim. +5. For base/head claims, use the equivalent A/B protocol in + [perf-verification.md](perf-verification.md). + +Avoid disturbing shared servers, ports, models, or accelerators without user +authorization. Do not install large dependencies, download gated assets, or +start remote workloads merely because hardware might exist. + +Report the command, environment, result, and any skipped path. Classify failures +as change-induced, pre-existing, infrastructure, environment, or flaky before +making a finding. A hardware gap lowers confidence; it is not automatic proof +of a defect. diff --git a/.claude/skills/review-pr/references/delivery/maintainer-style-study.md b/.claude/skills/review-pr/references/delivery/maintainer-style-study.md new file mode 100644 index 000000000..c04cef512 --- /dev/null +++ b/.claude/skills/review-pr/references/delivery/maintainer-style-study.md @@ -0,0 +1,105 @@ +# Maintainer Review Style + +Use this reference only when turning proved findings into comments. The baseline +maintainer samples favor short, direct comments and reserve long explanations +for architecture or non-obvious failure paths. + +Official docs: [contributing and review process](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/). + +## Evidence basis + +This calibration used 928 PRs merged from 2026-05-10 through 2026-08-08. +Candidates were ranked by substantive human inline-review threads, then by +distinct reviewers and review rounds. Author self-comments, bots and AI +reviewers, and bodyless approvals were excluded. Counts selected the sample; +the practices below came from reading root comments and follow-up replies. + +The main set contains the six leaders after filtering; three other high-ranked +PRs add core-runtime and benchmark coverage. All were merged. + +| PR | Review boundary | +| --- | --- | +| [#2906](https://github.com/vllm-project/vllm-omni/pull/2906) | New TTS model, shared utilities, recipes, and request state. | +| [#3907](https://github.com/vllm-project/vllm-omni/pull/3907) | Full-duplex API, session lifecycle, concurrency, and realtime evidence. | +| [#2162](https://github.com/vllm-project/vllm-omni/pull/2162) | Diffusion world model, OpenPI serving, tests, examples, and docs. | +| [#3454](https://github.com/vllm-project/vllm-omni/pull/3454) | Diffusion model, generic CLI surface, loaders, and configuration scope. | +| [#4425](https://github.com/vllm-project/vllm-omni/pull/4425) | Structured configuration, typing, defaults, and compatibility. | +| [#5397](https://github.com/vllm-project/vllm-omni/pull/5397) | Distributed offload, collectives, failure handling, and load semantics. | +| [#4281](https://github.com/vllm-project/vllm-omni/pull/4281) | Composable parallelism, configuration contracts, and examples. | +| [#3628](https://github.com/vllm-project/vllm-omni/pull/3628) | Stage metrics, benchmark definitions, opt-in API behavior, and cleanup. | +| [#3855](https://github.com/vllm-project/vllm-omni/pull/3855) | Stage runtime, thread safety, startup, and compatibility. | + +Historical comments calibrate delivery; they do not create merge policy. Live +code, tests, repository docs, and the current `CODEOWNERS` file remain +authoritative. When refreshing this study, promote a practice only when it +recurs across at least three PRs or is supported by current repository guidance. + +## Recurring review behavior + +- Start from a changed contract, not a style preference. State the exact + request, configuration, topology, or failure path that triggers the issue. +- Protect repository boundaries. Keep model-specific behavior in its domain, + move genuinely reusable logic to a shared module, and reuse existing loaders, + protocols, definitions, and config sources instead of bypassing them. +- Challenge unrelated scope and abstractions. Ask why a new flag, config, class, + helper, fallback, or file is needed when an existing path appears sufficient. +- Trace concurrency and terminal paths: overlapping requests, disconnect, + cancellation, partial failure, stale replies, collective schedule mismatch, + cleanup, and retry. Prefer an explicit startup error for unsupported + combinations over silently running with incomplete state or wrong weights. +- Preserve user-visible behavior across variants. An accepted CLI/API/config + field must be applied, explicitly rejected, or visibly warned about; compare + normal and headless, offline and online, and streaming and non-streaming paths. +- Ask for evidence at the affected boundary: a regression test using the real + tensor or protocol shape, correct pytest markers so CI runs it, comparable A/B + performance results, or an SLO-oriented latency/quality measurement. +- Keep interfaces explicit: avoid `Any` and opaque strings where a precise + union, enum, or typed envelope exists; remove dead code and duplicated + constants; validate requests before expensive execution. +- Document module scope and non-obvious invariants. Update examples, recipes, + supported-model/feature docs, and hardware assumptions when users rely on + them. +- When expertise crosses an ownership boundary, ask a named owner to inspect a + specific file or contract and explain why. Do not send a generic `PTAL` list. +- Keep non-blocking cleanup in a linked follow-up only after the current PR is + safe. If a revision fixes only part of a failure chain, continue the existing + thread with the remaining trigger instead of opening a duplicate finding. + +## Write the finding + +- Lead with the issue or decisive question; omit a review preamble. +- Name the concrete trigger and impact, then the smallest fix direction. +- Keep obvious fixes to one sentence. Use more detail only when the call path or + ownership boundary is otherwise unclear. +- Prefer one root-cause comment over repeated symptom comments. +- Hedge only when evidence is genuinely incomplete; mark that item as a + question or validation gap rather than overstating severity. +- Say briefly that no findings were found when the review is clean; zero + comments is a valid outcome when posting is authorized. + +Useful forms: + +```text +This leaves allocated when . Release it in +the shared terminal cleanup path. +``` + +```text +Does this producer also update ? It still reads the old field here. +``` + +```text +With , this bypasses , so . +Reuse that path or reject this combination before startup. +``` + +```text +@, could you check ? This changes used by +. +``` + +Use a priority label only for a proved high-impact defect. Avoid generic praise, +routine “Nit:” prefixes, decorative severity labels, audit templates, rule IDs, +comment-count narration, and “I left a few comments.” Keep approvals, review +events, reviewer requests, mentions, and GitHub replies subject to the user's +explicit authorization. diff --git a/.claude/skills/review-pr/references/delivery/review-requests.md b/.claude/skills/review-pr/references/delivery/review-requests.md new file mode 100644 index 000000000..e3dbe9d32 --- /dev/null +++ b/.claude/skills/review-pr/references/delivery/review-requests.md @@ -0,0 +1,99 @@ +# Focused Code-Owner Review Requests + +Read only when the user asks to identify, suggest, request, add, or ping +reviewers. Finding candidates is read-only; GitHub review requests and +`@mention` comments are external writes. + +Owner sources, in order: + +1. the reviewed head's + [CODEOWNERS](https://github.com/vllm-project/vllm-omni/blob/main/.github/CODEOWNERS) + for path ownership; +2. the active module design page's `owners` and `required_reviewers` metadata + for contract ownership and mandatory design review; and +3. documented + [governance expertise](https://docs.vllm.ai/projects/vllm-omni/en/latest/community/governance/) + for specialization and tie-breaking. + +Resolve all three from the reviewed head when available because ownership +changes over time. Use published docs only to discover a page absent from an +older checkout, never to override the frozen head, and do not copy a static +owner list into review output. CODEOWNERS remains the path authority; module +metadata identifies contract experts and required design reviewers. + +## Rank candidates + +1. Freeze the PR head and collect changed files, author, requested reviewers, + submitted reviews, and existing comments. +2. Resolve the **last matching CODEOWNERS rule** for each changed file. Do not + union an earlier broad rule with a later specific rule. +3. Resolve the primary module and feature overlays through + [design-contracts.md](../process/design-contracts.md). Read the active module + page's status, `owners`, and `required_reviewers`; do not treat owners on an + archived, superseded, or unrelated page as current authority. +4. Group files by the selected primary owner and the exact module or feature + contract needing review. Weight production owners and live consumers above + tests/docs/CI owners and the repository-wide fallback. Include an applicable + `required_reviewers` entry even when it is not the path owner, and label why. +5. Use documented governance specialties only to choose among matching owners: + for example serving versus configuration, diffusion model versus cache or + parallelism, TTS versus generic model execution, or CI versus tests. +6. Exclude the PR author, bots, users who already completed the needed review, + and duplicate pending requests. Do not infer availability or invent + expertise from a username. +7. Select one reviewer for a single-owner change and at most three for a real + cross-module change. If more are plausible, rank them but do not ping a + broad group; ask the user before exceeding three. + +If every specific owner is excluded, consider one repository-wide fallback +owner and label that choice as a fallback rather than a path match. Never fan +out to the full fallback list. Prefer a qualified owner who covers multiple +critical path groups over separate reviewers for each file. If CODEOWNERS is +missing or invalid, state that gap and use governance only as a labeled fallback. + +Recent file history may break a tie only when CODEOWNERS and governance remain +ambiguous. Never use commit count alone to override an explicit current owner. + +## Preview before writing + +Show the user the exact candidate set and rationale: + +```text +@owner — ; please verify . +``` + +Draft one consolidated comment tied to the frozen head: + +```text +Requesting focused review for ``: + +- @owner1: could you check the in ``? +- @owner2: could you check the in ``? + +Relevant validation: . +``` + +Mention only owners being requested. Keep the reason specific enough that each +person knows what decision or invariant needs attention; do not post a generic +“please review” or a generated ownership audit. + +## External-write gate + +- “Who should review?”, “suggest reviewers”, or “draft a request” authorizes + only the preview. +- “Request/add these reviewers” authorizes the GitHub review-request mutation. +- “Ping/comment/tag the owners” authorizes the consolidated comment. +- When the user explicitly asks to request reviews **with an owner comment**, + both writes are authorized. Obey narrower wording when only one action is + requested. + +Immediately before an authorized write, verify the PR is still open, the head +SHA is unchanged, and the candidate is not the author or already satisfied. +Report draft/WIP state and avoid notifications unless the user explicitly wants +reviewers requested before ready-for-review. + +Use the GitHub connector for reviewer requests and the single top-level comment; +use `gh` only when connector coverage is unavailable. Do not submit an approval, +request-changes event, label, assignment, or multiple comments as part of this +action. After writing, reread the PR and report the reviewers requested and the +comment URL. If the head changed, discard the draft and rerank before posting. diff --git a/.claude/skills/review-pr/references/features/communication.md b/.claude/skills/review-pr/references/features/communication.md new file mode 100644 index 000000000..265098f91 --- /dev/null +++ b/.claude/skills/review-pr/references/features/communication.md @@ -0,0 +1,24 @@ +# Communication Features + +Use with the OmniConnector, stage-runtime, configuration, and I/O module +contracts. Load the concrete backend design from the reviewed head: + +| Backend | Design | +| --- | --- | +| Mooncake store | [MooncakeStoreConnector](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/omni_connectors/mooncake_store_connector/) | +| Mooncake transfer engine | [MooncakeTransferEngineConnector](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/omni_connectors/mooncake_transfer_engine_connector/) | +| Mori transfer engine | [MoriTransferEngineConnector](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/omni_connectors/mori_transfer_engine_connector/) | +| Shared memory | [SharedMemoryConnector](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/omni_connectors/shared_memory_connector/) | +| Yuanrong store | [YuanrongConnector](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/omni_connectors/yuanrong_connector/) | +| Yuanrong transfer engine | [YuanrongTransferEngineConnector](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/omni_connectors/yuanrong_transfer_engine_connector/) | + +## Feature checks + +- Verify dependency, service, device/network, port, memory-pool, and environment + prerequisites with actionable startup errors. +- Match key/metadata construction, sender and receiver roles, allocation, + ownership, completion, and supported fast/serialized paths. +- Exercise timeout, retry, stale sender/buffer reclamation, partial transfer, + cancellation, and connector close semantics. +- Validate the smallest supported same-host or cross-node topology and document + backend limitations and operational diagnostics. diff --git a/.claude/skills/review-pr/references/features/diffusion-acceleration.md b/.claude/skills/review-pr/references/features/diffusion-acceleration.md new file mode 100644 index 000000000..4519a7242 --- /dev/null +++ b/.claude/skills/review-pr/references/features/diffusion-acceleration.md @@ -0,0 +1,26 @@ +# Diffusion Acceleration Features + +Use with the exact diffusion submodule plus platform, cache, quantization, or +benchmarking contracts selected by the live path. + +| Feature family | Designs | +| --- | --- | +| Parallelism | [CFG](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/cfg_parallel/), [Expert](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/expert_parallel/), [HSDP](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/hsdp/), [Pipeline](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/pipeline_parallel/), [Sequence](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/sequence_parallel/), [Tensor](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/tensor_parallel/), [VAE patch](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/vae_parallel/) | +| Attention | [Skip-Softmax](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/skip_softmax/) | +| Quantization | [Quantization](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/quantization/) | +| Cache | [Cache-DiT](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/cache_dit/), [TeaCache](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/teacache/) | +| Batching | [Diffusion Continuous Batching](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/diffusion_continuous_batching/) | +| Offload | [Distributed Layerwise Offload](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/distributed_layerwise_offload/) | + +## Feature checks + +- Verify feature selection, supported models/platforms/topologies, defaults, + dependencies, and every touched feature combination. +- Preserve feature-off and single-rank correctness; reject unsupported + combinations before collectives, loading, or execution begin. +- Trace shapes, shards, ranks, collectives, precision, cache validity, residency, + and per-request state through the selected runtime hooks. +- Compare output quality or accuracy against the disabled/reference path before + accepting latency or memory improvements. +- Require exact commands, topology, environment, warmup, repetitions, and + measurements for performance claims; update feature compatibility docs. diff --git a/.claude/skills/review-pr/references/features/infrastructure-performance.md b/.claude/skills/review-pr/references/features/infrastructure-performance.md new file mode 100644 index 000000000..b61bd7289 --- /dev/null +++ b/.claude/skills/review-pr/references/features/infrastructure-performance.md @@ -0,0 +1,22 @@ +# Infrastructure and Performance Features + +Load the matching design from the reviewed head: + +- [Prometheus Metrics](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/metrics/) + for logger wrapping, gating, lifecycle, stage transfer, and metric definitions. +- [Speech Generation Performance Optimizations](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/qwen3_omni_tts_performance_optimization/) + for stage batching, CUDA graphs, async chunk, streaming, re-prefill, compile, + and deployment evidence. + +## Feature checks + +- Route instrumentation changes through observability or profiling and result + methodology through benchmarking and the performance evidence check. +- Preserve opt-in/default behavior, units, labels, aggregation, timing scope, + lifecycle resets, and bounded overhead. +- For stacked optimizations, measure each increment against the same baseline + and explain interactions, quality guards, regressions, and limitations. +- Require exact model, checkpoint, hardware, software, workload, topology, + concurrency, warmup, repetitions, and production commands. +- Update the design page when metric meaning, supported optimization stack, or + deployment guidance changes. diff --git a/.claude/skills/review-pr/references/features/runtime-stage-execution.md b/.claude/skills/review-pr/references/features/runtime-stage-execution.md new file mode 100644 index 000000000..779cc295c --- /dev/null +++ b/.claude/skills/review-pr/references/features/runtime-stage-execution.md @@ -0,0 +1,24 @@ +# Runtime and Stage Execution Features + +Load the matching design from the reviewed head: + +| Feature | Design | Primary module intersections | +| --- | --- | --- | +| Disaggregated inference | [Design](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/disaggregated_inference/) | Configuration, stage runtime, OmniConnector, I/O contracts | +| Async chunk | [Design](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/async_chunk/) | Model integration, orchestration, stage runtime, connector, I/O | +| Async diffusion output | [Design](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/async_diffusion_output/) | Diffusion runtime, I/O contracts, entrypoints | +| Async Omni output materialization | [Design](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/omni_async_output_materialization/) | AR runtime, model integration, I/O contracts, orchestration | +| Automatic prefix caching | [Design](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/feature/prefix_caching/) | AR runtime, cache management, I/O contracts | + +## Feature checks + +- Confirm the feature's supported models, entrypoints, stage types, topology, + configuration, default, and compatibility limitations. +- Trace enablement from public/config ingress through every participating module + and verify the advertised production dispatch actually selects it. +- Preserve a correct disabled or fallback path and make unsupported combinations + fail or warn as documented. +- Test overlap, ordering, identity, backpressure, cancellation, failure, final + partial output, and cleanup at each asynchronous boundary. +- Update the feature page, support matrices, examples, and recipes when behavior + or coverage changes. diff --git a/.claude/skills/review-pr/references/modules/ar-runtime.md b/.claude/skills/review-pr/references/modules/ar-runtime.md new file mode 100644 index 000000000..98474d721 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/ar-runtime.md @@ -0,0 +1,22 @@ +# Autoregressive Runtime + +Primary design: [Autoregressive Runtime](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/ar_runtime/). + +Use for AR request queues, scheduling state, token and memory budgets, KV/input +readiness, workers, runners, and upstream vLLM request/cache transitions. + +## Contract checks + +- Preserve upstream request-state, scheduling, cache, and preemption semantics + unless an Omni-specific difference is documented and tested. +- Convert modality-specific data through explicit input or output adapters; do + not inject it through unrelated scheduler state. +- Keep workers and model runners focused on assigned execution rather than + admission or cross-stage routing. +- Synchronize token, sequence, batch, and memory budgets with worker-visible + state and prevent starvation, double counting, or stale readiness. +- Preserve legal transitions and one terminal cleanup path across completion, + abort, timeout, preemption, worker failure, and shutdown. + +Test request lifecycle, scheduling decisions, abort, cache state, adapters, and +every affected worker execution mode against the pinned upstream contract. diff --git a/.claude/skills/review-pr/references/modules/benchmarking.md b/.claude/skills/review-pr/references/modules/benchmarking.md new file mode 100644 index 000000000..c0c99cc31 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/benchmarking.md @@ -0,0 +1,24 @@ +# Benchmarking + +Primary design: [Benchmarking](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/benchmarking/). + +Use when benchmark code, workloads, metric calculations, CLI benchmark +entrypoints, or result metadata change. Use the performance check separately +when a PR uses benchmark results as evidence. + +## Contract checks + +- Record model and revision, code SHA, hardware, software, configuration, + workload, precision, topology, concurrency, warmup, repetitions, and + measurement window. +- Validate successful and semantically acceptable outputs before accepting + timing, memory, or scaling results. +- Define every metric's unit, population, aggregation, synchronization, and + timing scope. +- Keep workload parsing and metric computation deterministic and reusable + across base and head. +- Reject comparisons that change material workload or runtime settings without + isolating and explaining that difference. + +Test workload parsing and metric calculations with deterministic fixtures and +publish exact commands plus variability for quantitative claims. diff --git a/.claude/skills/review-pr/references/modules/cache-management.md b/.claude/skills/review-pr/references/modules/cache-management.md new file mode 100644 index 000000000..504a875bc --- /dev/null +++ b/.claude/skills/review-pr/references/modules/cache-management.md @@ -0,0 +1,22 @@ +# Cache Management + +Primary design: [Cache Management](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/cache_management/). + +Use for AR or diffusion cache identity, reusable state, validity, lifecycle, +reset, eviction, and teardown. + +## Contract checks + +- Include every model, request, input, timestep, precision, device, topology, + and feature property that can change a reused value in cache identity. +- Preserve a correct disabled and miss path; never reuse after validity inputs + change. +- Isolate request-scoped state across concurrent requests and define ownership + for model-scoped state. +- Make reset, invalidation, eviction, capacity, and teardown explicit on + completion, cancellation, failure, model reload, and shutdown. +- Keep cache optimizations behind the owning runtime's hooks rather than adding + a second request lifecycle. + +Test hit, miss, invalidation, disabled behavior, concurrency, capacity, reset, +and teardown, with quality comparison when reuse is approximate. diff --git a/.claude/skills/review-pr/references/modules/configuration.md b/.claude/skills/review-pr/references/modules/configuration.md new file mode 100644 index 000000000..bdb3a5159 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/configuration.md @@ -0,0 +1,24 @@ +# vLLM-Omni Configuration + +Primary design: [vLLM-Omni Configuration](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/vllm_omni_config/). + +Use for config construction, deploy YAML, registries, schema, defaults, +aliases, CLI projection, stage config, and topology. Inspect the reviewed +page's status: while its contract remains deferred or draft, current code and +tests—not proposed precedence rules—are authoritative. + +## Contract checks + +- Resolve each supported structured, legacy, CLI, environment, default, alias, + and stage-override producer through its actual runtime consumer. +- Reject unknown or owner-mismatched fields explicitly; keep process-local + runtime objects out of transportable configuration. +- Verify default, explicit, and feature-off values reach every live factory and + consumer without silent reinterpretation. +- Treat stage count, placement, connector, parallelism, device, and memory + fields as one topology contract and reject impossible combinations early. +- Preserve supported construction-path parity until a documented migration + removes a path. + +Require normalization and schema tests, a live-consumer assertion, and docs for +public keys, defaults, constraints, precedence, or migration behavior. diff --git a/.claude/skills/review-pr/references/modules/diffusion.md b/.claude/skills/review-pr/references/modules/diffusion.md new file mode 100644 index 000000000..7dbe91f29 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/diffusion.md @@ -0,0 +1,33 @@ +# Diffusion Module Family + +Primary design: [Diffusion Modules](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/diffusion/). + +Use for diffusion-stage behavior, then load the exact submodule page from the +reviewed head: + +| Submodule | Signals | Design | +| --- | --- | --- | +| Runtime | Admission, scheduling, execution, progress, outputs, cancellation, hooks, cleanup | [Diffusion Runtime](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/diffusion/diffusion_runtime/) | +| Model Integration | Pipelines, registry, loader, adapters, shared layers, model-specific processing | [Diffusion Model Integration](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/diffusion/diffusion_model_integration/) | +| Continuous Batching | Compatibility, request/step batches, per-request progress, capacity | [Continuous Batching](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/diffusion/continuous_batching/) | +| Parallelism | Rank topology, process groups, sharding, collectives, distributed execution | [Parallelism](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/diffusion/parallelism/) | +| Offloader | Residency, transfers, memory accounting, prefetch, teardown | [Offloader](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/diffusion/offloader/) | + +## Family-wide checks + +- Keep one scheduler-owned lifecycle from admission through completion, + cancellation, failure, and cleanup; optional features must use defined hooks. +- Select pipelines through registry and loader contracts instead of scattered + model-name conditionals; keep genuine model differences in model directories. +- Trace latent, conditioning, timestep, generator, shape, layout, dtype, device, + batch expansion, and output conversion through the actual consumer. +- Make batch compatibility and per-request isolation explicit; never associate + output by unstable batch position. +- Derive topology from validated configuration, define every sharding boundary, + keep collectives symmetric, and preserve a supported single-rank path. +- Track one residency owner per offloaded component, wait for transfer readiness, + preserve model state, bound retained copies, and clean up deterministically. + +Load matching feature designs for cache, quantization, parallelism, continuous +batching, or offload. Require feature-off correctness, representative inference, +terminal-path tests, and quality evidence for optimizations. diff --git a/.claude/skills/review-pr/references/modules/engine-orchestration.md b/.claude/skills/review-pr/references/modules/engine-orchestration.md new file mode 100644 index 000000000..c432d480b --- /dev/null +++ b/.claude/skills/review-pr/references/modules/engine-orchestration.md @@ -0,0 +1,23 @@ +# Engine Orchestration + +Primary design: [Engine Orchestration](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/engine_orchestration/). + +Use for request-state creation, cross-stage routing, output ordering, companion +tracking, RPC correlation, cancellation propagation, and terminal convergence. +Orchestration does not own replica placement, stage-process startup, public +rendering, connector transport, or payload and error schemas. + +## Contract checks + +- Keep one owner for cross-stage routing; entrypoints, models, and stage clients + must not independently select the next logical stage. +- Preserve request identity and output order across concurrent stages and + correlate every control or RPC result to the originating request and wave. +- Make terminal state monotonic: no new forwarding after success, abort, + request failure, or fatal engine failure. +- Propagate cancellation and failure to every active stage and release + orchestrator-owned tasks, queues, companions, and request state. +- Preserve shutdown ordering across the orchestrator and stage runtime. + +Test representative multi-stage routing, ordering, correlation, cancellation, +failure, and repeated shutdown. diff --git a/.claude/skills/review-pr/references/modules/entrypoints.md b/.claude/skills/review-pr/references/modules/entrypoints.md new file mode 100644 index 000000000..6e8ffb791 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/entrypoints.md @@ -0,0 +1,25 @@ +# Entrypoints and Serving Boundaries + +Primary design: [Entrypoints and Serving Boundaries](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/entrypoints/). + +Use for offline APIs, CLI and server composition, OpenAI-compatible routes, +request validation, response conversion, streaming, sessions, and engine +handoff. Entrypoints adapt public contracts; they do not own cross-stage +routing, stage lifecycle, configuration precedence, payload schemas, or error +classification. + +## Contract checks + +- Normalize and validate public values once before engine submission. +- Preserve request identity, modality, ordering, and exactly one terminal event + across streaming, disconnect, cancellation, and failure. +- Translate internal outputs and errors explicitly for each advertised offline, + HTTP, SSE, WebSocket, or asynchronous-job path. +- Keep model-specific behavior behind a common adapter or processor instead of + adding model-name policy to generic entrypoints. +- Require API or CLI changes to explain necessity, alternatives, compatibility, + migration, and the supported route or transport matrix. + +Test the affected public protocol, normalization, engine handoff, rendering, +and terminal outcomes. Route orchestration, I/O schema, and semantic error +changes to their module contracts. diff --git a/.claude/skills/review-pr/references/modules/error-contracts.md b/.claude/skills/review-pr/references/modules/error-contracts.md new file mode 100644 index 000000000..d21c28d9a --- /dev/null +++ b/.claude/skills/review-pr/references/modules/error-contracts.md @@ -0,0 +1,24 @@ +# Error Classification, Propagation, and Rendering + +Primary design: [Error Classification, Propagation, and Rendering](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/error_contracts/). + +Use when a change classifies failures, crosses process boundaries, decides +fatal versus request-scoped impact, sanitizes context, or renders offline, +HTTP, SSE, WebSocket, or asynchronous-job errors. The I/O module owns the wire +schema; this module owns the meaning of its error fields. + +## Contract checks + +- Keep request-scoped failure isolated unless the underlying engine is no + longer usable. +- Preserve stable classification and request context across process and + protocol boundaries without exposing prompts, outputs, secrets, or internal + stack details. +- Keep cancellation distinguishable from internal failure. +- Terminate each public transport exactly once with consistent status, body, + event, finish reason, and metrics classification. +- Maintain compatibility across mixed or older producers only when the + reviewed contract defines that behavior. + +Test table-driven classification, wire round trips, fatal/request-scoped +propagation, cancellation, sanitization, and every affected public renderer. diff --git a/.claude/skills/review-pr/references/modules/execution-platforms.md b/.claude/skills/review-pr/references/modules/execution-platforms.md new file mode 100644 index 000000000..749e8db50 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/execution-platforms.md @@ -0,0 +1,23 @@ +# Execution Platforms + +Primary design: [Execution Platforms](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/execution_platforms/). + +Use for hardware capability detection, worker selection, patches, kernels, +attention backends, and vendor-specific configuration or connectors. + +## Contract checks + +- Guard hardware behavior through platform selection or capability detection + and fail clearly when unsupported. +- Keep portable modules free of direct vendor imports when a common selector + exists; verify portable imports without the target accelerator. +- Override only behavior that differs from common or upstream implementations, + and compare sibling worker hooks when inheritance does not provide parity. +- Preserve dtype, layout, rank, stream, synchronization, workspace, + graph-capture, and resource-lifetime contracts. +- Do not let simplified CPU doubles hide platform MRO, initialization, or + dispatch failures. + +Run portable checks first, then the smallest matching-device test in a fresh +environment. Report unavailable hardware as a validation gap, never simulated +device evidence. diff --git a/.claude/skills/review-pr/references/modules/input-output-modality.md b/.claude/skills/review-pr/references/modules/input-output-modality.md new file mode 100644 index 000000000..6b252fa14 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/input-output-modality.md @@ -0,0 +1,23 @@ +# Input, Output, and Modality Contracts + +Primary design: [Input, Output, and Modality Contracts](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/input_output_modality_contracts/). + +Use for requests, prompt and modality types, message schemas, serialization, +output types, accumulation, completion, and compatibility shims crossing +entrypoint, orchestration, stage, connector, or model boundaries. + +## Contract checks + +- Give every boundary value an explicit modality and validated typed schema. +- Preserve request and stage identity across conversion, serialization, + streaming updates, batching, cancellation, and errors. +- Distinguish partial, final, empty, and failed output states for every modality. +- Verify producer and consumer agreement on shape, layout, dtype, device, + ownership, metadata, ordering, and optional or unknown fields. +- Keep internal objects out of public protocols; translate them at the owning + entrypoint. +- Preserve wire compatibility deliberately and give deprecated shims a bounded + migration path. + +Require construction, round-trip, accumulation, completion, rejection, and +compatibility tests at each changed producer-consumer boundary. diff --git a/.claude/skills/review-pr/references/modules/model-integration.md b/.claude/skills/review-pr/references/modules/model-integration.md new file mode 100644 index 000000000..f6f4b738b --- /dev/null +++ b/.claude/skills/review-pr/references/modules/model-integration.md @@ -0,0 +1,25 @@ +# Model Integration + +Primary design: [Model Integration](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/model_integration/). + +Use for model registration, configuration adapters, preprocessing, checkpoint +loading, stage inputs, model-specific hooks, shared layers, runners, and model +execution. Use the diffusion family reference for diffusion models. + +## Contract checks + +- Declare explicitly how the model class, loader, input processor, stage + configuration, and runtime capabilities are selected. +- Preserve weight-name mapping, dtype/device placement, optional-dependency + errors, and checkpoint-specific loading semantics. +- Trace each typed input and output through preprocessing, runner state, model + call, and its actual consumer. +- Reuse upstream vLLM behavior when sufficient; document and test the precise + difference for every override or patch. +- Keep model code out of admission, cross-stage routing, public policy, and + terminal lifecycle ownership. +- Place reusable behavior in shared modules and test it with more than one + integration. + +Require focused registry, loader, processor, runner, and representative model +execution tests. Apply the model-addition check for new or expanded support. diff --git a/.claude/skills/review-pr/references/modules/observability.md b/.claude/skills/review-pr/references/modules/observability.md new file mode 100644 index 000000000..31de19052 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/observability.md @@ -0,0 +1,21 @@ +# Observability + +Primary design: [Observability](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/observability/). + +Use for metrics, logs, correlation fields, and cross-stage request visibility. + +## Contract checks + +- Preserve stable request and stage identity across entrypoints, orchestration, + stages, workers, errors, and terminal cleanup. +- Treat metric names, units, labels, aggregation, and lifecycle as public + contracts; document any semantic change. +- Keep labels bounded: never use request IDs, prompts, outputs, paths, or other + unbounded user values. +- Register and reset metrics exactly once across repeated startup and shutdown, + and keep disabled instrumentation inexpensive. +- Align emitted values with their actual timing and lifecycle owner rather than + reconstructing them from ambiguous client strings. + +Test registration, label sets, units, resets, error classification, and +representative multi-stage emission. diff --git a/.claude/skills/review-pr/references/modules/omni-connector.md b/.claude/skills/review-pr/references/modules/omni-connector.md new file mode 100644 index 000000000..856ab7750 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/omni-connector.md @@ -0,0 +1,24 @@ +# OmniConnector + +Primary design: [OmniConnector](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/omni_connector/). + +Use for connector contracts and model-agnostic data transfer across stages, +processes, devices, or nodes. Connector implementations transport and +synchronize data; they do not choose the next stage or implement model policy. + +## Contract checks + +- Prove producer and consumer agreement on request/stage identity, shape, + layout, dtype, placement, ownership, ordering, and completion. +- Keep backend selection and metadata/key construction explicit; do not silently + reinterpret or drop payloads across SHM, store, network, or vendor paths. +- Bound connect, put/get, retry, synchronization, and readiness waits with + backpressure, cancellation, and actionable errors. +- Prevent route, endpoint, port, key, and buffer collisions across repeated + startup, concurrent requests, and replica replacement. +- Release connections, handles, buffers, listener work, and stale sender state + on success, cancellation, failure, and shutdown. + +Load the matching communication feature design for a concrete backend. Require +focused producer-consumer tests and the smallest runnable multi-process or +multi-node path. diff --git a/.claude/skills/review-pr/references/modules/profiling.md b/.claude/skills/review-pr/references/modules/profiling.md new file mode 100644 index 000000000..6037d9449 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/profiling.md @@ -0,0 +1,21 @@ +# Profiling + +Primary design: [Profiling](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/profiling/). + +Use for opt-in instrumentation and trace collection across supported profiler +backends. + +## Contract checks + +- Keep expensive collection disabled by default and bounded by explicit start, + stop, timeout, and cleanup behavior. +- Preserve scheduling, generated outputs, resource ownership, and concurrency + semantics apart from documented synchronization overhead. +- Include stage, rank, worker, operation, and request correlation where useful + without embedding user payloads or secrets. +- Make repeated start/stop and partial failure safe, including multi-rank trace + naming and artifact ownership. +- Measure disabled and enabled overhead before making performance claims. + +Test the disabled path, repeated lifecycle, failure cleanup, trace context, and +each affected backend. diff --git a/.claude/skills/review-pr/references/modules/quantization.md b/.claude/skills/review-pr/references/modules/quantization.md new file mode 100644 index 000000000..2043f8239 --- /dev/null +++ b/.claude/skills/review-pr/references/modules/quantization.md @@ -0,0 +1,22 @@ +# Quantization + +Primary design: [Quantization](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/quantization/). + +Use for method registration, checkpoint interpretation, layer replacement, +reduced precision, platform capability, and runtime constraints. + +## Contract checks + +- Validate checkpoint metadata, selected method, supported layers, device + capability, required dependencies, and parallel layout before loading. +- Preserve loader and weight-name mapping semantics, dtype, scale/granularity, + zero point, padding, and buffer handling through the real kernel consumer. +- Fail clearly for unsupported combinations; do not silently choose a different + method or precision. +- Keep online and pre-quantized checkpoint paths explicit and ensure patches or + fallbacks do not bypass model-owned loaders. +- Require numerical comparison against a defined reference with explicit + tolerance before treating performance results as valid. + +Test registration, loading, representative layers, metadata, platform guards, +parallel combinations, and numerical correctness separately from performance. diff --git a/.claude/skills/review-pr/references/modules/stage-runtime.md b/.claude/skills/review-pr/references/modules/stage-runtime.md new file mode 100644 index 000000000..2016f8e9f --- /dev/null +++ b/.claude/skills/review-pr/references/modules/stage-runtime.md @@ -0,0 +1,23 @@ +# Stage Runtime and Replica Lifecycle + +Primary design: [Stage Runtime and Replica Lifecycle](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/module/stage_runtime/). + +Use for local or distributed placement, startup, readiness, stage pools, +replica identity and membership, selection, affinity, liveness, draining, and +stage-client or process lifecycle. Cross-stage request policy belongs to engine +orchestration; model scheduling belongs to the owning runtime. + +## Contract checks + +- Acquire, replace, and retire replicas through one stage-runtime lifecycle. +- Preserve stable replica identity and request affinity until termination or a + declared replica loss. +- Make readiness explicit before admission; surface partial-startup and + membership failures without leaving half-initialized capacity. +- Keep local and distributed semantics aligned for supported modes. +- Make drain, shutdown, and cleanup idempotent across repeated signals and + process failure. + +Test startup, readiness, selection, affinity, membership changes, abort, +process loss, draining, and repeated shutdown on affected local/distributed +paths. diff --git a/.claude/skills/review-pr/references/process/design-contracts.md b/.claude/skills/review-pr/references/process/design-contracts.md new file mode 100644 index 000000000..e7a60fa32 --- /dev/null +++ b/.claude/skills/review-pr/references/process/design-contracts.md @@ -0,0 +1,34 @@ +# Resolve Design Contracts + +Use after freezing the review head and before selecting module or feature +references. The maintained [design index](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/) +defines two routing axes: + +- **Module designs** own code boundaries and invariants. Select one primary + module and a second only when the live producer-consumer path crosses it. +- **Feature designs** describe behavior spanning modules. Load every feature + page whose implementation, default, support, or contract changes. + +## Resolution order + +1. Open `docs/design/index.md` in the reviewed head, not the reviewer's checkout + or published latest tree. +2. Match changed behavior against each module page's `primary_code_paths`, + `primary_path_exceptions`, ownership boundary, and `depends_on` metadata. +3. Read `status`, `architecture_state`, `last_verified_commit`, and any linked + in-flight work before treating prose as current behavior. +4. Use current code and tests when a page is draft, deferred, stale, absent, or + conflicts with the frozen implementation. Report material documentation + drift separately. +5. Treat candidate invariants and promotion gates as review questions until + repository policy, current code/tests, or owner-approved normative text + makes them enforceable. Do not turn a draft identifier into an automatic + blocker. +6. Use published latest docs only for discovery when the reviewed head lacks a + page; never import a newer contract into an older branch silently. + +When behavior changes an active module or feature contract, require the same PR +to update that page. Require a new page only when repository policy, an accepted +RFC/issue, or explicit maintainer acceptance criteria do so; otherwise treat it +as a non-blocking design suggestion. Do not demand a second page when an existing +design already owns the exact contract. diff --git a/.claude/skills/review-pr/references/process/general-checks.md b/.claude/skills/review-pr/references/process/general-checks.md new file mode 100644 index 000000000..88dfd405e --- /dev/null +++ b/.claude/skills/review-pr/references/process/general-checks.md @@ -0,0 +1,129 @@ +# General Review Checks + +Apply these checks to every PR after freezing the review surface. A pattern is +not a finding until the diff introduces or exposes a reachable trigger, impact, +and smallest safe fix. Do not report unrelated backlog. + +Official docs: [contributing guide](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/) +[feature compatibility](https://docs.vllm.ai/projects/vllm-omni/en/latest/user_guide/feature_compatibility/), +and [design documents](https://docs.vllm.ai/projects/vllm-omni/en/latest/design/). + +## Contract and scope + +- Match the title/body claims to every changed production path, test, config, + dependency, generated artifact, and user-visible document. +- Trace each changed value through public ingress, validation/defaulting, + producer, transformations, process or device boundaries, final consumer, and + terminal cleanup. +- Check every applicable sync/async, offline/online, streaming/non-streaming, + feature-off, topology, and compatibility path without demanding unsupported + modes merely for symmetry. + +## Module, API, and class design + +Apply these rules to new or materially changed production code; do not require +unrelated cleanup from untouched files. + +1. Start each source module with a concise module docstring that defines its + responsibility and boundaries and identifies its primary classes and key + functions. Place it after any required license header and before imports. +2. Give every added or materially changed class and function a clear docstring + covering its responsibility, parameters, return value, raised exceptions, + and non-obvious invariants. Use explicit typed parameters at owned API + boundaries; avoid opaque `**kwargs` and `Any`. Allow them only when an + upstream or compatibility interface requires them, and document and validate + the accepted keys and concrete types. +3. Move stable helpers shared by multiple features into an appropriately scoped + utility module. Do not let feature modules accumulate large collections of + unrelated private utility functions; keep a private helper local only when + it is single-purpose and tightly coupled to that feature. +4. Keep every method on a key class within that class's stated responsibility, + and document non-obvious design decisions and invariants. Before adding a + method, prefer reuse, consolidation, or extension of an existing abstraction; + split the class when new behavior creates a separate responsibility or makes + the class unnecessarily long. +5. Require changes to user-facing behavior to update `docs/`. When an existing + active module or feature design page owns the changed contract, update that + page. Require a new design page only when the reviewed branch's repository + policy, an accepted RFC/issue, or explicit maintainer acceptance criteria do + so; otherwise suggest it as non-blocking. For a major architectural change, + apply the branch's contributing policy for a design RFC or issue; the current + policy uses more than 500 changed production-code lines, excluding kernel, + data, config, and test changes. Resolve page status and authority through + [design-contracts.md](design-contracts.md), and do not demand a duplicate page + when an existing design owns the exact contract. When required, cover scope, + data flow, public contracts, failure behavior, alternatives, validation, + limitations, and rollout. +6. Require every performance PR to provide reproducible base-versus-head A/B + results under the same hardware, software, model, workload, precision, + topology, warmup, repetitions, and measurement scope. Include exact commands, + variability, and a correctness or quality guard; one-sided or non-comparable + numbers are incomplete evidence. For a new model absent from the frozen base, + compare head with its pinned canonical reference implementation under rule 12 + instead. Follow + [perf-verification.md](../checks/perf-verification.md) for the full protocol. +7. Require every addition, removal, default change, or semantic change to a + user-facing inference API or CLI to explain why it is necessary, which + alternatives were considered, and why the chosen interface is preferable. + Record compatibility, deprecation or migration behavior, and the rationale + in the PR. Update the corresponding module and feature design documents when + they define the changed interface or workflow, or when policy requires one. +8. Require every bug-fix PR to add an automated regression test that reproduces + the defect on the frozen base and passes on the fixed head. Exercise the + production path or nearest stable boundary and assert the corrected behavior, + not only process survival. Record the exact base/head command and result, and + place hardware- or model-specific coverage in the correct CI lane. A manual + reproduction supplements but does not replace the regression test. +9. Require every new or materially changed recipe to follow + [`recipes/TEMPLATE.md`](../../../../../recipes/TEMPLATE.md) and the naming and + layout conventions in + [`recipes/README.md`](../../../../../recipes/README.md). + Complete every applicable summary, use-case, reference, hardware, + environment, exact-command, verification, and notes section with tested, + reproducible details; explain any intentional deviation in the PR. +10. Require every new model to verify its supported modalities, platforms, + execution modes, and features with production-path evidence. Update + `docs/models/supported_models.md` and every applicable feature support or + compatibility table; do not claim support that the PR does not validate. + Follow + [model-addition-checklist.md](../checks/model-addition-checklist.md) for the + model-specific documentation checks. +11. Require every new model PR to add or update its in-repository model-family + recipe under `recipes/` and its `recipes/README.md` index entry. Apply rule + 9, and keep the recipe's model identifiers, tasks, serving modes, hardware, + commands, supported features, and limitations aligned with validated PR + evidence. An external recipe link does not replace the in-repository recipe. +12. Require every new model to compare accuracy or output quality and performance + against a pinned baseline or canonical reference implementation. Match the + checkpoint, inputs, precision, hardware, workload, and measurement method; + provide exact commands, results, and explanations for material differences. + Prefer a detailed timing split for each stage and explain when it is + unavailable. Follow + [model-addition-checklist.md](../checks/model-addition-checklist.md) and + [perf-verification.md](../checks/perf-verification.md) for the evidence + contract. + +## Blocking risk scan + +| Risk | Prove before reporting | +| --- | --- | +| Correctness | A live input reaches a wrong output, exception, silent drop, shape/dtype/device mismatch, or partially initialized state. | +| Compatibility | A supported API, config key, default, serialization schema, model identifier, or caller breaks without validation, migration, or deprecation. | +| Lifecycle | Allocation or startup lacks cleanup on partial failure, timeout, cancellation, shutdown, or repeated requests. | +| Concurrency/distribution | Ordering, identity, rank, world size, completion, timeout, backpressure, or collective assumptions fail on a supported path. | +| Cache/shared state | Identity omits a correctness-affecting input, or isolation, invalidation, disabled behavior, eviction, or cleanup is incomplete. | +| Async behavior | Blocking I/O, sleep, a lock across `await`, or unnecessary serialization harms a reachable async path. | +| Security/data handling | Secrets, user payloads, unsafe deserialization/eval/shell, unvalidated paths, or unbounded metric labels are reachable. | +| Validation evidence | Tests bypass the production dispatcher, mock away the change, use unrealistic types/MRO, or cannot fail when the defect returns. | +| User contract | User-visible model, feature, API, CLI, config, default, or compatibility behavior changes without accurate docs or examples. | + +Search bounded callers and sibling implementations before reporting dead code, +duplication, or a missed consumer. Keep new abstractions only when they have a +distinct live caller, invariant, or compatibility need. + +## Finding bar + +Anchor each finding to a changed `path:line`; name the trigger or call path, +current behavior, impact, and smallest fix direction. Treat pending CI, missing +hardware, or unsupported measurements as validation gaps unless the repository +contract makes that evidence a merge requirement. diff --git a/.claude/skills/review-pr/references/process/review-execution.md b/.claude/skills/review-pr/references/process/review-execution.md new file mode 100644 index 000000000..f06f27933 --- /dev/null +++ b/.claude/skills/review-pr/references/process/review-execution.md @@ -0,0 +1,191 @@ +# Review Execution and Delivery + +Use this reference for every review. It defines snapshot collection, gates, +validation records, delivery permissions, maintainer tone, and re-review. + +Official docs: [contributing guide](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/) +and [CI failure triage](https://docs.vllm.ai/projects/vllm-omni/en/latest/contributing/ci/failures/). + +## Contents + +- [Freeze the review surface](#freeze-the-review-surface) +- [Report status before analysis](#report-status-before-analysis) +- [Apply review gates](#apply-review-gates) +- [Run bounded validation](#run-bounded-validation) +- [Deliver maintainer-style findings](#deliver-maintainer-style-findings) +- [Re-review safely](#re-review-safely) + +## Freeze the review surface + +For a GitHub PR, read the base/head SHA before and after fetching metadata and +the diff: + +```bash +gh api "repos/vllm-project/vllm-omni/pulls/" \ + --jq '{base_sha: .base.sha, head_sha: .head.sha}' +REVIEW_FIELDS="number,url,title,body,isDraft,baseRefName,headRefName,mergeable,mergeStateStatus,statusCheckRollup,files" +gh pr view --repo vllm-project/vllm-omni \ + --json "${REVIEW_FIELDS}" +gh pr diff --repo vllm-project/vllm-omni +gh api "repos/vllm-project/vllm-omni/pulls/" \ + --jq '{base_sha: .base.sha, head_sha: .head.sha}' +``` + +Discard the snapshot if either SHA changed. Do not mix comments or validation +from different heads. + +After the SHAs stabilize, fetch and materialize a trusted `head_sha` in an +isolated detached worktree. Run every source read, `rg` search, import, and test +from that snapshot, not the caller's checkout. A detached worktree provides +snapshot isolation, not a security boundary. + +Treat a fork head as untrusted unless the user and environment policy explicitly +establish trust. Never run its imports, tests, builds, hooks, package setup, or +repo-configurable linters/plugins on the reviewer host. Execute them only in a +disposable sandbox or VM with no credentials or inherited secrets, no host agent +or service sockets, minimal read-only host mounts, disabled or explicitly +allowlisted network, resource/time limits, and destruction after the run. If +that boundary is unavailable, limit the review to the remote diff, SHA-addressed +reads such as `git show :`, and existing CI evidence; report all +executable validation as a gap. + +Before the first source read, record a pristine snapshot fingerprint outside +the reviewed worktree. Include `HEAD`, the NUL-delimited index entries and blob +IDs, and a NUL-safe manifest of every worktree entry except Git metadata, +including tracked, untracked, and ignored paths with file type, mode, symlink +target, and content hash. Before each validation group and delivery, recompute +and byte-compare the fingerprint as well as asserting `HEAD == head_sha`. If a +tool changes or creates any entry, discard affected evidence and recreate the +snapshot before another group; do not rely on `HEAD` alone or clean an unknown +worktree in place. If an exact snapshot cannot be created, use SHA-addressed +reads and report filesystem-dependent validation as a gap. + +For a local branch/worktree, determine the target ref from the user, current PR, +or configured upstream; never infer it from the branch name. Resolve the target +once and include all task-owned worktree state: + +```bash +git status --porcelain=v2 -z +git rev-parse HEAD +git rev-parse +git merge-base HEAD +git diff --stat +git diff --name-status +git diff --binary +git diff --cached --binary +git diff --binary +git ls-files --others --exclude-standard -z +``` + +Freeze the exact bytes of every in-scope untracked file from the NUL-delimited +list, plus a NUL-safe path, file-type, mode, and content-hash manifest. Also +fingerprint `HEAD`, the target and merge base, porcelain status, and both binary +index and worktree patches. Recording only untracked names is insufficient. +Keep these snapshots in the evidence packet and do not mutate the reviewed +checkout during a read-only review. + +For local changes without PR metadata, mark the title/body as synthetic. Base +them on the user request and commit messages without inventing validation claims. + +If no PR, branch, or usable worktree is identifiable, ask for a PR URL/number or +explicit base and head rather than guessing. + +## Report status before analysis + +Within 60 seconds of starting, and before source searches or tests, send this +host update: + +```text +Pinned head: +Base/comparison: +CI: +Mergeability: +Preliminary findings: +``` + +Mark early findings as preliminary and continue. This is not a GitHub comment. + +## Apply review gates + +- Report draft/WIP state. Continue a local review when explicitly requested, + but do not publish review events without separate authorization. +- Record DCO, pre-commit, required CI, and mergeability. Pending or unknown gates + do not block source review. +- Treat a failed gate as its own evidence. Do not restate its formatting/lint + output as a new code-review finding. +- Open CI logs only when the first failing step overlaps the frozen diff or + blocks the verdict. Start with the first error, not the last cascade. +- Treat the PR body as navigation. Commands, benchmark tables, and claimed tests + become evidence only after their provenance and relevance are checked. + +## Run bounded validation + +Keep one evidence packet for files read, bounded searches, callers, tests, CI, +hardware, routes, and findings. Reuse it rather than refetching facts. + +Before pytest, run a short import/version compatibility check. Record each +validation result with: + +```text +repo, head SHA, command, result, Python/platform, dependency or lock fingerprint +``` + +After preflight, run targeted tests and low-cost static checks alongside source +inspection when possible. Imports, tests, builds, and linters may create caches +or rewrite files, so isolate each potentially mutating group in a disposable +snapshot or recreate the pinned snapshot before continuing. Map source symbols +to tests with bounded `rg` searches rather than assuming the test directory +mirrors production paths. + +Classify failures as code, test, infrastructure, or flaky before reporting. +Skipped hardware tests are gaps, not passes. For available hardware, verify the +smallest representative unit/E2E path and compare actual output with PR claims. + +## Deliver maintainer-style findings + +Default to roughly 1-5 short comments, but treat that as calibration rather than +a quota: report every real blocker and report no finding when there is no issue. + +Write each finding as: + +```text +[P1] Short imperative title — path/to/file.py: +. . . +``` + +Use [maintainer-style-study.md](../delivery/maintainer-style-study.md) for +comment tone. +Keep rule IDs, grades, and audit matrices internal unless the user asks for the +complete audit. + +Verify every inline `path:line` against the frozen diff before delivery. For a +PR, re-read the remote head and byte-compare the detached snapshot fingerprint, +or recreate it from `head_sha`, before asserting current lines. For a local +review, recompute and byte-compare the frozen `HEAD`, target, merge base, status, +index patch, worktree patch, and untracked-content manifest. Any mismatch makes +the review stale; discard the affected evidence and restart. Prefer one +root-cause comment to several symptom comments. + +Local presentation is the default. Only explicit authorization permits GitHub +posting. When authorized, publish one consolidated final review after +validation; do not post preliminary or incremental comments. Submit a review +event (`APPROVE`, `COMMENT`, or `REQUEST_CHANGES`) only when the user explicitly +chooses that event. + +Reviewer requests and owner `@mention` comments are separate external writes; +follow [review-requests.md](../delivery/review-requests.md) when the user asks +for them. + +## Re-review safely + +Freeze the new head and compare it with the previously reviewed SHA. Then: + +1. inspect the delta and every file affected by conflict resolution or rebase; +2. revalidate earlier findings against current lines and behavior; +3. read unresolved, non-outdated threads and the author's evidence; +4. rerun only checks invalidated by the delta; +5. look for new regressions introduced by the fix; +6. do not repeat resolved or outdated comments. + +If the target head changes during re-review, discard stale validation and start +again from the new snapshot. diff --git a/.claude/skills/review-pr/references/process/review-routing.md b/.claude/skills/review-pr/references/process/review-routing.md new file mode 100644 index 000000000..10768d329 --- /dev/null +++ b/.claude/skills/review-pr/references/process/review-routing.md @@ -0,0 +1,76 @@ +# Module and Feature Review Routing + +Read [design-contracts.md](design-contracts.md) first. Route from the frozen +head's live producer-consumer behavior, then confirm the result against its +`docs/design/` metadata. Titles and paths are evidence, not the deciding rule. + +## Primary module contract + +Choose one primary module and a second only when the live call path crosses a +documented dependency or primary-path exception. + +| Module | Live behavior signals | Read | +| --- | --- | --- | +| Entrypoints | Offline/CLI/API ingress, validation, rendering, streaming, sessions | [entrypoints.md](../modules/entrypoints.md) | +| Configuration | Schema, defaults, deploy/stage construction, registry, topology | [configuration.md](../modules/configuration.md) | +| I/O and Modality | Requests, messages, serialization, output types, accumulation, completion | [input-output-modality.md](../modules/input-output-modality.md) | +| Error Contracts | Classification, fatality, propagation, sanitization, public rendering | [error-contracts.md](../modules/error-contracts.md) | +| Engine Orchestration | Cross-stage routing, request state, ordering, RPC correlation, terminal convergence | [engine-orchestration.md](../modules/engine-orchestration.md) | +| Stage Runtime | Placement, startup, readiness, replicas, affinity, membership, shutdown | [stage-runtime.md](../modules/stage-runtime.md) | +| OmniConnector | Cross-stage/process/device/node transport and synchronization | [omni-connector.md](../modules/omni-connector.md) | +| Model Integration | Registration, preprocessing, loading, runners, model-specific execution | [model-integration.md](../modules/model-integration.md) | +| AR Runtime | Scheduling, request/cache state, adapters, workers, upstream vLLM semantics | [ar-runtime.md](../modules/ar-runtime.md) | +| Diffusion Family | Diffusion runtime, models, batching, parallelism, or offload | [diffusion.md](../modules/diffusion.md) | +| Execution Platforms | Hardware selection, capabilities, vendor workers, kernels, patches | [execution-platforms.md](../modules/execution-platforms.md) | +| Cache Management | Cache identity, validity, reuse, eviction, reset, teardown | [cache-management.md](../modules/cache-management.md) | +| Quantization | Method/metadata selection, layer mapping, reduced precision, constraints | [quantization.md](../modules/quantization.md) | +| Observability | Metrics, logs, labels, units, correlation, lifecycle | [observability.md](../modules/observability.md) | +| Profiling | Opt-in instrumentation, traces, start/stop lifecycle, overhead | [profiling.md](../modules/profiling.md) | +| Benchmarking | Workloads, metric computation, benchmark CLI, result metadata | [benchmarking.md](../modules/benchmarking.md) | + +For tests-, docs-, CI-, or recipe-only changes, route to the production module +whose contract they protect. When none exists, use the applicable evidence +check without inventing a production owner. + +## Feature-design overlays + +Load every category whose documented behavior, enablement, default, support, or +compatibility changes. + +| Feature-design section | Read | +| --- | --- | +| Runtime and stage execution | [runtime-stage-execution.md](../features/runtime-stage-execution.md) | +| Communication and concrete connectors | [communication.md](../features/communication.md) | +| Diffusion acceleration | [diffusion-acceleration.md](../features/diffusion-acceleration.md) | +| Infrastructure and performance | [infrastructure-performance.md](../features/infrastructure-performance.md) | + +## Evidence and change overlays + +| Signal | Read | Optional repo-local skill | +| --- | --- | --- | +| New or expanded model, loader, processor, registry, or stage config | [model-addition-checklist.md](../checks/model-addition-checklist.md) | [`add-tts-model`](../../../add-tts-model/SKILL.md) or [`add-diffusion-model`](../../../add-diffusion-model/SKILL.md) | +| Latency, throughput, memory, scaling, precision, or quality claim | [perf-verification.md](../checks/perf-verification.md) | [`diffusion-perf-opt`](../../../diffusion-perf-opt/SKILL.md) for diffusion | +| Tests changed, absent for risky behavior, or test-only | [test-quality-evaluation.md](../checks/test-quality-evaluation.md) | [`vllm-omni-test`](../../../vllm-omni-test/SKILL.md) | +| CI, examples, docs, public behavior, or contributor evidence | [tests-docs-checklist.md](../checks/tests-docs-checklist.md) | None | +| Suitable hardware/server and runnable affected path | [verification.md](../checks/verification.md) | None | +| User asks who should review or requests owner notification | [review-requests.md](../delivery/review-requests.md) | None | + +Use [`quantization`](../../../quantization/SKILL.md) for a quantization module +change and [`$vllm-omni-npu-model-runner-upgrade`](../../../vllm-omni-npu-upgrade/SKILL.md) +for an NPU execution-platform change when those skills are available. + +For a bug fix, require a reachable reproduction and regression test. For a +refactor, prove parity and remove obsolete paths. For a feature, verify the +public/config contract, module integration, compatibility/default behavior, +production dispatch, feature design, and docs. + +## Calibrate findings + +- **P0:** security exposure, data corruption, or broad project unusability. +- **P1:** reachable runtime failure, wrong output, compatibility break, or + unsafe lifecycle in the changed behavior. +- **P2:** real non-blocking defect with a concrete future failure mode. + +Treat draft candidate invariants, missing hardware, and unsupported claims as +questions or validation gaps unless current code, tests, or policy makes them a +merge requirement.