diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index efefad99..d22a775d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -45,17 +45,19 @@ repos: hooks: # Static guard that every recipe `_target_` still resolves to a real symbol, # so a rename can't leave a dead Hydra path to fail only at launch time. + # A dead path can only appear via a recipe edit or a Python rename/removal, so + # those extensions gate the (always full-tree) scan; `--all-files` CI is unaffected. - id: check-recipe-targets name: recipe _target_ paths resolve - entry: python scripts/check_recipe_targets.py + entry: python lint/check_recipe_targets.py language: python pass_filenames: false - always_run: true + files: \.(ya?ml|py)$ # Experimental-tier boundaries: core never imports experimental, # packages never import each other, requirements stay additive-only. - id: check-experimental-boundaries name: experimental-tier boundaries hold - entry: python scripts/check_experimental_boundaries.py + entry: python lint/check_experimental_boundaries.py language: python pass_filenames: false always_run: true diff --git a/CLAUDE.md b/CLAUDE.md index c58f1791..a3fa86c5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,6 +84,10 @@ Fail closed when the work is not ready: - If the change is duplicate, too trivial, missing context, or lacks a credible verification path, stop and explain what is missing. - Do not invent process exceptions just to keep moving. +Verification harnesses and guard scripts: +- `lint/` holds only guard scripts wired into `.pre-commit-config.yaml`; a file there that no hook `entry:` references does not belong in the repo. +- One-off verification harnesses written to prove a PR correct are run and their commands + results quoted in the PR's Test Plan, **not committed**. The tests/ tree was removed by policy (#99/#267); do not recreate it under any name (`tests/`, `scripts/`, `tools/`, ...). + ## 6. Review and Domain Guides **Verify guidance against the current repo before applying it.** diff --git a/experimental/README.md b/experimental/README.md index 99e13382..86fae5f7 100644 --- a/experimental/README.md +++ b/experimental/README.md @@ -35,7 +35,7 @@ graduates into core instead of being borrowed sideways. ## Rules (lint-enforced where possible) -1. **Import direction** (`scripts/check_experimental_boundaries.py`): +1. **Import direction** (`lint/check_experimental_boundaries.py`): core never imports `experimental`; packages never import each other. 2. **Additive-only requirements** (same script): reward and actor share one Python process, so a `requirements.txt` cannot version-"isolate" — @@ -47,7 +47,7 @@ graduates into core instead of being borrowed sideways. `pyproject.toml` only — no version-compat branches; a wrong environment fails loudly and the user aligns the environment. 4. **`_target_` hygiene**: every dotpath in `experimental/**` configs - must resolve (`scripts/check_recipe_targets.py` scans this tier). + must resolve (`lint/check_recipe_targets.py` scans this tier). 5. **Owner + verification**: each package README carries its owner and a verification table (config × hardware × head × status). Unverified drive-by configs are rejected in review. diff --git a/scripts/check_experimental_boundaries.py b/lint/check_experimental_boundaries.py similarity index 100% rename from scripts/check_experimental_boundaries.py rename to lint/check_experimental_boundaries.py diff --git a/lint/check_recipe_targets.py b/lint/check_recipe_targets.py new file mode 100755 index 00000000..ec155865 --- /dev/null +++ b/lint/check_recipe_targets.py @@ -0,0 +1,150 @@ +#!/usr/bin/env python3 +"""Static guard: every ``_target_`` in a recipe must resolve to a real symbol. + +Renames (e.g. ``unirl.algorithms.ar_grpo.ARGRPO`` -> ``unirl.algorithms.grpo.GRPO``) +break Hydra ``instantiate`` only at *runtime*, and this repo's CI is lint-only, so a +stale dotted path can merge silently. This check parses every recipe ``_target_:`` +pointing into one of the ``PACKAGES`` trees and confirms the module file and the +attribute exist — purely via ``ast``, importing nothing (no torch/vllm/sglang needed). + +Only ~0.2s of the runtime is parsing; the rest is filesystem latency, which dominates +when the checkout lives on a network filesystem. So the tree is walked once to index +every module (rather than probing candidate paths per target, which costs ~10k stat +calls) and file contents are read through a thread pool. On CephFS that takes the +hook from ~5m30s to ~26s. + +Run by the ``check-recipe-targets`` pre-commit hook (so it rides the existing +``pre-commit run --all-files`` lint CI). Exits non-zero, listing each unresolved +target, when any path is dead. +""" + +from __future__ import annotations + +import ast +import fnmatch +import os +import re +import sys +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] + +# YAML trees that hold recipes / stage configs with ``_target_`` entries. +SCAN_DIRS = ["examples", "experimental", "CPPO", "DRPO", "FlowDPPO", "unirl"] +# Vendored / sub-project trees kept byte-pristine (mirror .pre-commit-config exclude). +SKIP_PARTS = {".git", "vendor"} +# The only packages ``_TARGET_RE`` accepts, so the only ones worth indexing; the +# regex alternation is derived from this tuple so the two cannot drift apart. +PACKAGES = ("unirl", "experimental") + +_TARGET_RE = re.compile(r"""^\s*_target_:\s*['"]?((?:%s)\.[A-Za-z0-9_.]+)['"]?\s*$""" % "|".join(PACKAGES)) + +# Deep enough to hide network-filesystem round trips behind each other. +_READ_THREADS = 32 + + +def _scan() -> tuple[dict[str, Path], list[Path]]: + """One walk over ``SCAN_DIRS``: dotted module path -> file, plus every recipe file.""" + modules: dict[str, Path] = {} + packages: dict[str, Path] = {} + recipes: list[Path] = [] + for d in SCAN_DIRS: + for dirpath, _dirnames, filenames in os.walk(ROOT / d): + parts = Path(dirpath).relative_to(ROOT).parts + # Only an importable directory chain can be named by a dotted ``_target_``. + importable = parts[0] in PACKAGES and all(p.isidentifier() for p in parts) + skipped = bool(SKIP_PARTS & set(parts)) + for name in filenames: + path = Path(dirpath, name) + if importable and name.endswith(".py"): + stem = name[:-3] + if stem == "__init__": + # Both ``pkg`` and the explicit ``pkg.__init__`` spelling reach it. + packages[".".join(parts)] = path + packages[".".join((*parts, stem))] = path + elif stem.isidentifier(): + modules[".".join((*parts, stem))] = path + elif not skipped and fnmatch.fnmatch(name, "*.y*ml"): + recipes.append(path) + # A module file shadows a package of the same name, keeping the probe order this + # check has always used (``pkg/sub.py`` before ``pkg/sub/__init__.py``); note that + # Python's own import machinery resolves the other way round. + return {**packages, **modules}, sorted(recipes) + + +def _read_all(paths: list[Path]) -> list[str]: + with ThreadPoolExecutor(max_workers=_READ_THREADS) as pool: + return list(pool.map(lambda p: p.read_text(encoding="utf-8"), paths)) + + +def _top_level_names(source: str, path: Path) -> frozenset[str] | None: + """Top-level names bound in ``source`` (class/func/assign/import), or None.""" + try: + tree = ast.parse(source, filename=path) + except SyntaxError: + return None + names: set[str] = set() + for node in tree.body: + if isinstance(node, (ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)): + names.add(node.name) + elif isinstance(node, ast.Assign): + for t in node.targets: + if isinstance(t, ast.Name): + names.add(t.id) + elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): + names.add(node.target.id) + elif isinstance(node, (ast.Import, ast.ImportFrom)): + for alias in node.names: + names.add(alias.asname or alias.name.split(".")[0]) + return frozenset(names) + + +def _split_module(dotted: str, modules: dict[str, Path]) -> tuple[Path, str] | None: + """Longest module prefix of ``dotted`` that exists, with the attribute after it. + + Walks the standard Python split: try module = all-but-last part, attr = last; + if that module does not exist, fold trailing parts back into the attribute chain + until one does. None when no prefix names a module at all. + """ + parts = dotted.split(".") + for split in range(len(parts) - 1, 0, -1): + module_file = modules.get(".".join(parts[:split])) + if module_file is not None: + return module_file, parts[split] + return None + + +def main() -> int: + modules, recipes = _scan() + + targets: list[tuple[Path, int, str]] = [] + for path, text in zip(recipes, _read_all(recipes)): + for lineno, line in enumerate(text.splitlines(), 1): + m = _TARGET_RE.match(line) + if m: + targets.append((path, lineno, m.group(1))) + + module_split = {dotted: _split_module(dotted, modules) for _, _, dotted in targets} + # Only the modules a recipe actually names are worth reading and parsing. + needed = sorted({hit[0] for hit in module_split.values() if hit is not None}) + top_level = {path: _top_level_names(source, path) for path, source in zip(needed, _read_all(needed))} + + failures: list[str] = [] + for path, lineno, dotted in targets: + hit = module_split[dotted] + names = top_level[hit[0]] if hit is not None else None + if names is None or hit[1] not in names: + failures.append(f"{path.relative_to(ROOT)}:{lineno}: unresolved _target_ '{dotted}'") + + if failures: + print("Unresolved recipe _target_ paths (rename leftover or typo):", file=sys.stderr) + for f in failures: + print(f" {f}", file=sys.stderr) + return 1 + print(f"check-recipe-targets: {len(targets)} recipe _target_ paths resolve.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/check_recipe_targets.py b/scripts/check_recipe_targets.py deleted file mode 100755 index eeb1a7d6..00000000 --- a/scripts/check_recipe_targets.py +++ /dev/null @@ -1,103 +0,0 @@ -#!/usr/bin/env python3 -"""Static guard: every ``_target_`` in a recipe must resolve to a real symbol. - -Renames (e.g. ``unirl.algorithms.ar_grpo.ARGRPO`` -> ``unirl.algorithms.grpo.GRPO``) -break Hydra ``instantiate`` only at *runtime*, and this repo's CI is lint-only, so a -stale dotted path can merge silently. This check parses every recipe ``_target_:`` -pointing into the ``unirl`` package and confirms the module file and the attribute -exist — purely via ``ast``, importing nothing (no torch/vllm/sglang needed). - -Run by the ``check-recipe-targets`` pre-commit hook (so it rides the existing -``pre-commit run --all-files`` lint CI). Exits non-zero, listing each unresolved -target, when any path is dead. -""" - -from __future__ import annotations - -import ast -import re -import sys -from functools import lru_cache -from pathlib import Path - -ROOT = Path(__file__).resolve().parents[1] - -# YAML trees that hold recipes / stage configs with ``_target_`` entries. -SCAN_DIRS = ["examples", "experimental", "CPPO", "DRPO", "FlowDPPO", "unirl"] -# Vendored / sub-project trees kept byte-pristine (mirror .pre-commit-config exclude). -SKIP_PARTS = {".git", "vendor"} - -_TARGET_RE = re.compile(r"""^\s*_target_:\s*['"]?((?:unirl|experimental)\.[A-Za-z0-9_.]+)['"]?\s*$""") - - -@lru_cache(maxsize=None) -def _module_top_level_names(module_file: Path) -> frozenset[str] | None: - """Top-level names bound in ``module_file`` (class/func/assign/import), or None.""" - try: - tree = ast.parse(module_file.read_text(encoding="utf-8"), filename=str(module_file)) - except (OSError, SyntaxError): - return None - names: set[str] = set() - for node in tree.body: - if isinstance(node, (ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)): - names.add(node.name) - elif isinstance(node, ast.Assign): - for t in node.targets: - if isinstance(t, ast.Name): - names.add(t.id) - elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): - names.add(node.target.id) - elif isinstance(node, (ast.Import, ast.ImportFrom)): - for alias in node.names: - names.add(alias.asname or alias.name.split(".")[0]) - return frozenset(names) - - -def _resolve(dotted: str) -> bool: - """True if ``dotted`` (e.g. unirl.algorithms.grpo.GRPO) names a real module attr. - - Walks the standard Python split: try module = all-but-last part, attr = last; - if that module file is missing, fold trailing parts back into the attribute chain - until a module file exists, then check the first attribute after it is top-level. - """ - parts = dotted.split(".") - for split in range(len(parts) - 1, 0, -1): - mod_parts, attr_parts = parts[:split], parts[split:] - base = ROOT.joinpath(*mod_parts) - module_file = base.with_suffix(".py") - if not module_file.is_file(): - module_file = base / "__init__.py" - if not module_file.is_file(): - continue # not a module here — fold one more part into the attr chain - names = _module_top_level_names(module_file) - return names is not None and attr_parts[0] in names - return False - - -def main() -> int: - failures: list[str] = [] - checked = 0 - for d in SCAN_DIRS: - for path in sorted((ROOT / d).rglob("*.y*ml")): - if SKIP_PARTS & set(path.relative_to(ROOT).parts): - continue - for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): - m = _TARGET_RE.match(line) - if not m: - continue - checked += 1 - if not _resolve(m.group(1)): - rel = path.relative_to(ROOT) - failures.append(f"{rel}:{lineno}: unresolved _target_ '{m.group(1)}'") - - if failures: - print("Unresolved recipe _target_ paths (rename leftover or typo):", file=sys.stderr) - for f in failures: - print(f" {f}", file=sys.stderr) - return 1 - print(f"check-recipe-targets: {checked} unirl _target_ paths resolve.") - return 0 - - -if __name__ == "__main__": - sys.exit(main())