perf(lint): make the recipe _target_ guard filesystem-cheap - #283
Conversation
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.
- 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.
github-actions
Bot
added
need review
…ook-index # Conflicts: # .pre-commit-config.yaml
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/ 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).
|
@CjhHa1 heads-up — I pushed three maintainer commits onto this branch (AI-assisted, reviewed by me). Explanation below; happy to split any of it back out if you'd rather keep this PR minimal. 1. 2. 3. Verification on the merged tree (local disk checkout):
|
2 participants
Summary
scripts/check_recipe_targets.pyresolves each_target_by probing candidate module paths on disk. With 2286 targets that is roughly 10kstatcalls, and nearly all of them are negative lookups (asking for files that do not exist).On a local checkout this is invisible. On a checkout that lives on a network filesystem it dominates everything else: on CephFS the hook takes 5m36s, of which only 0.7s is CPU — the rest is metadata round trips. Because the hook carries
always_run: true, every commit and every push pays it, even for changes that cannot possibly affect a recipe.What changed
os.walkbuilds adotted path -> fileindex up front, so the 2286 lookups become dict hits instead of filesystem probes.lru_cachegave the same effect; preserved).\.(ya?ml|py)$instead ofalways_run. A_target_can only be stranded by a recipe edit or a Python rename/removal, so nothing else needs to trigger the scan. The scan itself is still full-tree (pass_filenames: false), andpre-commit run --all-filesin CI is unaffected.Measurements
Back to back on the same CephFS checkout, and on a local-disk checkout of the same commit:
Correctness
The rewrite preserves the original resolution rules: longest module prefix wins, a
.pymodule shadows a package of the same name (the opposite of Python's own import machinery, but what this check has always done), only the first attribute after the module is checked, the explicitpkg.__init__.Symbolspelling still resolves, and vendored trees stay resolvable (SKIP_PARTSapplies to recipe discovery only, exactly as before).Old and new verdicts were compared over 1863 well-formed dotted paths — every real target plus mutations covering renamed symbols, renamed intermediate modules, case changes, package-only paths,
__init__spellings at every depth, paths into vendored trees, deeper attribute chains and pure nonsense. Zero mismatches: 547 resolve, 1316 do not.One deliberate difference
Paths with an empty component, e.g.
unirl.algorithms..cppo.CPPO, were silently accepted before, becausePath.joinpath()discards empty parts. Python cannot import such a path (ModuleNotFoundError), so this was a false negative — the guard would have waved through a recipe that fails at launch. They are now reported. No such path exists in the tree today; the change only removes a blind spot.Known gaps in the new trigger condition
Both are consequences of gating on
files:instead ofalways_run, and both are still caught by CI'spre-commit run --all-files:.pyfiles skips the hook at the pre-commit stage, since pre-commit'sget_staged_files()filters with--diff-filter=ACMRTUXB. The pre-push stage has no such filter, so it still runs there before anything leaves the machine.excluderemoves those paths before this hook'sfilesfilter is evaluated. This is latent: no recipe target currently points into a vendored tree.If either is judged too loose, dropping just the
.pre-commit-config.yamlhunk restoresalways_runand leaves the 13x speedup intact.Behavior note
A module file that cannot be read now raises instead of being reported as an unresolved target. Both are fail-closed, and a traceback naming the file and errno is more actionable than
unresolved _target_ 'x.y.Z'when the real problem is a permission or I/O error.Test Plan
python3 scripts/check_recipe_targets.pyreports the same2286 unirl _target_ paths resolve._target_produces byte-identical stderr and exit code 1 under both implementationspre-commit run check-recipe-targets --all-filespasses--files README.md) and runs for a.pychangeMaintainer addendum (2026-07-31, haonan3)
Three commits pushed on top (AI-assisted; I reviewed the diff):
071af5df— merge of current main (post-feat(experimental): tier contract, boundary lint, private-package convention #279/chore(scripts): drop ad-hoc verify harnesses, move dataset converters to datasets/ #284)..pre-commit-config.yamlconflict resolved keeping both this PR'sfiles:gating and feat(experimental): tier contract, boundary lint, private-package convention #279's boundaries hook.4bc557c5— fixes the merge-preview CI failure: the index gated onPACKAGE = "unirl"while main's_TARGET_RE(since feat(experimental): add WAN ReFL as a self-contained training package #210) also acceptsexperimental., soexperimental.*targets were extracted but unresolvable.PACKAGESnow derives the regex alternation and gates the walk — accepted roots and index can no longer drift apart.14b29205— folds in the planned guard-folder rename: both pre-commit guard scripts movescripts/→lint/(hookentry:lines +experimental/README.mdrefs updated), CLAUDE.md gains the guard-folder contract,scripts/is retired.Verification:
python lint/check_recipe_targets.py→ 2310 resolve (same count as main's prober, now incl. the 6experimental.*targets); old-vs-new equivalence over 139 unique real targets + mutations = 695 verdicts, 0 mismatches; dead-target injection → both exit 1, byte-identical stderr; fullSKIP=no-commit-to-branch pre-commit run --all-filesgreen (16 hooks, both guards firing fromlint/).