Skip to content

coding_standards.md

Latest commit

 

History

History
376 lines (275 loc) · 17.9 KB

coding_standards.md

File metadata and controls

376 lines (275 loc) · 17.9 KB

LMCache Code Quality and Review Standard

This document defines the coding quality standards and code review expectations for the LMCache project. All contributors (human and AI) should follow these standards. Reviewers should use this as the authoritative reference when reviewing PRs.

Source of truth: This file is the canonical reference. AGENTS.md contains the quick-reference checklist; CLAUDE.md and .gemini/styleguide.md point here for details.

1. General Rules

1.1 PR Scope and Breakdown

  • PRs must be small and focused. If the number of changed files is large, break it down conceptually into multiple PRs. Each PR should do one thing well.
  • Prefer a series of small, reviewable PRs over one large omnibus PR.
  • If a feature spans multiple modules, split by module boundary or by logical phase (e.g., interface change first, then implementation, then tests).

1.2 Documentation Requirements

  • User docs (docs/source/) and design docs (docs/design/) must be updated whenever there are user-facing changes or new functionalities.
  • Docstrings must be updated whenever there are changes to existing functions, methods, or class fields.
  • If there are documents specifying schemas of arguments or return values, update them as well.
  • Code owners must be updated if ownership changes. Reviewers should remind PR authors.

1.3 License Header

All Python files must have # SPDX-License-Identifier: Apache-2.0 as the first line.

2. Typing

2.1 Strong Typing is Enforced

  • All functions and methods must have type hints for arguments and return values.
  • Avoid Any. If the type is not clear, redesign the interface until it is.
  • Generic containers must be fully typed:
    • Use list[SomeType], not bare list.
    • Use dict[KeyType, ValueType], not bare dict.
    • Use tuple[int, str] or tuple[int, ...], not bare tuple.
  • Avoid Optional. Prefer always initializing objects (even if empty/unused) over wrapping in Optional. If a value can genuinely be absent, use X | None syntax rather than Optional[X].

2.2 Runtime Validation

  • Never use assert for runtime-dependent checks (e.g., config validation, input validation). Assertions are stripped by python -O. Use explicit if ...: raise ValueError(...) instead.
  • Only use assert for development-time invariant checks that indicate programmer errors.

3. Docstrings

3.1 When Docstrings Are Required

Scope Requirement
Public functions and methods Full docstring (always required)
Module-level / global helper functions Full docstring
Long private helper functions Full docstring
Short, clear class-private helpers Short docstring acceptable
Override methods with no new behavior Short docstring acceptable

3.2 Full Docstring Format

A full docstring must include:

  1. Summary: A clear one-sentence description of what the function does.
  2. Details (when needed): A few more sentences covering:
    • Thread safety guarantees or requirements
    • Assumptions about the caller/callee
    • Important invariants
  3. Args: Detailed description of every argument, including:
    • Type and purpose
    • Assumptions or constraints (e.g., "length of keys must equal length of values")
  4. Returns: Detailed description of the return value.
  5. Raises: All exceptions the function may raise.
  6. Note (when needed): Important assumptions or caveats that developers should be aware of.

3.3 Docstring Accuracy

  • Docstrings must match the function's actual current behavior, not planned future behavior.
  • When modifying a function, always update the docstring to reflect the change.
  • If a parameter is currently a no-op (accepted but not yet used), the docstring must say so explicitly. Do not describe behavior that is not yet implemented.

4. Function and Interface Design

4.1 Naming

  • Function names must be self-explanatory. A developer should understand the behavior without reading the implementation.
  • Parameter names should imply semantics. For predicates/filters, name them to indicate what True/False means (e.g., key_eligible_filter rather than just filter).
  • Variable naming must be consistent across files and modules.
  • Private methods and private members start with _.

4.2 Self-Contained Functions

  • Functions should be self-contained. If a function's input is expected to come from another specific function (or its output is expected to feed into another), this must be documented explicitly in the docstring.

4.3 Parameter and Return Value Design

  • Avoid ambiguity in return values. For example, returning None for both "process not finished" and "request not found" makes it impossible for the caller to distinguish. Use distinct return types or raise exceptions.
  • Document schemas if the parameter or return value includes dict or complex containers. Specify the expected keys, value types, and semantics.
  • Avoid boolean parameters. They make call sites unreadable. Prefer:
    • Breaking into two separate functions, or
    • Using an enum to make intent explicit.
  • Clarify caller assumptions in the docstring (e.g., "caller must hold the lock", "must be called from the main thread").

4.4 Class Design

  • Minimize the public interface. Follow the principle of minimal surface area -- only add a new public method or property when there is a concrete need.
  • Class fields with internal state must have:
    • A clear, documented meaning
    • A clear invariant (at any point in time, a developer should know what the value should be without running the code)
    • A docstring or inline comment explaining it
  • Private members must never be directly accessed by other classes. Interact only through public APIs. This is enforced by CI in lmcache/v1/multiprocess/ and lmcache/v1/distributed/, but applies project-wide.
  • Prefer @property that derives from existing data over storing redundant fields.

