From d6f1d3d61ca462ff8e89312ebf0fdb675edad104 Mon Sep 17 00:00:00 2001 From: Hongsheng Liu Date: Fri, 26 Jun 2026 20:31:10 +0800 Subject: [PATCH] [Doc] Add code-quality dimension to precheck-pr skill (#4697) Signed-off-by: hsliu_ustc Signed-off-by: Hongsheng Liu Co-authored-by: hsliu_ustc Co-authored-by: Claude Opus 4.8 Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .claude/skills/precheck-pr/SKILL.md | 3 + .../precheck-pr/references/checklists.md | 27 ++- .../precheck-pr/references/code-quality.md | 179 ++++++++++++++++++ 3 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 .claude/skills/precheck-pr/references/code-quality.md diff --git a/.claude/skills/precheck-pr/SKILL.md b/.claude/skills/precheck-pr/SKILL.md index ab6034a48..cd86ec4aa 100644 --- a/.claude/skills/precheck-pr/SKILL.md +++ b/.claude/skills/precheck-pr/SKILL.md @@ -63,6 +63,8 @@ If multiple rows apply (e.g., a diffusion model is also a new model), union the Ask: "Quick mode or full mode?" Then walk the checklist for the detected PR type from [references/checklists.md](references/checklists.md). Each item produces ✓, ✗, or ⚠. +**Also run the Code-Quality sweep on every PR**, regardless of type or mode: the five diff-scoped checks in [references/code-quality.md](references/code-quality.md) — kwargs fragility, broad-except swallow, `Any`/wrong type hints, hot-path `.clone()`/`deepcopy`, and event-loop blocking — plus the advisory conventions (log level, structured logging, synchronization, cleanup, dependencies, naming) in [checklists.md](references/checklists.md). These count only lines the PR *adds* — the pre-existing backlog across the repo is out of scope. + ### Step 5: Print Report ``` @@ -74,6 +76,7 @@ Pre-check report for Dimension Result ───────────────── ────── PR title format ✓ + Code quality ⚠ 1 broad except, 2 Any hints PR desc integrity ✓ Registry/config ✓ Dead code ⚠ 2 warnings diff --git a/.claude/skills/precheck-pr/references/checklists.md b/.claude/skills/precheck-pr/references/checklists.md index 5b9472d96..a232403a7 100644 --- a/.claude/skills/precheck-pr/references/checklists.md +++ b/.claude/skills/precheck-pr/references/checklists.md @@ -35,6 +35,31 @@ PR title must follow the project convention documented in `docs/contributing/REA --- +## All PRs: Code Quality (run on every PR, quick & full) + +Diff-scoped sweep for five fragility patterns that are pervasive in the existing codebase — **only lines the PR adds count**, not the pre-existing backlog. Detection commands, codebase examples, and full severity rules live in [code-quality.md](code-quality.md). + +- [ ] **No new `**kwargs` string-lookup plumbing:** the diff adds no `**kwargs` + `kwargs["..."]` / `kwargs.get("...")` / `"..." in kwargs`. ⚠ any new instance; ✗ if a new string key is duplicated across ≥2 files without a shared constant, or unknown keys are silently dropped on a fail-fast path. +- [ ] **No new broad exception swallow:** the diff adds no `except Exception:` / bare `except:`. ⚠ any new instance; ✗ for `except: pass` / `except Exception: return None` / `continue` on a fail-fast path (init, config validation, weight loading, request handling). +- [ ] **No new `Any` / wrong type hints:** the diff adds no `: Any` / `-> Any` / untyped production signature, and no new `SimpleNamespace` in tests faking an object that already has a real typed stub. ⚠ any new instance; ✗ for a *wrong* (actively misleading) annotation or a `SimpleNamespace` test fake of a typed object. +- [ ] **No new hot-path copy:** the diff adds no `.clone()` / `.copy_()` / `copy.deepcopy(...)` inside a per-step loop or per-request path. ⚠ any new instance; ✗ for `.clone()` of a full latent/activations tensor inside an AR or diffusion step loop without a comment, or `deepcopy` of a scheduler / sampling-config on the request path. +- [ ] **No new event-loop blocking:** the diff adds no `time.sleep` / blocking HTTP (`requests`, `urllib`) / `await` inside a held lock in `async def` code. ⚠ any new instance; ✗ for blocking sleep or HTTP on the serving path (`engine/`, `entrypoints/`, async `connectors/`), or a lock held across an `await`. Blocking inside a dedicated worker thread is fine. + +Roll the ⚠/✗ counts into the report as a single **Code quality** dimension row. + +### Conventions (eyeball — no grep) + +Judgment calls that don't grep cleanly — apply them while reading the diff, not as a mechanical check: + +- **Right log level** — `info` is for end-user-visible events; routine internal events are `debug`; hot paths are `trace` or removed. Logging is not free. +- **Structured fields over f-strings** — pass dynamic values as structured fields (`logger.info("msg", extra={...})`), not interpolated strings, so they stay greppable and parseable. +- **No reflexive synchronization** — don't add `threading.Lock` / `asyncio.Lock` / `deepcopy` / re-wrap shared state unless there is real concurrent mutation; the GIL covers most Python code. Owners decide their own synchronization — don't pre-lock in a constructor. +- **Context managers for cleanup** — use `with` / `contextlib` for files, locks, CUDA streams, and profilers; not raw `open()` + manual `.close()` or ad-hoc `finally`. +- **No new dependencies without justification** — prefer the stdlib and existing vllm-omni deps; a new top-level dep needs a reason (check `pyproject.toml` / `requirements*.txt`). +- **Naming / interface discipline** — don't rename or change interfaces just for taste in this PR, but do fix names that actively mislead (e.g. `serve` implying a long-running server, `Instance` where multiple instance kinds coexist). + +--- + ## Bug Fix PRs ### Quick @@ -49,7 +74,7 @@ PR title must follow the project convention documented in `docs/contributing/REA - [ ] **Regression test exists:** `git diff --name-only` includes at least one `tests/` file - [ ] **Regression test reproduces the original error:** the test would fail on main, pass on this branch - [ ] **Fix matches root cause exactly:** no "fix the symptom + something else" — if the root cause is dtype, the fix is dtype, not dtype + reformatting -- [ ] **No silent failure risk:** no bare `except: pass`, no `try: ... except: return None`, no empty fallback added +- [ ] **No silent failure risk:** no bare `except: pass`, no `try: ... except: return None`, no empty fallback added. See the broad-except pattern in [code-quality.md](code-quality.md#2-broad-exception-swallow) for the full severity rule (⚠ any new broad catch; ✗ for a swallow on a fail-fast path). - [ ] **Upstream pattern match:** the fix follows the same pattern as existing code for similar cases (grep for analogous `from_pretrained` calls, etc.) - [ ] **Environment documented:** torch/transformers/vllm versions listed if the bug is version-dependent diff --git a/.claude/skills/precheck-pr/references/code-quality.md b/.claude/skills/precheck-pr/references/code-quality.md new file mode 100644 index 000000000..cb4b4ee44 --- /dev/null +++ b/.claude/skills/precheck-pr/references/code-quality.md @@ -0,0 +1,179 @@ +# Code-Quality Patterns + +Five fragility patterns to sweep on **every** PR, in both quick and full mode. These are pervasive in the existing codebase (hundreds of pre-existing sites) — so the checks are **diff-scoped**: only lines the PR *adds* count. Whole-repo greps light up every PR with the backlog and are useless. + +All commands assume: +```bash +BASE="$(git merge-base HEAD origin/main)" +``` + +**Severity:** ⚠ for any *new* instance introduced by the diff; ✗ for the acute sub-cases defined per pattern. Acute ✗ findings belong in the report's blocking count. + +--- + +## 1. `**kwargs` string-lookup passthrough + +### Anti-pattern +```python +def forward(self, **kwargs): + # BLOCKER-prone: raw dict + string keys duplicated across files + if "runtime_additional_information" in kwargs and \ + "model_intermediate_buffer" not in kwargs: + ... +``` +Real instances of this exact guard are copy-pasted across `cosyvoice3.py`, `indextts2_talker.py` (×2), `indextts2_s2mel_decoder.py`, `qwen3_tts_talker.py`, `qwen3_omni.py`. A typo in any string key silently no-ops; there is no type check, no IDE completion, and unknown keys are silently dropped. + +### Why dangerous +Raw `dict` + `**kwargs` + lists of param-name strings is **fragile by construction**: the compiler can't see a missing or misspelled key, so failures are silent. Each duplicated copy is a new place for the strings to drift. + +### Severity +- ⚠ any *new* `**kwargs` signature whose body does `kwargs["..."]` / `kwargs.get("...")` / `"..." in kwargs`. +- ✗ when the diff introduces a **new** string key that is duplicated across ≥2 files without a shared module constant, or when `**kwargs` plumbing **silently drops unknown keys** on a fail-fast path (init, config validation, weight loading). + +### Detect (diff-scoped) +```bash +git diff ${BASE}...HEAD -- "*.py" | grep "^+[^+]" \ + | grep -E '\*\*kwargs|kwargs(\.get|\[)|"[a-z_]+"\s+in\s+kwargs' +``` + +### Fix +Prefer explicit typed params. The repo already has the right primitive: `msgspec.Struct` with `forbid_unknown_fields=True` rejects unknown keys instead of silently dropping them — see `_StructBase` in `vllm_omni/data_entry_keys.py:105` and `MoriPullRequest` in `vllm_omni/distributed/omni_connectors/connectors/mori_transfer_engine_connector.py:81`. A `@dataclass` or `TypedDict` works too. + +If `**kwargs` is unavoidable (vLLM base-class compat), centralize the key strings as module-level constants behind **one** typed accessor, instead of repeating the literal across files. + +--- + +## 2. Broad exception swallow + +### Anti-pattern +```python +# BLOCKER-prone: catches everything, returns None on any failure +try: + evt = build_event(...) + return evt +except Exception: + return None +``` +Real clusters: `vllm_omni/metrics/stats.py:240,274,367,450` and `vllm_omni/metrics/{modality,utils}.py` — all `except Exception: return None`. + +### Why dangerous +A generic `except Exception:` is **too flexible**. If we catch an exception type we don't expect, it is almost always an unhandled edge case that needs to be *fixed* — not swallowed into `None`/`pass`/`continue`. Broad catches turn fixable bugs into silent wrong behavior and make debugging impossible. + +The repo backs this with ruff `BLE001` (broad-exception catch); it is currently suppressed via `# noqa: BLE001` in `vllm_omni/patch.py:413`. Prefer fixing over suppressing. + +### Severity +- ⚠ any *new* `except Exception:` / `except BaseException:` / bare `except:`. +- ✗ for `except:` / `except Exception: pass|return None|continue` on a **fail-fast path** (init, config validation, weight loading, request handling, connector setup). This extends the Bug-Fix "No silent failure risk" ✗ in [checklists.md](checklists.md). + +### Detect (diff-scoped) +```bash +# Any new broad catch (⚠ baseline): +git diff ${BASE}...HEAD -- "*.py" | grep "^+[^+]" \ + | grep -E 'except\s*(Exception|BaseException)?\s*:' +# Note: this matches only the immediate-colon swallow forms (`except :`, +# `except Exception:`) — it deliberately excludes specific catches like +# `except ValueError:` and the `except Exception as e:` variant. Eyeball +# the latter; `as e` usually means it is logged, but still verify. +``` + +### Fix +Catch the **specific** types you actually expect (`ValueError`, `KeyError`, `AttributeError`, `OSError`, `torch.cuda.OutOfMemoryError`, `msgspec.ValidationError`, …). At a genuine top-level / best-effort boundary (metrics, signal handlers), at minimum **log** the exception; never swallow into a bare `pass`/`return None` on a path that should fail loudly. + +--- + +## 3. `Any` / wrong type annotations + +### Anti-pattern +```python +# ⚠ Any param + Any return — no contract, no narrowing +def _extract_mm_output(engine_outputs: Any) -> dict[str, Any]: + ... +``` + +### Why dangerous (root cause) +`Any` disables the type checker for that position, so typos and wrong shapes pass silently. The repo has 636 `: Any` params, 244 `-> Any` returns, and **1,173 `SimpleNamespace` in `tests/`**. The `Any` leak is largely test-driven: vibe-coded unit tests fake objects with `SimpleNamespace`, which forces the production code consuming them to be typed `Any`, and the `Any` then propagates outward into public signatures. + +### Severity +- ⚠ any *new* `: Any` / `-> Any` / fully-untyped signature in production code. +- ✗ for a **wrong** annotation (actively misleading — e.g. `-> bool` that returns `Optional[bool]`), or a *new* `SimpleNamespace` in a test that mimics an object which already has a real typed stub / `@dataclass` / `TypedDict` / `Protocol`. + +### Detect (diff-scoped) +```bash +# Any leaking into new signatures: +git diff ${BASE}...HEAD -- "*.py" | grep "^+[^+]" \ + | grep -E ':\s*Any\b|->\s*Any\b' +# SimpleNamespace newly added in tests (the Any-leak source): +git diff ${BASE}...HEAD -- "tests/" | grep "^+[^+]" | grep "SimpleNamespace" +``` + +### Fix +Replace `Any` with the concrete type, a `Protocol`, or a `Union`. If a type is genuinely dynamic, prefer `object` + `isinstance` narrowing over `Any` — `object` keeps the checker engaged. In tests, replace `SimpleNamespace` with the real class, a small `@dataclass`/`TypedDict`, or a `Protocol` + fake, so the system under test stays fully typed. + +--- + +## 4. Unnecessary copy — `.clone()` / `copy.deepcopy` in hot paths + +### Anti-pattern +```python +# Per-step in a diffusion/AR loop: clones the full latent every iteration +for t in timesteps: + trajectory_latents.append(x_t.clone()) # O(N) GPU alloc + memcpy each step +``` +Real clusters: `vllm_omni/diffusion/models/bagel/bagel_transformer.py` calls `x_t.clone()` ~9× across a trajectory loop (lines 1820–2186); the cache hooks `vllm_omni/diffusion/cache/teacache/hook.py:155` and `magcache/hook.py:182` clone `hidden_states` per step. Separately, `copy.deepcopy(self.scheduler)` / `copy.deepcopy(req.sampling_params)` is copy-pasted across diffusion pipelines — `pipeline_qwen_image.py:786`, `pipeline_hunyuan_image3.py:1904`, `pipeline_ltx2*.py` (×3), `pipeline_dreamzero.py:1085`, `diffusion_model_runner.py:398` — a full object-graph deepcopy on the request path. (Contrast `dreamzero/causal_wan_model.py:203`, which carries a `# No .clone() needed.` comment — the aware pattern.) + +### Why dangerous +A `.clone()` of a GPU tensor or a `copy.deepcopy` of a config object inside a per-step / per-request path is a hidden allocator: it reads as one Python line but triggers a GPU memcpy or a deep object-graph walk every iteration. Across 50 diffusion steps or hundreds of AR steps it dominates latency and VRAM bandwidth. `deepcopy` additionally ignores any custom device / `__copy__` logic and can drag tensors through CPU. + +### Severity +- ⚠ any *new* `.clone()` / `.copy_()` / `copy.deepcopy(...)` inside a per-step loop or per-request path. +- ✗ for `.clone()` of a full latent/activations tensor inside an AR or diffusion step loop without an explanatory comment, or `copy.deepcopy` of a scheduler / sampling-config on the request hot path. + +### Detect (diff-scoped) +```bash +git diff ${BASE}...HEAD -- "*.py" | grep "^+[^+]" \ + | grep -E '\.clone\(\)|\.copy_\(|copy\.deepcopy|deepcopy\(' +``` + +### Fix +Tensors: prefer a view (`x[..., :k]`, `torch.narrow`, `as_strided`) or write into a pre-allocated buffer with `dst.copy_(src)` instead of `.clone()`; if the caller no longer needs the original, `move`/`del` it rather than copy. Schedulers / configs: build a fresh lightweight instance per request, or split out the mutable per-request bits so the shared immutable part isn't deepcopied. If a copy is genuinely required (e.g. the source is mutated in place later), keep it and add a one-line comment saying why. + +--- + +## 5. Blocking the asyncio event loop / lock held across `await` + +### Anti-pattern +```python +# BLOCKER-prone: blocking sleep / blocking HTTP / lock held across an await, +# all inside an async (event-loop) function +async def handle(self, req): + with self._lock: + resp = await self._client.call(req) # lock held across await -> serializes all requests + time.sleep(0.1) # stalls the single loop for every concurrent request +``` + +### Why dangerous +vLLM-Omni serves over a single asyncio event loop. Any synchronous blocking work on an `async def` path — `time.sleep`, blocking HTTP (`requests`/`urllib`), heavy CPU work, or a lock held across an `await` — stalls the loop for *every* concurrent request, not just the blocking one. A lock held across `await` silently serializes the whole pipeline; blocking I/O tanks throughput and inflates tail latency. + +### Severity +- ⚠ any *new* `time.sleep(`, blocking HTTP call (`requests.get/post`, `urllib.request`, `urlopen`), or `await` inside a `with Lock` / `async with` critical section, in `async def` code. +- ✗ for `time.sleep` or blocking HTTP on the serving path (`engine/`, `entrypoints/`, async `connectors/`), or a lock acquired and held across an `await` on the request path. + +### Detect (diff-scoped) +```bash +# New blocking primitives introduced by the diff — then eyeball whether each +# sits in an async function on the loop vs a dedicated worker thread: +git diff ${BASE}...HEAD -- "*.py" | grep "^+[^+]" \ + | grep -E 'time\.sleep|requests\.(get|post|put|delete)|urllib\.request|urlopen' +# Lock acquired with an await potentially inside the critical section (manual eyeball): +git diff ${BASE}...HEAD -- "*.py" | grep "^+[^+]" | grep -E 'async with|\.acquire\(\)' +``` +Note: blocking inside a **dedicated worker thread is correct** — the repo already does this (`diffusion/model_loader/hub_prefetch.py`, `distributed/omni_connectors/kv_transfer_manager.py`, the `mooncake`/`mori` connectors poll with `time.sleep` on their own threads). The check targets the *event-loop* path only. + +### Fix +Move blocking work off the loop with `asyncio.to_thread(...)` / `run_in_executor`, or use a native async client (`httpx.AsyncClient` / `aiohttp`). Never `await` while holding a lock — drop the lock before the `await` and re-acquire after, or restructure so the critical section contains no `await`. + +--- + +## Running the sweep + +Run all five detections against `${BASE}...HEAD`, then for each hit decide ⚠ vs ✗ using the severity rules above. Roll the results into the report as a single **Code quality** dimension row (count of ⚠ and ✗), alongside the type-specific checklist from [checklists.md](checklists.md).