Add git submodule update to build_backend.py#3190
Add git submodule update to build_backend.py#3190aleozlx merged 2 commits intoflashinfer-ai:mainfrom
Conversation
📝 WalkthroughWalkthroughThe PR modifies the JIT cache compilation process in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request modifies flashinfer-jit-cache/build_backend.py to automatically initialize and update git submodules during the JIT cache compilation process. The review feedback highlights that running git commands unconditionally may cause build failures in environments where the source is not a git repository (such as source distributions) or where the git executable is missing. It is recommended to check for the existence of the .git directory and implement error handling for the subprocess call.
| # Ensure 3rdparty submodules are populated (may be empty in CI Docker images) | ||
| import subprocess | ||
|
|
||
| subprocess.run( | ||
| ["git", "submodule", "update", "--init", "--recursive"], | ||
| cwd=str(project_root), | ||
| check=True, | ||
| ) |
There was a problem hiding this comment.
Running git submodule update unconditionally will cause the build to fail when the source is not a git repository (e.g., when installing from a source distribution or a zip archive). It is important to check for the existence of the .git directory before executing git commands. Additionally, wrapping the call in a try-except block ensures that the build doesn't crash if the git executable is missing from the environment.
| # Ensure 3rdparty submodules are populated (may be empty in CI Docker images) | |
| import subprocess | |
| subprocess.run( | |
| ["git", "submodule", "update", "--init", "--recursive"], | |
| cwd=str(project_root), | |
| check=True, | |
| ) | |
| # Ensure 3rdparty submodules are populated (may be empty in CI Docker images) | |
| if (project_root / ".git").exists(): | |
| import subprocess | |
| try: | |
| subprocess.run( | |
| ["git", "submodule", "update", "--init", "--recursive"], | |
| cwd=str(project_root), | |
| check=True, | |
| ) | |
| except (subprocess.CalledProcessError, FileNotFoundError) as e: | |
| print(f"Warning: Failed to update git submodules: {e}") |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flashinfer-jit-cache/build_backend.py`:
- Around line 85-92: The unconditional subprocess.run(["git", ...]) call should
be guarded and should not rely on a PATH-only "git" executable; reuse the
repository existence check already used earlier (the same test that determines
if this source is a git checkout) to skip the submodule update for non-git/sdist
sources, and resolve the git binary with shutil.which before invoking
subprocess.run to avoid Ruff S607; specifically, replace the direct call to
subprocess.run with logic that (1) checks the existing is-git-repo condition or
project_root/.git presence and returns/skip if false, (2) locates the git
executable via shutil.which("git") (or raises/logs a clear error if not found),
and (3) calls subprocess.run using the absolute git path variable instead of the
literal "git" string for the subprocess invocation that updates submodules
(refer to the subprocess.run invocation and the project_root variable in
build_backend.py).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1503a1e2-691b-4bbe-a8fc-e82ec9b5fb56
📒 Files selected for processing (1)
flashinfer-jit-cache/build_backend.py
| # Ensure 3rdparty submodules are populated (may be empty in CI Docker images) | ||
| import subprocess | ||
|
|
||
| subprocess.run( | ||
| ["git", "submodule", "update", "--init", "--recursive"], | ||
| cwd=str(project_root), | ||
| check=True, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Verify current code uses PATH-only git invocation"
rg -n 'subprocess\.run\(\s*\["git",\s*"submodule"' flashinfer-jit-cache/build_backend.py -n -C2 || true
echo
echo "2) Demonstrate git submodule fails outside a git repo"
tmpdir="$(mktemp -d)"
if git -C "$tmpdir" submodule update --init --recursive >"$tmpdir/out.log" 2>&1; then
echo "Unexpected: command succeeded outside a git repo"
exit 1
else
echo "Expected failure:"
sed -n '1,3p' "$tmpdir/out.log"
fiRepository: flashinfer-ai/flashinfer
Length of output: 263
🏁 Script executed:
cat -n flashinfer-jit-cache/build_backend.py | sed -n '80,95p'Repository: flashinfer-ai/flashinfer
Length of output: 801
🏁 Script executed:
# Check where _compile_jit_cache is called
rg -n '_compile_jit_cache' flashinfer-jit-cache/ --type py -B2 -A2
# Check if there are any other references to git in the build
echo "---"
echo "Other git references in build files:"
rg -n 'git' flashinfer-jit-cache/build_backend.py --type pyRepository: flashinfer-ai/flashinfer
Length of output: 1547
🏁 Script executed:
# Check how get_git_version handles non-git scenarios
rg -n 'def get_git_version' --type py -A10
# Check if 3rdparty is included in sdist
ls -la flashinfer-jit-cache/3rdparty/ 2>/dev/null | head -20 || echo "3rdparty dir check failed"
# Check the actual directory structure
find flashinfer-jit-cache -maxdepth 2 -type d -name "3rdparty" -o -name ".gitmodules"Repository: flashinfer-ai/flashinfer
Length of output: 540
🏁 Script executed:
# Read full get_git_version function
cat build_utils.py | sed -n '24,50p'
# Check .gitmodules
if [ -f .gitmodules ]; then
echo ".gitmodules exists:"
cat .gitmodules
else
echo "No .gitmodules found"
fi
# Check 3rdparty structure
ls -la 3rdparty/ 2>/dev/null | head -20 || echo "3rdparty not found at root"Repository: flashinfer-ai/flashinfer
Length of output: 1254
Guard submodule update for non-git sources and avoid PATH-only executable lookup.
At lines 88–92, git submodule update ... is unconditional and uses a partial executable path ("git"). This will fail:
- In sdist builds (non-git checkouts)
- When
gitis not in PATH (flagged by Ruff S607)
The codebase already detects git repo existence at lines 57–59. Apply the same pattern here:
Suggested fix
def _compile_jit_cache(output_dir: Path, verbose: bool = True):
"""Compile AOT modules using flashinfer.aot functions directly."""
# Get the project root directory
project_root = Path(__file__).parent.parent
# Ensure 3rdparty submodules are populated (may be empty in CI Docker images)
+ import shutil
import subprocess
- subprocess.run(
- ["git", "submodule", "update", "--init", "--recursive"],
- cwd=str(project_root),
- check=True,
- )
+ git_dir = project_root / ".git"
+ if git_dir.exists():
+ git_bin = shutil.which("git")
+ if git_bin is None:
+ raise RuntimeError("git is required to initialize submodules in a git checkout")
+ subprocess.run(
+ [git_bin, "submodule", "update", "--init", "--recursive"],
+ cwd=str(project_root),
+ check=True,
+ )
+ else:
+ required = [
+ project_root / "3rdparty" / "cutlass",
+ project_root / "3rdparty" / "spdlog",
+ project_root / "3rdparty" / "cccl",
+ ]
+ missing = [str(p) for p in required if not p.exists()]
+ if missing:
+ raise RuntimeError(
+ "Missing required 3rdparty dependencies outside git checkout: "
+ + ", ".join(missing)
+ )🧰 Tools
🪛 Ruff (0.15.12)
[error] 89-89: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flashinfer-jit-cache/build_backend.py` around lines 85 - 92, The
unconditional subprocess.run(["git", ...]) call should be guarded and should not
rely on a PATH-only "git" executable; reuse the repository existence check
already used earlier (the same test that determines if this source is a git
checkout) to skip the submodule update for non-git/sdist sources, and resolve
the git binary with shutil.which before invoking subprocess.run to avoid Ruff
S607; specifically, replace the direct call to subprocess.run with logic that
(1) checks the existing is-git-repo condition or project_root/.git presence and
returns/skip if false, (2) locates the git executable via shutil.which("git")
(or raises/logs a clear error if not found), and (3) calls subprocess.run using
the absolute git path variable instead of the literal "git" string for the
subprocess invocation that updates submodules (refer to the subprocess.run
invocation and the project_root variable in build_backend.py).
|
/bot run |
|
bot run looks clean |
📌 Description
Follow up to #3158. This adds git submodule update --init --recursive in the jit-cache build_backend.py before AOT compilation begins, ensuring all submodules are populated.
🔍 Related Issues
#3159
🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Reviewer Notes
Summary by CodeRabbit