4.5 Config and Dispatch

  • Use isinstance() on config objects to determine behavior, not string-based type names.
  • Config classes should follow existing conventions (e.g., @dataclass). Controllers and managers should always be created (even with empty/default config), never wrapped in Optional.

5. New Functionality

5.1 Design Documentation

  • A design doc is required for non-trivial new features or architectural changes.
  • docs/design/ mirrors the lmcache/ package tree. A design doc for code at lmcache/<path>/ lives at docs/design/<path>/. For example, design docs for lmcache/v1/distributed/l2_adapters/ go under docs/design/v1/distributed/l2_adapters/. A doc that spans several sibling submodules belongs at their common parent directory.
  • Use descriptive file names (e.g., overall.md, l2_eviction.md, query-command.md) rather than mirroring module file names. See docs/design/README.md for the full convention.
  • User-facing READMEs (README.md in module directories) stay next to the code and are symlinked from the corresponding docs/design/<path>/ location — do not move them.

5.2 Testing

  • All new features must include corresponding unit tests.
  • All bug fixes must include regression tests.
  • Tests must:
    • Verify the public interface and docstring contract, not implementation details.
    • Be written as if the function's implementation is unknown.
    • Not access private members (_-prefixed) of the class under test.
    • Not test private helper functions directly. Exercise them through the public API.
  • Test files should mirror the source structure under tests/.

5.3 Reuse Existing Code

  • When adding or updating functions, variables, or logic, check whether there is existing code that can be reused or existing state that can be exported instead of duplicating.

6. Modification to Existing Code

6.1 Documentation and Schema Updates

  • Must update docstrings for any modified functions or fields.
  • Should update user docs and design docs if the change affects behavior or APIs.
  • Update schema documents if argument or return value structures change.
  • Check for existing reusable code before introducing new helpers or utilities.

6.2 Caller Impact Analysis (Global View)

Any change to an existing function, class, or module must be evaluated in the context of its callers, not just the file being edited. A local change that looks correct in isolation can silently break unchanged code that depends on the prior contract. Authors and reviewers must both take this global view.

When caller analysis is required:

  • Adding, removing, renaming, or reordering function parameters
  • Changing a parameter's type, default value, or accepted domain (e.g., allowing None where it wasn't before, or narrowing an accepted type)
  • Changing the return type, return semantics, or the meaning of sentinel values (e.g., what None means)
  • Adding, removing, or changing the exceptions a function raises
  • Changing side effects, ordering guarantees, thread-safety guarantees, or blocking vs. non-blocking behavior
  • Changing invariants of a class field that other classes read via a public accessor
  • Modifying a public method that is part of an interface or base class (subclasses are callers too)

How to perform the analysis:

  1. Find every caller of the changed symbol across the codebase, including:
    • Non-test production code
    • Unit tests and integration tests
    • Documentation snippets and examples
    • Downstream integrations (e.g., lmcache/integration/vllm/)
  2. For each caller, verify:
    • The new signature is compatible (keyword-arg callers are especially sensitive to renames).
    • Caller assumptions about return values, exceptions, and side effects still hold.
    • If the caller passes the function's output to another function, that downstream contract still holds.
  3. Update callers in the same PR when a breaking change is genuinely required. Do not leave callers broken with the intent to fix them later.
  4. Prefer backwards-compatible extensions (new parameter with a default, new optional return field) over breaking changes unless there is a strong reason.
  5. If the change affects a base class or protocol, check all implementers, not just direct callers.

When in doubt, err on the side of reading one more caller. The cost of reading five call sites is small; the cost of a latent bug that ships because no one looked is large.

7. Code Organization and Style

7.1 File Structure

  • Module-level helper functions go at the top of the file (after imports, before classes).
  • Private/helper methods within a class go at the end of the class, after all public methods.
  • Code file length should not be too long. If a file is growing large, think about breaking it into smaller modules.

7.2 Import Ordering

Imports must follow this section-heading convention:

# Standard
import os

# Third Party
import torch

# First Party
from lmcache.v1.config import LMCacheEngineConfig

# Local
from .utils import helper
  • All imports must be at the top of the file. No lazy imports.
  • Always import torch before importing native C extensions (lmcache.c_ops, etc.).

7.3 Formatting and Linting

# Run all checks (mirrors CI)
pre-commit run --all-files

# Individual tools
ruff check .              # Lint (E, F, B, SLF rules)
ruff format .             # Format (line-length 88)
isort .                   # Import sorting (black profile, from_first=true)
mypy --config-file=pyproject.toml   # Type checking
codespell --toml pyproject.toml     # Spell checking

C++/CUDA files use clang-format (Google style, 80-col). Rust code uses cargo fmt and cargo clippy.

7.4 Logging

  • Use the project's init_logger(__name__) from lmcache.logging, not bare logging.getLogger().
  • Operational logs (store/retrieve activity, background task progress) should be at DEBUG level, not INFO.
  • Log levels must be appropriate: do not use WARNING for conditions that are expected during normal concurrent operation (e.g., a key not found due to a benign race condition).

