[Doc] Add code-quality dimension to precheck-pr skill (#4697)
Signed-off-by: hsliu_ustc <hsliu_ustc@noreply.gitcode.com> Signed-off-by: Hongsheng Liu <liuhongsheng4@huawei.com> Co-authored-by: hsliu_ustc <hsliu_ustc@noreply.gitcode.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
3 files changed
+208
-1
No files matched your search
@@ -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 <branch>
|
||||
Dimension Result
|
||||
───────────────── ──────
|
||||
PR title format ✓
|
||||
Code quality ⚠ 1 broad except, 2 Any hints
|
||||
PR desc integrity ✓
|
||||
Registry/config ✓
|
||||
Dead code ⚠ 2 warnings
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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).
|
||||
Reference in new issue
Block a user