Enhance large class styles and code styles (#28978)
This commit is contained in:
fzyzcjy
@@ -0,0 +1,18 @@
|
||||
---
|
||||
paths:
|
||||
- "**/*.py"
|
||||
---
|
||||
|
||||
# General Code Style
|
||||
|
||||
Default conventions for new and modified Python code. Prefer these unless there is a concrete reason not to; call out deviations in review.
|
||||
|
||||
- **Prefer stateless.** Favor pure functions over methods that mutate instance state; pass inputs in, return outputs out.
|
||||
- **Prefer immutable.** Default to immutable data (frozen structs, tuples, read-only values); mutate only when there is a clear need.
|
||||
- **Functions stay small.** Keep each function under ~100 LOC; split larger ones into named helpers.
|
||||
- **Files stay small.** Keep each file under ~2k LOC; split larger modules along cohesive boundaries.
|
||||
- **Core functions read like pseudocode.** The main / orchestration function of a unit should be short and read like algorithm pseudocode — push detail into well-named helpers so the top-level flow is obvious.
|
||||
- **Avoid mixins.** Don't add behavior via mixin classes; prefer explicit composition (hold a collaborator and call it) or plain functions.
|
||||
- **Prefer protected over public.** Default methods to protected (`_name`); expose only what callers actually use.
|
||||
- **Prefer keyword arguments.** Call functions of 2+ args by keyword, and design APIs to be called that way.
|
||||
- **Pass what you need, not the god object.** Give a callee the specific values it uses (by keyword), not a whole large object (`ModelRunner`, `Scheduler`); reserve passing the whole object for a leaf whose contract genuinely requires it. Even then, keep it read-only — read fields off it and return results for the caller to assign, rather than writing fields back through it.
|
||||
@@ -3,6 +3,7 @@
|
||||
Before modifying the following components, read the listed skill first.
|
||||
|
||||
- **Speculative decoding code** (anything under `python/sglang/srt/speculative/`, related attention backends, scheduler accumulators, IPC fields, observability metrics, or CLI flags) → [`speculative-naming`](../skills/speculative-naming/SKILL.md)
|
||||
- **`Scheduler` / `TokenizerManager` / `ModelRunner` `__init__`** (`python/sglang/srt/managers/scheduler.py`, `python/sglang/srt/managers/tokenizer_manager.py`, `python/sglang/srt/model_executor/model_runner.py`) → [`large-class-init-style`](../skills/large-class-init-style/SKILL.md)
|
||||
- **`Scheduler` / `TokenizerManager` / `ModelRunner` `__init__`** (`python/sglang/srt/managers/scheduler.py`, `python/sglang/srt/managers/tokenizer_manager.py`, `python/sglang/srt/model_executor/model_runner.py`) → [`large-class-style`](../skills/large-class-style/SKILL.md)
|
||||
- **Any edit to a frozen core file** (currently `python/sglang/srt/model_executor/model_runner.py`) → [`large-class-style`](../skills/large-class-style/SKILL.md)
|
||||
- **Environment variables** (adding, renaming, or reviewing any `SGLANG_*` env var, migrating a legacy `SGL_*` alias, or touching `python/sglang/srt/environ.py`) → [`env-var-conventions`](../skills/env-var-conventions/SKILL.md)
|
||||
- **Scripted runtime** (anything related to the scripted runtime) → [`scripted-runtime-notes`](../skills/scripted-runtime-notes/SKILL.md)
|
||||
|
||||
@@ -1,32 +0,0 @@
|
||||
---
|
||||
name: large-class-init-style
|
||||
description: '`__init__` style for SGLang `Scheduler`, `TokenizerManager`, and `ModelRunner`. Use when modifying the `__init__` of any of these three classes, or reviewing changes that add new construction logic to them.'
|
||||
---
|
||||
|
||||
# `__init__` Style for Scheduler / TokenizerManager / ModelRunner
|
||||
|
||||
Apply when modifying the `__init__` of:
|
||||
|
||||
- `Scheduler` — `python/sglang/srt/managers/scheduler.py`
|
||||
- `TokenizerManager` — `python/sglang/srt/managers/tokenizer_manager.py`
|
||||
- `ModelRunner` — `python/sglang/srt/model_executor/model_runner.py`
|
||||
|
||||
## Why
|
||||
|
||||
- Downstream forks override one piece (tokenizer, KV cache, IPC, …).
|
||||
- Inline logic forces them to copy the whole `__init__`, which rots against upstream.
|
||||
- Splitting into `init_*` helpers lets them override exactly what they need.
|
||||
- Reference shape: `TokenizerManager.__init__` in `python/sglang/srt/managers/tokenizer_manager.py`.
|
||||
|
||||
## Rules
|
||||
|
||||
- **`__init__` is an orchestrator.** Sequence of `self.init_*(...)` calls + minimal glue. No non-trivial construction inlined.
|
||||
- **One helper per overridable unit.** Each `init_*` = one concern a subclass might swap. Don't lump.
|
||||
- **Naming:** `init_<thing>` (snake_case, names the component). Conditional construction → `maybe_init_<thing>`, gate inside the helper.
|
||||
- **No silent state coupling.** A helper only reads `self.*` set by earlier helpers. Ordering lives in `__init__`. Shared intermediates → pass as args, not via `self.*`.
|
||||
- **New logic = new helper.** Default to adding `init_<thing>`, not another inline block. One-line `self.foo = server_args.foo` is fine; structured logic is not.
|
||||
- **Preserve override points.** Prefer additive changes to existing `init_*` signatures. Breaking changes → call out in PR.
|
||||
|
||||
## Scope
|
||||
|
||||
Only the three classes listed above. Not other manager-style classes, not small dataclass/utility constructors.
|
||||
@@ -0,0 +1,139 @@
|
||||
---
|
||||
name: large-class-style
|
||||
description: 'Code style for SGLang large classes `Scheduler`, `TokenizerManager`, and `ModelRunner`: frozen-code conventions and `__init__` orchestration style. Use when modifying any of these three classes or reviewing changes to them.'
|
||||
---
|
||||
|
||||
# Code Style for Scheduler / TokenizerManager / ModelRunner
|
||||
|
||||
Conventions for SGLang's three large classes:
|
||||
|
||||
- `Scheduler` — `python/sglang/srt/managers/scheduler.py`
|
||||
- `TokenizerManager` — `python/sglang/srt/managers/tokenizer_manager.py`
|
||||
- `ModelRunner` — `python/sglang/srt/model_executor/model_runner.py`
|
||||
|
||||
## 1. Frozen Code
|
||||
|
||||
- Some core files are **frozen**: *orchestration-only* — a thin composition root that constructs collaborators, wires them, delegates to them, and coordinates the calls. They must stay that way.
|
||||
- **Domain logic does not belong in a frozen file**; it lives in a collaborator class in its own module.
|
||||
|
||||
### 1.1 Why
|
||||
|
||||
- The file is a thin orchestrator over collaborator classes; freezing keeps it that way and stops it growing back into a god class.
|
||||
- Keeping domain logic in collaborators (their own files) is what makes per-file code ownership, single responsibility, and unit testing possible.
|
||||
- The orchestrator is the composition root: it may know about every collaborator, because wiring and sequencing them is its job. Coordination stays here — domain logic does not.
|
||||
|
||||
### 1.2 Frozen files
|
||||
|
||||
- `python/sglang/srt/model_executor/model_runner.py`
|
||||
|
||||
### 1.3 Allowed: orchestration
|
||||
|
||||
Every statement refers to a collaborator and is one of:
|
||||
|
||||
1. **Construct** — a short `init_<thing>` helper whose body is essentially a single construction (follows §2); use `maybe_init_<thing>` with a one-line gate when conditional.
|
||||
2. **Wire** — a short call that runs the helper from the orchestrator (e.g. in `__init__`).
|
||||
3. **Delegate** — calls to a collaborator's methods at the necessary call sites (`self.foo.run(...)`).
|
||||
4. **Coordinate** — the minimal control flow that *selects or orders* the above: an `if` choosing whether / which collaborator to wire or call, the order of calls, threading one call's result into the next.
|
||||
|
||||
- Heuristic: a statement is allowed only if it constructs, wires, delegates, or selects/orders those — never if it *computes or transforms* a value beyond passing arguments and results through.
|
||||
|
||||
```python
|
||||
# model_runner.py — orchestration only.
|
||||
def init_foo(self): # construct
|
||||
self.foo = FooManager(server_args=self.server_args, device=self.device)
|
||||
|
||||
self.init_foo() # wire (in __init__)
|
||||
|
||||
if self.server_args.enable_bar: # coordinate: select
|
||||
self.bar.prepare(forward_batch) # delegate
|
||||
out = self.foo.run(forward_batch) # delegate
|
||||
self.baz.consume(out) # coordinate: thread result into next delegate
|
||||
```
|
||||
|
||||
### 1.4 Not allowed: domain logic
|
||||
|
||||
- Config building, data transformation, algorithm bodies, math, post-processing — any branch or loop that *computes* rather than *coordinates*.
|
||||
- It belongs in the collaborator.
|
||||
|
||||
```python
|
||||
# NOT allowed in a frozen file: domain logic inlined.
|
||||
self.foo = None
|
||||
if self.server_args.enable_foo:
|
||||
config = build_foo_config(self.model_config, self.device) # config logic in frozen file
|
||||
self.foo = FooManager(config) # inline construction, not via (maybe_)init_foo
|
||||
out = [step(x) for x in batch] # computation, not coordination
|
||||
```
|
||||
|
||||
- Fix: move that body into `FooManager` (its `__init__` or a factory) plus a `(maybe_)init_foo` helper.
|
||||
|
||||
### 1.5 Where coordination logic goes
|
||||
|
||||
1. **Default: extract.** Pull cohesive coordination into a low-coupling collaborator (an initializer, a forward pipeline) and delegate to it.
|
||||
2. **Residue stays.** Coordination that can't be cohesively extracted may remain — but only the minimal **Coordinate** form above, kept pseudocode-readable. This is the explicit exception, not a fallback; note why it stays.
|
||||
|
||||
- When the residue outgrows pseudocode, that is the signal to extract a dedicated coordinator — not to keep inlining.
|
||||
|
||||
### 1.6 Pass what the collaborator needs, not the god object
|
||||
|
||||
- When you extract domain logic into a collaborator (a factory, an initializer, a pipeline), give it the **specific values** it needs — `model_config`, `device`, the sizes — not the whole frozen object (`ModelRunner`, `Scheduler`).
|
||||
- Passing the god object back re-creates the coupling the split was meant to remove: the module still reads dozens of attributes off it, can't be unit-tested without building the whole class, and every field rename ripples back in.
|
||||
|
||||
- Default to **narrow, keyword args**. Reference shape: `layer_setup.resolve_layer_indices(*, model, model_config, is_draft_worker, spec_algorithm)`.
|
||||
- Return a small **frozen struct** and let the orchestrator assign it onto its own fields. The collaborator should not reach back in and mutate the god object.
|
||||
- If a leaf genuinely needs the live object — its constructor contract already takes the runner, or it reads state that mutates after init — confine that dependency to the **smallest leaf** and pass narrow args everywhere above it. Note why it can't be narrowed.
|
||||
|
||||
### 1.7 If you do pass the god object, keep it read-only
|
||||
|
||||
- A callee that genuinely takes the live object should **read** fields off it and **return** results; it writes fields back only when there is genuinely no other way.
|
||||
- The orchestrator owns the assignment onto its own fields.
|
||||
- Why: a callee that mutates the god object scatters its writes across other modules — you can no longer see what `ModelRunner` owns by reading `model_runner.py`, the hidden writes race with the orchestrator's own ordering, and the callee silently depends on being invoked at exactly the right moment.
|
||||
|
||||
```python
|
||||
# Good — callee reads the runner and returns a small frozen struct; the orchestrator
|
||||
# owns the writes.
|
||||
# model_runner.py
|
||||
class ModelRunner:
|
||||
def bar(self):
|
||||
self.foo_result = foo(self)
|
||||
|
||||
# another_file.py
|
||||
def foo(model_runner) -> FooResult:
|
||||
return FooResult(a=xx, b=yy, c=zz)
|
||||
|
||||
# Avoid — callee reaches back in and writes the runner's fields.
|
||||
# model_runner.py
|
||||
class ModelRunner:
|
||||
def bar(self):
|
||||
foo(self)
|
||||
|
||||
# another_file.py
|
||||
def foo(model_runner):
|
||||
model_runner.a = xx
|
||||
model_runner.b = yy
|
||||
model_runner.c = zz
|
||||
```
|
||||
|
||||
## 2. `__init__` style
|
||||
|
||||
Apply when modifying the `__init__` of the three classes above.
|
||||
|
||||
### 2.1 Why
|
||||
|
||||
- Downstream forks override one piece (tokenizer, KV cache, IPC, …).
|
||||
- Inline logic forces them to copy the whole `__init__`, which rots against upstream.
|
||||
- Splitting into `init_*` helpers lets them override exactly what they need.
|
||||
- Reference shape: `TokenizerManager.__init__` in `python/sglang/srt/managers/tokenizer_manager.py`.
|
||||
|
||||
### 2.2 Rules
|
||||
|
||||
- **`__init__` is an orchestrator.** Sequence of `self.init_*(...)` calls + minimal glue. No non-trivial construction inlined.
|
||||
- **One helper per overridable unit.** Each `init_*` = one concern a subclass might swap. Don't lump.
|
||||
- **Naming:** `init_<thing>` (snake_case, names the component). Conditional construction → `maybe_init_<thing>`, gate inside the helper.
|
||||
- **No silent state coupling.** A helper only reads `self.*` set by earlier helpers. Ordering lives in `__init__`. Shared intermediates → pass as args, not via `self.*`.
|
||||
- **New logic = new helper.** Default to adding `init_<thing>`, not another inline block. One-line `self.foo = server_args.foo` is fine; structured logic is not.
|
||||
- **Preserve override points.** Prefer additive changes to existing `init_*` signatures. Breaking changes → call out in PR.
|
||||
|
||||
### 2.3 Scope
|
||||
|
||||
- Only the three classes listed above.
|
||||
- Not other manager-style classes, not small dataclass/utility constructors.
|
||||
Reference in New Issue
Block a user