7.5 Error Handling

  • Replace assert with if/raise for all runtime validation (see Section 2.2).
  • Error/failure paths must not leave the system in an inconsistent state. Check:
    • Locks are released
    • Pool entries are freed
    • File descriptors are closed
    • Partial state is cleaned up on failure

7.6 Resource Management

  • Unbounded collections are memory leaks. If a set or dict grows over time (e.g., tracking seen keys), ensure entries are cleaned up when the corresponding resource is freed.
  • CUDA/GPU resources must be properly managed (allocated, freed, synchronized).
  • No unnecessary memory copies or allocations in hot paths.

8. Thread Safety

  • All shared state access must be protected by appropriate locks.
  • Lock granularity should be appropriate -- not too coarse (blocking unrelated operations) and not too fine (risking missed synchronization).
  • Even test-only or debug methods must hold self._lock for thread safety.
  • When code runs under multiple controller threads (e.g., StoreController + PrefetchController), verify that concurrent access is safe.
  • Start with the simplest correct design. Only introduce threading, locks, and queues when there is a concrete need that cannot be met otherwise.

9. Code Review Process

9.1 Review Focus

Reviews should focus on:

  1. Design decisions and architectural soundness -- is this the right approach?
  2. Correctness -- logic bugs, error handling, resource management, edge cases.
  3. Caller impact (global view) -- do the changes break any unchanged callers, tests, or subclasses? (See Section 6.2.)
  4. Thread safety -- shared state, lock protocols, concurrent access patterns.
  5. Code cleanliness and maintainability -- naming, modularity, readability.
  6. Documentation -- docstrings, design docs, user docs.
  7. Test coverage -- especially failure paths and concurrent access.
  8. Design doc compliance -- does the implementation match documented contracts?

Reviews should not focus on:

  • Security-related issues (out of scope for most changes)
  • Trivial style matters already caught by linters
  • Unrelated refactors of unchanged code (only flag unchanged code when it is a caller of a changed symbol and the change affects it; do not report unrelated issues in unchanged code)

9.2 Severity Levels

Severity Meaning Examples
error Must fix before merge Missing SPDX header, missing type hints on public function, missing docstring on public function, new feature with zero tests, cross-class private member access, poor architectural decision, ambiguous return values, assert used for runtime validation, breaking change that leaves unchanged callers broken
warning Should fix Docstring could be more detailed, tests that test implementation details, naming could be clearer, code could be more modular, missing strict=True on zip of parallel lists, caller assumptions weakened without audit
info Suggestion only, non-blocking Optional naming improvement, minor refactor for readability, style preference, backwards-compatible extension where a cleaner API break might be justified

9.3 Reviewer Guidelines

  • Only review lines in the diff. Do not report issues in unchanged code.
  • Do not praise the code. Only report issues or confirm the PR is clean.
  • Do not pad findings. If the PR is clean, say so.
  • Be precise about file paths and line numbers.
  • If unsure whether something is an issue, treat it as info, not error.
  • Do not leave trivial or nitpick comments that do not affect correctness, maintainability, or readability.

9.4 Review Output Format

  1. Start with a short summary of what the PR does.
  2. Present design doc compliance as a table (requirement | pass/fail | notes) if a relevant design doc exists.
  3. List issues grouped by severity (error > warning > info).
  4. End with a summary table: severity | count | key items.

10. Quick Reference Checklist

Use this checklist before submitting a PR or during review:

Correctness

  • Code does what it claims; matches PR description
  • Edge cases handled (empty inputs, None values, boundary conditions)
  • Error/failure paths do not leave inconsistent state
  • No regressions (existing tests still pass)

Typing and Style

  • pre-commit run --all-files passes
  • All functions have type hints (arguments + return values)
  • No use of Any or bare generic containers
  • License header present on all Python files
  • Import ordering follows section-heading convention

Documentation

  • All public functions have complete docstrings
  • Docstrings match actual behavior
  • Design docs updated for non-trivial changes
  • User docs updated for user-facing changes

Encapsulation and Design

  • No cross-class private member access
  • Public APIs minimal and well-defined
  • Module-level helpers at top; private methods at end of class
  • No boolean parameters (use enum or split into separate functions)
  • No ambiguous return values

Caller Impact (Global View)

  • All callers of changed signatures still compile/pass type checks
  • All callers still handle the new return type / new exceptions correctly
  • Subclasses/implementers of modified base classes or protocols updated
  • Tests and integration code (tests/, lmcache/integration/) checked, not just production paths

Testing

  • New features have corresponding tests
  • Bug fixes have regression tests
  • Tests target public interface, not implementation details
  • Tests do not access private members

Safety and Performance

  • No assert for runtime validation (use if/raise)
  • No unbounded collection growth
  • Thread safety maintained for shared state
  • No unnecessary memory copies in hot paths
  • CUDA/GPU resources properly managed