From 5a100d9086adca6fd6a273884d8f7c8c444cd287 Mon Sep 17 00:00:00 2001 From: Liangsheng Yin Date: Thu, 20 Aug 2026 14:18:40 -0700 Subject: [PATCH] [misc] Trim restating comments and docstrings in srt/managers (#35622) --- .claude/rules/comment-style.md | 142 +++++++++++------- python/sglang/srt/managers/schedule_batch.py | 15 +- .../sglang/srt/managers/tokenizer_manager.py | 62 +------- .../managers/tokenizer_manager_score_mixin.py | 123 +-------------- 4 files changed, 98 insertions(+), 244 deletions(-) diff --git a/.claude/rules/comment-style.md b/.claude/rules/comment-style.md index a46170ebe..09d56239d 100644 --- a/.claude/rules/comment-style.md +++ b/.claude/rules/comment-style.md @@ -15,15 +15,47 @@ Applies to `#` and `//` comments, Python docstrings, and C/C++ Doxygen blocks like code, with the burden of proof reversed: the author justifies a comment's existence, not the reviewer its removal. -**The test.** Delete the comment. If someone familiar with the repo can recover -the fact from the surrounding code plus a grep, it stays deleted. +## Two modes -The next three sections are that test applied. A comment either carries a fact -you cannot see from here (**keep**), carries nothing the code does not already -carry (**delete**), or carries a real fact whose home is somewhere else -(**move**). +Writing a comment and editing someone else's are different decisions. -## Keep: facts you cannot see from here +**Writing.** You still have the context that made the comment necessary, so the +judgment is reliable. Everything below `## Cleanup` is written for this case. + +**Editing.** The judgment is unreliable and the error is one-way. A one-line +comment carries too little text to tell a restatement apart from the last anchor +for a cross-file fact -- deciding takes the whole function and its callers, more +context than the line costs. And the payoff is asymmetric: keeping a useless +one-liner costs a line of scroll, while deleting a load-bearing one deletes a +fact silently, inside a diff of two hundred deletions where no reviewer will +catch it. So: a bright line, not a judgment call. + +> **A comment-only diff does not touch one-line comments.** It removes or +> condenses multi-line prose blocks. A one-liner is rewritten only for a reason +> of its own -- it is wrong, it is stale, or it is commented-out code -- never +> because the line below it says the same thing. + +## Cleanup: what a comment-only diff may remove + +- **Multi-line prose restating the code** -- the function name, the next line, + the branch condition, or the loop body, written out as a paragraph. +- **Python `Args:` / `Returns:` blocks** on anything that is not a documented API + surface -- see `## Documentation blocks` for the list of surfaces that keep + them. +- **Multi-line history and rationale** -- what the code used to do, why the + change was made, which approach was abandoned. Each of these has a home + elsewhere (see `## Where other explanations live`). +- **Commented-out code**, at any length. + +Condense rather than delete when a block carries one fact that is not +recoverable: keep that sentence, drop the enumeration around it. + +## What a comment is for + +State the fact a reader cannot see from here. Delete the comment and ask what it +costs to recover: a fact that lives in another file costs everything, a grouping +the names only half-encode costs a tedious reconstruction, a restated line costs +nothing. - **Cross-boundary constraints.** Layout, field order, or call order shared with a CUDA kernel, the Rust router, an IPC schema, or another process. Nothing in @@ -70,29 +102,51 @@ carry (**delete**), or carries a real fact whose home is somewhere else # None means the request is excluded from the radix cache, not that lookup failed. ``` -## Delete: nothing the code does not already carry +- **Structure the names only partly encode** -- a group boundary in a long flat + block, a section split in a long body. `# ===== Helpers =====` above two + functions costs nothing to see past; the same banner splitting a genuinely long + flat module (see `python/sglang/srt/environ.py`) is the only statement of where + one group ends. -- **Restating the code.** The function name, the next line, the branch - condition, or the loop body in prose. -- **Step-by-step play-by-play.** `# Step 1: ...` / `# Step 2: ...` over - straight-line code. If the flow needs numbering, extract named helpers. -- **Naming the callers.** That is what grep is for. -- **Section banners on short bodies.** `# ===== Helpers =====` above two - functions. Banners are for genuinely long modules only - (see `python/sglang/srt/environ.py`). -- **Hedging.** "This should probably be revisited." Either establish the fact - and state it, or file it as `TODO()`. +Not worth writing, at any length: prose that restates the line below it, +`# Step 1:` / `# Step 2:` numbering over straight-line code (extract named +helpers if the flow needs numbers), the names of the callers (that is what grep +is for), and hedging -- "this should probably be revisited" is either a fact to +establish and state, or a `TODO()`. -## Move: real facts that live somewhere else +## Form and tags + +- **One or two lines.** A genuinely intricate invariant may run longer; that is a + rare exception, not a licence. Documentation blocks are the separate case + below. +- **ASCII and English only.** No Unicode arrows, math symbols, or CJK. +- **Break at a clause boundary.** If a comment needs a second line, wrap after a + semicolon or a comma -- never mid-phrase. A sentence that will not split + cleanly is a sentence that should be shortened instead. +- **Attach the comment to the line it constrains**, not to the top of the + function. Comments collected into a preamble are the ones that go stale. +- **You change the line, you own its comment.** Update it or delete it -- never + leave it orphaned. A stale comment is worse than no comment. + +Two tag spellings only. New code does not use `FIXME`, `XXX`, or `HACK`; existing +occurrences are grandfathered and get folded into these when the line is touched. + +- `# NOTE:` -- a constraint or trap. Most of the time the prefix adds nothing; + drop it and just state the fact. +- `# TODO():` -- planned work, **always** with an owner or an issue + link. A bare `# TODO: fix this` is rejected in review; an unowned TODO is + never retired. `TODO(perf)` and similar topic tags are acceptable when the work + is a standing category rather than one person's task. + +## Where other explanations live Every explanation has exactly one home. A second copy in a comment drifts from the first. - **What the code used to do -> `git log`.** Comments describe the current - state: not what was tried first, not which approach was abandoned. - - The exception is a past failure that is still a live constraint. That is not - history -- but write it as the constraint, not as the story. + state: not what was tried first, not which approach was abandoned. The + exception is a past failure that is still a live constraint -- write it as the + constraint, not as the story. ```python # Bad: We used to call this before init_new but it broke CUDA graph capture. @@ -104,44 +158,16 @@ the first. diff, and is written for a reviewer who is long gone. - **Design rationale -> `docs/` or a module docstring. CLI semantics -> the `ServerArgs` help text. Test intent -> the test.** -- **Superseded code -> `git show`.** Never leave commented-out code behind. +- **Superseded code -> `git show`.** Also out: review attribution ("as suggested in review") and our own PR numbers used as a changelog. An upstream issue URL is different -- it is not a changelog -entry but a workaround's retirement condition, and the Keep section requires it. +entry but a workaround's retirement condition, and the section above requires it. What stays next to the line is why *this line* exists, written for a reader who has neither the PR nor the discussion. -## Form - -Governs `#` / `//` comments; the length and placement of documentation blocks -follow the section below. ASCII-and-English applies to both. - -- **One or two lines.** A genuinely intricate invariant may run longer; that is - a rare exception, not a licence. -- **ASCII and English only.** No Unicode arrows, math symbols, or CJK. -- **Break at a clause boundary.** If a comment needs a second line, wrap after a - semicolon or a comma -- never mid-phrase. A sentence that will not split - cleanly is a sentence that should be shortened instead. -- **Attach the comment to the line it constrains**, not to the top of the - function. Comments collected into a preamble are the ones that go stale. -- **You change the line, you own its comment.** Update it or delete it -- never - leave it orphaned. A stale comment is worse than no comment. - -## Tags - -Two spellings only. New code does not use `FIXME`, `XXX`, or `HACK`; existing -occurrences are grandfathered and get folded into these when the line is touched. - -- `# NOTE:` -- a constraint or trap. Most of the time the prefix adds nothing; - drop it and just state the fact. -- `# TODO():` -- planned work, **always** with an owner or an issue - link. A bare `# TODO: fix this` is rejected in review; an unowned TODO is - never retired. `TODO(perf)` and similar topic tags are acceptable when the work - is a standing category rather than one person's task. - -## API documentation blocks +## Documentation blocks Python docstrings and Doxygen blocks are part of the product on the surfaces users, integrators, and tooling read, and noise everywhere else. Where warranted @@ -162,3 +188,11 @@ they may run past two lines; every other rule in this file still applies. - **No:** internal helpers, overrides, private methods. Never hand-write or generate Python `Args:` / `Returns:` blocks -- the signature and its type hints already carry the names and types. + + The one exception is the orchestration step. In the frozen classes + (`Scheduler`, `TokenizerManager`, `ModelRunner` -- see + `.claude/skills/large-class-style`), each `init_*` / `handle_*` / event-loop + method is an override point a subclass picks from a list; its one-line + docstring is the catalog entry that says what the step does, so it stays even + when it reads close to the method name. A genuine private helper + (`_validate_*`, `_normalize_*`) that no subclass overrides still gets nothing. diff --git a/python/sglang/srt/managers/schedule_batch.py b/python/sglang/srt/managers/schedule_batch.py index 23de35caa..97d913c0c 100755 --- a/python/sglang/srt/managers/schedule_batch.py +++ b/python/sglang/srt/managers/schedule_batch.py @@ -372,9 +372,6 @@ class MultimodalDataItem: return len([item for item in flatten_nested_list(l) if item is not None]) == 0 def set_pad_value(self): - """ - Set the pad value after first hashing the data - """ if self.pad_value is not None: return @@ -722,9 +719,6 @@ class MultimodalInputs: return image_tokens, audio_tokens, video_tokens def merge(self, other: MultimodalInputs): - """ - merge image inputs when requests are being merged - """ # args needed to be merged optional_args = [ @@ -1233,11 +1227,7 @@ class Req(ReqDllmMixin): return self.kv_committed_len def update_spec_correct_drafts_histogram(self, num_correct_drafts: int): - """Update the speculative decoding acceptance histogram. - - Args: - num_correct_drafts: Number of correct draft tokens (no bonus) in this step. - """ + """Record one step accepted draft count (excludes bonus token) into the histogram.""" if len(self.spec_correct_drafts_histogram) <= num_correct_drafts: self.spec_correct_drafts_histogram.extend( [0] * (num_correct_drafts - len(self.spec_correct_drafts_histogram) + 1) @@ -1459,9 +1449,6 @@ class Req(ReqDllmMixin): return self.tokenizer.decode(self.output_ids[-tail_len:]) def check_match_stop_str_prefix(self) -> bool: - """ - Check if the suffix of tail_str overlaps with any stop_str prefix - """ if not self.sampling_params.stop_strs: return False diff --git a/python/sglang/srt/managers/tokenizer_manager.py b/python/sglang/srt/managers/tokenizer_manager.py index f10f1b764..1adf77b56 100644 --- a/python/sglang/srt/managers/tokenizer_manager.py +++ b/python/sglang/srt/managers/tokenizer_manager.py @@ -833,13 +833,6 @@ class TokenizerManager(TokenizerControlMixin, TokenizerManagerScoreMixin): def _detect_input_format( self, texts: Union[str, List[str]], is_cross_encoder: bool ) -> InputFormat: - """Detect the format of input texts for proper tokenization handling. - - Returns: - - InputFormat.SINGLE_STRING: Regular single text like "Hello world" - - InputFormat.BATCH_STRINGS: Regular batch like ["Hello", "World"] - - InputFormat.CROSS_ENCODER_PAIRS: Cross-encoder pairs like [["query", "document"]] - """ if isinstance(texts, str): return InputFormat.SINGLE_STRING @@ -894,38 +887,6 @@ class TokenizerManager(TokenizerControlMixin, TokenizerManagerScoreMixin): Tuple[List[int], Optional[List[int]]], Tuple[List[List[int]], Optional[List[List[int]]]], ]: - """ - Tokenize text(s) using the appropriate tokenizer strategy. - - This method handles multiple input formats and chooses between async dynamic - batch tokenizer (for single texts only) and regular tokenizer. - - Args: - texts: Text input in various formats: - - Regular cases: - - Single string: "How are you?" - - Batch of strings: ["Hello", "World", "How are you?"] - - Cross-encoder cases (sentence pairs for similarity/ranking): - - Single pair: [["query text", "document text"]] - - Multiple pairs: [["q1", "d1"], ["q2", "d2"], ["q3", "d3"]] - - is_cross_encoder: Whether to return token_type_ids for cross-encoder models. - Enables proper handling of sentence pairs with segment IDs. - - Returns: - Single input cases: - Tuple[List[int], Optional[List[int]]]: (input_ids, token_type_ids) - Example: ([101, 2129, 102], [0, 0, 0]) for single text - Example: ([101, 2129, 102, 4068, 102], [0, 0, 0, 1, 1]) for cross-encoder pair - - Batch input cases: - Tuple[List[List[int]], Optional[List[List[int]]]]: (batch_input_ids, batch_token_type_ids) - Example: ([[101, 2129, 102], [101, 4068, 102]], None) for regular batch - - Note: token_type_ids is None unless is_cross_encoder=True. - """ if not texts or self.tokenizer is None: raise ValueError("texts cannot be empty and tokenizer must be initialized") @@ -1490,11 +1451,6 @@ class TokenizerManager(TokenizerControlMixin, TokenizerManagerScoreMixin): token_id: int, embeds: List[torch.Tensor], ) -> PositionalEmbeds: - """Resolve placeholder positions in input_ids and create PositionalEmbeds. - - Scans input_ids for occurrences of token_id and pairs them with the - provided embedding tensors. - """ positions = [idx for idx, tok in enumerate(input_ids) if tok == token_id] if len(positions) != len(embeds): raise ValueError( @@ -1692,11 +1648,8 @@ class TokenizerManager(TokenizerControlMixin, TokenizerManagerScoreMixin): state: ReqState, is_stream: bool, ) -> Optional[dict]: - """Handle abort/error finish reasons from the scheduler. - - Returns the output dict if it should be yielded (stream abort), or None - for normal flow. Raises ValueError or HTTPException for non-stream aborts. - """ + """Returns the output dict to yield (stream abort), None for normal flow; + raises ValueError/HTTPException for non-stream aborts.""" finish_reason = out["meta_info"]["finish_reason"] if ( @@ -3433,17 +3386,6 @@ class TokenizerManager(TokenizerControlMixin, TokenizerManagerScoreMixin): def _should_dispatch_to_encoder( self, obj: Union[GenerateReqInput, EmbeddingReqInput] ) -> bool: - """Check if the request should be dispatched to encoder for processing. - - Returns True if the request should be dispatched to encoder (multiple multimodal items), - False if it should be processed locally (single multimodal item or no multimodal items). - - Args: - obj: The request input object - - Returns: - bool: True if should dispatch to encoder, False otherwise - """ if obj.batch_size > 1: logger.warning( "Batch request (batch_size=%d) is not supported in EPD disaggregation mode; skipping encoder dispatch.", diff --git a/python/sglang/srt/managers/tokenizer_manager_score_mixin.py b/python/sglang/srt/managers/tokenizer_manager_score_mixin.py index 6d5475517..d7a5b6e6a 100644 --- a/python/sglang/srt/managers/tokenizer_manager_score_mixin.py +++ b/python/sglang/srt/managers/tokenizer_manager_score_mixin.py @@ -38,18 +38,6 @@ class TokenizerManagerScoreMixin: This is a thin wrapper over `score_request` that treats `prompts` as already-composed inputs (i.e., no query/item concatenation needed). - - Args: - prompts: A single prompt string, a list of prompt strings, or a list of - pre-tokenized prompt token ID sequences. - label_token_ids: Token IDs to compute probabilities for. - apply_softmax: Whether to normalize probabilities using softmax. - request: Optional FastAPI request object. - - Returns: - ScoreResult with: - scores: List of score lists, one for each prompt, each in the order of label_token_ids. - prompt_tokens: The number of prompt tokens processed. """ # Text prompts if isinstance(prompts, str) or ( @@ -83,14 +71,6 @@ class TokenizerManagerScoreMixin: """ Build a single token sequence for multi-item scoring. Format: queryitem1item2item3 - - Args: - query: Query token IDs - items: List of item token ID sequences - delimiter_token_id: Token ID to use as delimiter - - Returns: - Tuple of (combined token sequence, delimiter indices) """ combined_sequence = query[:] # Start with query delimiter_indices = [] @@ -111,16 +91,6 @@ class TokenizerManagerScoreMixin: query: Optional[Union[str, List[int]]], items: Optional[Union[str, List[str], List[List[int]]]], ) -> Tuple[List[int], List[List[int]]]: - """ - Tokenize query and items into token IDs. - - Args: - query: The query text (str) or pre-tokenized token IDs (List[int]). - items: Item texts or pre-tokenized token IDs. - - Returns: - (query_ids, items_ids): query token IDs and list of per-item token IDs. - """ if isinstance(query, str): query_ids = self.tokenizer.encode(query) else: @@ -153,20 +123,6 @@ class TokenizerManagerScoreMixin: populated (input_token_ids_logprobs for generation models, embedding for classification models), then uniformly validates, skips the query-boundary delimiter, and normalizes. - - Args: - results: Results from generate_request - items: List of items being scored - label_token_ids: Token IDs to extract scores for - apply_softmax: Whether to apply softmax normalization - batch_request: The original batch request containing input sequence - return_pooled_hidden_states: Whether to extract pooled hidden states - from the result and include them in the ScoreResult. - - Returns: - ScoreResult with per-item scores, prompt token count, and optional - pooled_hidden_states (when return_pooled_hidden_states=True and the - model populated the field). """ single_result = results[0] if isinstance(results, list) else results meta_info = single_result.get("meta_info", {}) @@ -246,15 +202,6 @@ class TokenizerManagerScoreMixin: For generation (CausalLM) models: reads output_token_ids_logprobs. For non-generation (SequenceClassification) models: reads the embedding field which contains pooled class logits from the classification head. - - Args: - results: Results from generate_request - label_token_ids: Token IDs to extract scores for (generation models only) - apply_softmax: Whether to apply softmax normalization - return_pooled_hidden_states: Whether to extract pooled hidden states - - Returns: - ScoreResult with per-item scores, prompt token count, and optional pooled_hidden_states. """ scores = [] phs_list = [] @@ -329,17 +276,7 @@ class TokenizerManagerScoreMixin: label: str = "input", ) -> Tuple[List[torch.Tensor], List[int]]: """Scan token_ids for placeholder occurrences and pair with embeddings. - - Args: - token_ids: The token sequence to scan. - embeds: Embedding tensors to place at placeholder positions (None = skip). - embed_override_token_id: The placeholder token ID. - position_offset: Added to each found position (for absolute coordinates). - label: Label for error messages (e.g. "query", "items[2]"). - - Returns: - (embeds, positions) lists. Empty lists if embeds is None. - """ + Returns empty lists when embeds is None.""" if embeds is None: return [], [] positions = [ @@ -365,10 +302,7 @@ class TokenizerManagerScoreMixin: item_position_offset: int, item_label: str, ) -> Optional[PositionalEmbeds]: - """Resolve embed overrides for a single query+item pair. - - Returns PositionalEmbeds if any overrides exist, None otherwise. - """ + """Resolve embed overrides for a query+item pair; None when no overrides exist.""" q_embeds, q_positions = self._resolve_overrides_for_sequence( query, query_embed_overrides, @@ -405,11 +339,8 @@ class TokenizerManagerScoreMixin: ) -> Tuple[None, List[List[int]], Optional[list], Optional[List[int]]]: """Build input_ids and resolve embed overrides for token-ID inputs. - Works identically for multi-item-scoring and single-item modes — the only difference is - how input_ids are assembled and what position offset each item gets. - - Returns: - (text_prompts, input_ids, positional_embed_overrides, delimiter_indices) + Multi-item-scoring and single-item modes differ only in how input_ids + are assembled and what position offset each item gets. """ # Both query and items are token IDs has_embeds = ( @@ -540,28 +471,8 @@ class TokenizerManagerScoreMixin: - Generation (CausalLM): Requires label_token_ids; returns logprob-based scores. - SequenceClassification: label_token_ids is optional; returns pooled class logits. - Args: - query: The query text or pre-tokenized query token IDs - items: The item text(s) or pre-tokenized item token IDs - label_token_ids: List of token IDs to compute probabilities for - apply_softmax: Whether to normalize probabilities using softmax - item_first: If True, prepend items to query. Ignored for multi-item scoring. - embed_override_token_id: Placeholder token ID for embedding override positions. - query_embed_overrides: Embedding vectors replacing placeholder tokens in query. - item_embed_overrides: Per-item embedding vectors replacing placeholder tokens in items. - request: Optional FastAPI request object - return_pooled_hidden_states: Whether to include the raw pooled transformer - hidden states (before the task-specific head) in the result. Only - supported for non-generation models (SequenceClassification, - RewardModel). Raises ValueError for CausalLM models. - - Returns: - ScoreResult with: - scores: List of score lists, one per item. - prompt_tokens: The number of prompt tokens processed. - pooled_hidden_states: Per-item CPU tensors when - return_pooled_hidden_states=True and the model supports it; - None otherwise. + return_pooled_hidden_states is only supported for non-generation models + (SequenceClassification, RewardModel); raises ValueError for CausalLM. """ is_generation = self.is_generation @@ -734,17 +645,6 @@ class TokenizerManagerScoreMixin: label_token_ids: List[int], apply_softmax: bool, ) -> List[float]: - """ - Convert logprobs dictionary to ordered score list. - - Args: - logprobs: Dictionary mapping token_id to logprob - label_token_ids: Token IDs in desired order - apply_softmax: Whether to apply softmax normalization - - Returns: - List of scores in the same order as label_token_ids - """ score_list = [ logprobs.get(token_id, float("-inf")) for token_id in label_token_ids ] @@ -762,16 +662,7 @@ class TokenizerManagerScoreMixin: def _extract_logprobs_for_tokens( self, logprobs_data: List, label_token_ids: List[int] ) -> Dict[int, float]: - """ - Extract logprobs for specified token IDs from logprobs data. - - Args: - logprobs_data: List of (logprob, token_id, text) tuples - label_token_ids: Token IDs to extract logprobs for - - Returns: - Dictionary mapping token_id to logprob - """ + """Extract logprobs for label_token_ids from (logprob, token_id, text) tuples.""" logprobs = {} if logprobs_data: for logprob, token_id, _ in logprobs_data: