From ae273f36ad240f96be412ac1a58150eea58095d3 Mon Sep 17 00:00:00 2001 From: CjhHa1 Date: Thu, 30 Jul 2026 23:18:22 +0800 Subject: [PATCH 1/4] perf(lint): make the recipe _target_ guard filesystem-cheap check_recipe_targets.py probed candidate module paths per target, so the 2286 targets cost ~10k stat calls, nearly all of them negative lookups. That is invisible on local disk but brutal on a network checkout: on CephFS the hook took 5m36s, of which only 0.7s was CPU. Since it carries always_run, every commit and every push paid it. Walk the tree once into a dotted-path -> file index, read file contents through a thread pool, and parse only the modules a recipe actually names. Same output, same exit codes: an old-vs-new comparison over 933 dotted paths (the real targets plus mutations covering renamed symbols, renamed intermediate modules, package-only paths, vendored trees and deeper attribute chains) agrees on all of them, 446 resolving and 487 not. CephFS 5m36s -> 25.9s back to back on the same checkout; local disk is unchanged at ~0.2s. Also gate the hook on yaml/py edits rather than always_run, since only those can strand a _target_; `--all-files` CI still runs it. --- .pre-commit-config.yaml | 4 +- scripts/check_recipe_targets.py | 106 ++++++++++++++++++++++---------- 2 files changed, 75 insertions(+), 35 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ef24c1faf..454a99573 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -45,9 +45,11 @@ 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 language: python pass_filenames: false - always_run: true + files: \.(ya?ml|py)$ diff --git a/scripts/check_recipe_targets.py b/scripts/check_recipe_targets.py index ceb14ba3f..50874ad4f 100755 --- a/scripts/check_recipe_targets.py +++ b/scripts/check_recipe_targets.py @@ -7,6 +7,12 @@ pointing into the ``unirl`` package 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 is the +difference between ~5min and ~15s per run. + 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. @@ -15,9 +21,11 @@ from __future__ import annotations import ast +import fnmatch +import os import re import sys -from functools import lru_cache +from concurrent.futures import ThreadPoolExecutor from pathlib import Path ROOT = Path(__file__).resolve().parents[1] @@ -26,16 +34,46 @@ SCAN_DIRS = ["examples", "CPPO", "DRPO", "FlowDPPO", "unirl"] # Vendored / sub-project trees kept byte-pristine (mirror .pre-commit-config exclude). SKIP_PARTS = {".git", "vendor"} +# The only package ``_TARGET_RE`` accepts, so the only one worth indexing. +PACKAGE = "unirl" _TARGET_RE = re.compile(r"""^\s*_target_:\s*['"]?(unirl\.[A-Za-z0-9_.]+)['"]?\s*$""") +# Deep enough to hide network-filesystem round trips behind each other. +_READ_THREADS = 32 + -@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.""" +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 + skipped = bool(SKIP_PARTS & set(parts)) + for name in filenames: + if parts[0] == PACKAGE and name.endswith(".py"): + if name == "__init__.py": + packages[".".join(parts)] = Path(dirpath, name) + else: + modules[".".join((*parts, name[:-3]))] = Path(dirpath, name) + elif not skipped and fnmatch.fnmatch(name, "*.y*ml"): + recipes.append(Path(dirpath, name)) + # A module file shadows a package of the same name, as in Python's own lookup. + 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(module_file.read_text(encoding="utf-8"), filename=str(module_file)) - except (OSError, SyntaxError): + tree = ast.parse(source, filename=str(path)) + except SyntaxError: return None names: set[str] = set() for node in tree.body: @@ -53,49 +91,49 @@ def _module_top_level_names(module_file: Path) -> frozenset[str] | None: return frozenset(names) -def _resolve(dotted: str) -> bool: - """True if ``dotted`` (e.g. unirl.algorithms.grpo.GRPO) names a real module attr. +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 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. + 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): - 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 + 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] = [] - 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)}'") + 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: {checked} unirl _target_ paths resolve.") + print(f"check-recipe-targets: {len(targets)} unirl _target_ paths resolve.") return 0 From 5eb6178b9fa49a0311be56849bdc1cc3660c06d4 Mon Sep 17 00:00:00 2001 From: CjhHa1 Date: Fri, 31 Jul 2026 15:11:58 +0800 Subject: [PATCH 2/4] perf(lint): close review gaps in the recipe _target_ index - Index each package initializer under both ``pkg`` and ``pkg.__init__``. The old probe accepted the explicit spelling (``unirl.__init__.__getattr__`` resolved), and dropping it was an unintended behavior change. - Only index directory chains and module stems that are valid identifiers, so a file such as ``foo.bar.py`` cannot fabricate a dotted path the old probe could never have reached. - Drop a redundant ``str()`` around the ``ast.parse`` filename; it accepts any os.PathLike and decodes it to str anyway. - Correct two claims in the comments: this check prefers a module over a package of the same name, which is the opposite of Python's own import machinery, and the measured CephFS runtime is ~26s rather than the ~15s first estimated. Differential corpus grown from 933 to 1863 well-formed dotted paths, now covering ``__init__`` spellings at every depth: still zero mismatches against the old implementation. --- scripts/check_recipe_targets.py | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/scripts/check_recipe_targets.py b/scripts/check_recipe_targets.py index 50874ad4f..20408e835 100755 --- a/scripts/check_recipe_targets.py +++ b/scripts/check_recipe_targets.py @@ -10,8 +10,8 @@ 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 is the -difference between ~5min and ~15s per run. +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 @@ -51,16 +51,24 @@ def _scan() -> tuple[dict[str, Path], 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] == PACKAGE and all(p.isidentifier() for p in parts) skipped = bool(SKIP_PARTS & set(parts)) for name in filenames: - if parts[0] == PACKAGE and name.endswith(".py"): - if name == "__init__.py": - packages[".".join(parts)] = Path(dirpath, name) - else: - modules[".".join((*parts, name[:-3]))] = Path(dirpath, name) + 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(dirpath, name)) - # A module file shadows a package of the same name, as in Python's own lookup. + 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) @@ -72,7 +80,7 @@ def _read_all(paths: list[Path]) -> list[str]: 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=str(path)) + tree = ast.parse(source, filename=path) except SyntaxError: return None names: set[str] = set() From 4bc557c5f490484c8269a83a442a23a3ea906433 Mon Sep 17 00:00:00 2001 From: haonan3 Date: Fri, 31 Jul 2026 16:28:54 +0800 Subject: [PATCH 3/4] fix(lint): index every package _TARGET_RE accepts, not only unirl The index gate hardcoded PACKAGE = "unirl" from before main broadened _TARGET_RE to unirl|experimental (#210), so experimental.* targets were extracted but could never resolve. Derive the regex from PACKAGES so the accepted roots and the index can't drift apart again. --- scripts/check_recipe_targets.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/scripts/check_recipe_targets.py b/scripts/check_recipe_targets.py index 8ad17c0c7..0a31e82e6 100755 --- a/scripts/check_recipe_targets.py +++ b/scripts/check_recipe_targets.py @@ -4,8 +4,8 @@ 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). +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 @@ -34,10 +34,13 @@ 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 package ``_TARGET_RE`` accepts, so the only one worth indexing. -PACKAGE = "unirl" +# 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*['"]?((?:unirl|experimental)\.[A-Za-z0-9_.]+)['"]?\s*$""") +_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 @@ -52,7 +55,7 @@ def _scan() -> tuple[dict[str, Path], list[Path]]: 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] == PACKAGE and all(p.isidentifier() for p in parts) + 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) @@ -141,7 +144,7 @@ def main() -> int: for f in failures: print(f" {f}", file=sys.stderr) return 1 - print(f"check-recipe-targets: {len(targets)} unirl _target_ paths resolve.") + print(f"check-recipe-targets: {len(targets)} recipe _target_ paths resolve.") return 0 From 14b29205671de9777d3f61906fe2f0c73bd1a8b6 Mon Sep 17 00:00:00 2001 From: haonan3 Date: Fri, 31 Jul 2026 16:32:07 +0800 Subject: [PATCH 4/4] =?UTF-8?q?chore(lint):=20retire=20scripts/=20?= =?UTF-8?q?=E2=80=94=20guard=20scripts=20live=20in=20lint/?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scripts/ kept accumulating non-guard files because its name promised generic tooling space (#158's ep_verify/, #210's verify script). Name the folder after its real contract instead: lint/ holds exactly the scripts wired into .pre-commit-config.yaml, and CLAUDE.md now states the positive rule (verification harness results are quoted in the PR Test Plan, not committed). --- .pre-commit-config.yaml | 4 ++-- CLAUDE.md | 4 ++++ experimental/README.md | 4 ++-- {scripts => lint}/check_experimental_boundaries.py | 0 {scripts => lint}/check_recipe_targets.py | 4 +--- 5 files changed, 9 insertions(+), 7 deletions(-) rename {scripts => lint}/check_experimental_boundaries.py (100%) rename {scripts => lint}/check_recipe_targets.py (98%) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e8f0b66a3..d22a775d7 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -49,7 +49,7 @@ repos: # 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 files: \.(ya?ml|py)$ @@ -57,7 +57,7 @@ repos: # 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 c58f1791d..a3fa86c58 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 99e133821..86fae5f76 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/scripts/check_recipe_targets.py b/lint/check_recipe_targets.py similarity index 98% rename from scripts/check_recipe_targets.py rename to lint/check_recipe_targets.py index 0a31e82e6..ec1558656 100755 --- a/scripts/check_recipe_targets.py +++ b/lint/check_recipe_targets.py @@ -38,9 +38,7 @@ # 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) -) +_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