From 4e3768ea36f4d4a23578e07edfadb2406258b3e0 Mon Sep 17 00:00:00 2001 From: ethernet Date: Fri, 31 Jul 2026 13:26:27 -0400 Subject: [PATCH] perf(ci): sparse+blobless checkout for the test-slice generate job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Generate slices" job spent ~90% of its wall time in actions/checkout (4-27s across the last 10 runs, of a 10-32s job) pulling a ~212MB working tree. Everything in the test matrix waits on it. It doesn't need those files. `--generate-slices` returns before `_approximately_count_tests`, so it only ever uses test file *paths* plus the cached durations — it never opens a test file. The blocker was discovery: `_discover_files` rglobs the filesystem, which finds nothing under a sparse checkout. So add `--discover-from-git`, which lists paths via `git ls-files`. Sparse checkout only clears the worktree (entries are marked skip-worktree), so the index still carries every path and enumerates exactly the same set. Skip-part filtering and the root-override rule are duplicated to match `_discover_files` semantics. Measured on a real clone of this repo: full depth=1 clone: 7s 212M blobless+sparse clone: 3s 3.4M (still sees all 2510 test paths) Both discovery paths produce byte-identical slice JSON over the full 2472 test files, so slice assignment is unchanged. Tests assert the properties that make this safe: the two discovery paths agree on a full checkout, the git path still works when the files are absent from disk, and the skip-part override behaves the same either way. --- .github/workflows/tests.yml | 13 ++++- scripts/run_tests_parallel.py | 66 ++++++++++++++++++++- tests/test_run_tests_parallel.py | 99 ++++++++++++++++++++++++++++++++ 3 files changed, 176 insertions(+), 2 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 97e12357f7b..62c452b97ad 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -26,6 +26,17 @@ jobs: steps: - name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + # This job only needs test file *paths* — it computes the LPT + # distribution and never opens a test file. So skip the blobs + # (~212MB working tree -> ~3MB) and materialize only the script + # itself. `--discover-from-git` below lists the test paths from + # the git index, which a sparse checkout leaves fully populated. + # Checkout was ~90% of this job's wall time, and every test slice + # waits on it. + filter: blob:none + sparse-checkout: scripts/run_tests_parallel.py + sparse-checkout-cone-mode: false - name: Restore duration cache uses: actions/cache/restore@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 @@ -42,7 +53,7 @@ jobs: - name: Generate test slices id: matrix run: | - MATRIX=$(python3 scripts/run_tests_parallel.py --generate-slices ${{ inputs.slice_count }}) + MATRIX=$(python3 scripts/run_tests_parallel.py --generate-slices ${{ inputs.slice_count }} --discover-from-git) echo "matrix=$MATRIX" >> "$GITHUB_OUTPUT" test: diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 2042247d8bb..c0a172e59a8 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -170,6 +170,56 @@ def _discover_files(roots: List[Path]) -> List[Path]: return sorted(out) +def _discover_files_from_git(roots: List[Path], repo_root: Path) -> List[Path]: + """Like :func:`_discover_files`, but list paths from the git index. + + ``_discover_files`` walks the filesystem, which requires a full + working tree. The CI ``--generate-slices`` job only needs test file + *names* (it never opens them), so it checks out sparsely — a blobless, + sparse clone is ~3MB against ~212MB for the full tree. Under a sparse + checkout ``rglob`` finds nothing, because the files aren't on disk. + + The git *index* still carries every path (sparse checkout only clears + the worktree, marking entries skip-worktree), so ``git ls-files`` + enumerates the same set the filesystem walk would have. Skip-part + filtering and root-override semantics match ``_discover_files`` + exactly, so both discovery paths produce identical slicing input. + """ + seen: set[Path] = set() + out: List[Path] = [] + for root in roots: + rel = root.relative_to(repo_root) if root.is_absolute() else root + # Same opt-in rule as _discover_files: naming a skipped dir as a + # root overrides the skip for that subtree. + root_skip_overrides = {part for part in rel.parts if part in _SKIP_PARTS} + effective_skips = _SKIP_PARTS - root_skip_overrides + try: + listed = subprocess.run( + ["git", "ls-files", "-z", "--", str(rel)], + cwd=repo_root, + capture_output=True, + text=True, + check=True, + ).stdout + except (subprocess.CalledProcessError, FileNotFoundError) as e: + print(f"error: git ls-files failed for {rel}: {e}", file=sys.stderr) + raise SystemExit(2) from e + for entry in listed.split("\0"): + if not entry: + continue + path = Path(entry) + if not path.name.startswith("test_") or path.suffix != ".py": + continue + if any(part in effective_skips for part in path.parts): + continue + abs_path = repo_root / path + if abs_path in seen: + continue + seen.add(abs_path) + out.append(abs_path) + return sorted(out) + + def _kill_tree(proc: "subprocess.Popen", pgid: int | None = None) -> None: """Kill the pytest subprocess and every descendant it spawned. @@ -773,6 +823,15 @@ def main() -> int: "so the CI generate job can feed it directly into a matrix." ), ) + parser.add_argument( + "--discover-from-git", + action="store_true", + help=( + "List test files from the git index (git ls-files) instead of " + "walking the filesystem. Lets --generate-slices run against a " + "blobless sparse checkout, where the test files aren't on disk." + ), + ) parser.add_argument( "--files", metavar="LIST", @@ -811,6 +870,7 @@ def main() -> int: OUR_FLAGS = { "-j", "--jobs", "--paths", "--include-integration", "--file-timeout", "--file-retries", "--slice", "--generate-slices", "--files", + "--discover-from-git", } # pytest short flags that consume the NEXT token as their value. PYTEST_VALUE_FLAGS = {"-k", "-m", "-p", "-o", "-c", "-r", "-W"} @@ -929,7 +989,11 @@ def main() -> int: global _SKIP_PARTS # noqa: PLW0603 — config knob _SKIP_PARTS = set() - files = _discover_files(roots) + files = ( + _discover_files_from_git(roots, repo_root) + if args.discover_from_git + else _discover_files(roots) + ) if not files: print("No test files to run", file=sys.stderr) diff --git a/tests/test_run_tests_parallel.py b/tests/test_run_tests_parallel.py index 563d9f09573..42f2c98c684 100644 --- a/tests/test_run_tests_parallel.py +++ b/tests/test_run_tests_parallel.py @@ -378,3 +378,102 @@ def test_explicit_k_wins_over_node_id_inference(tmp_path: Path) -> None: # -k test_beta wins: one test ran, and it wasn't filtered to nothing. assert proc.returncode == 0, proc.stdout assert "1 tests passed" in proc.stdout + + +# ── Git-index discovery (--discover-from-git) ──────────────────────────────── +# +# The CI "Generate slices" job only needs test file *paths* — it computes the +# LPT distribution and never opens a test file. So it checks out blobless + +# sparse (~212MB working tree -> ~3MB), which makes the filesystem rglob in +# ``_discover_files`` find nothing. ``--discover-from-git`` lists paths from +# the git index instead, which a sparse checkout leaves fully populated. +# +# These are behavior contracts, not snapshots: the two discovery paths must +# agree on a full checkout (otherwise the flag silently changes which tests +# run in which slice), and the git path must still work when the files are +# absent from disk (otherwise it doesn't solve the problem it exists for). + + +def _git_repo_with_tests(tmp_path: Path) -> Path: + """A real git repo containing test files, some under skipped dirs.""" + repo = tmp_path / "repo" + (repo / "tests" / "sub").mkdir(parents=True) + (repo / "tests" / "docker").mkdir() + (repo / "tests" / "test_a.py").write_text("def test_a():\n assert True\n") + (repo / "tests" / "sub" / "test_b.py").write_text("def test_b():\n assert True\n") + # Not a test_*.py file — must never be discovered. + (repo / "tests" / "helper.py").write_text("X = 1\n") + # Under a _SKIP_PARTS dir — must be filtered out, same as the fs walk. + (repo / "tests" / "docker" / "test_c.py").write_text("def test_c():\n assert True\n") + subprocess.run(["git", "init", "-q"], cwd=repo, check=True) + subprocess.run(["git", "add", "-A"], cwd=repo, check=True) + subprocess.run( + ["git", "-c", "user.email=t@t", "-c", "user.name=t", + "commit", "-qm", "init"], + cwd=repo, check=True, + ) + return repo + + +def _discovery_fns(): + """Import both discovery functions straight from the runner script.""" + import importlib.util + + repo_root = Path(__file__).resolve().parent.parent + spec = importlib.util.spec_from_file_location( + "_rtp_under_test", repo_root / "scripts" / "run_tests_parallel.py" + ) + assert spec is not None and spec.loader is not None + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +def test_git_discovery_matches_filesystem_discovery(tmp_path: Path) -> None: + """On a full checkout both discovery paths return the same file set. + + This is the safety property for the CI flag: flipping discovery from + rglob to ``git ls-files`` must not change which files get sliced, or + slice assignment silently shifts under us. + """ + repo = _git_repo_with_tests(tmp_path) + mod = _discovery_fns() + roots = [repo / "tests"] + from_fs = mod._discover_files(roots) + from_git = mod._discover_files_from_git(roots, repo) + assert from_git == from_fs + # Sanity: the fixture actually produced something to compare. Sorted by + # full path, so tests/sub/test_b.py precedes tests/test_a.py. + assert [p.relative_to(repo).as_posix() for p in from_git] == [ + "tests/sub/test_b.py", + "tests/test_a.py", + ] + + +def test_git_discovery_finds_files_absent_from_disk(tmp_path: Path) -> None: + """The git path works when the worktree is sparse (files not checked out). + + Simulates the CI sparse checkout by deleting the test files while + leaving the index intact. rglob finds nothing; git ls-files still + enumerates every path. + """ + repo = _git_repo_with_tests(tmp_path) + mod = _discovery_fns() + roots = [repo / "tests"] + expected = mod._discover_files_from_git(roots, repo) + + for path in repo.rglob("test_*.py"): + path.unlink() + + assert mod._discover_files(roots) == [] + assert mod._discover_files_from_git(roots, repo) == expected + + +def test_git_discovery_honors_skip_part_override(tmp_path: Path) -> None: + """Naming a skipped dir as a root opts into it, same as the fs walk.""" + repo = _git_repo_with_tests(tmp_path) + mod = _discovery_fns() + roots = [repo / "tests" / "docker"] + from_git = mod._discover_files_from_git(roots, repo) + assert [p.name for p in from_git] == ["test_c.py"] + assert from_git == mod._discover_files(roots)