[None][build] Use cp -rL and skip up-to-date copies in copy_resolving_symlink#16020
Conversation
…_symlink copy_resolving_symlink was unconditionally deleting and re-copying large directory trees (e.g. CUTLASS headers) on every build. On network-mounted filesystems this can take several minutes even when nothing changed. Add an mtime check: if the destination already exists and is at least as new as the resolved source, skip the copy. The source mtime advances whenever CMake rebuilds the target, so this correctly invalidates the cache on real rebuilds while skipping no-op incremental runs. When a copy is needed, shell out to cp -rL instead of Python copytree. cp uses kernel-level copy primitives that are significantly faster on NFS. Falls back to copytree if cp is unavailable. Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
📝 WalkthroughWalkthroughThe ChangesIncremental Copy Optimization
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/build_wheel.py (1)
846-849: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePartial executable path for
cp(S607).Static analysis flags the partial path
"cp"in the subprocess call. Args are static (no untrusted input, noshell=True), so exploitability is low, but resolving to an absolute path hardens against$PATHtampering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build_wheel.py` around lines 846 - 849, The subprocess call in the build_wheel copy step uses a bare "cp" executable, which should be hardened against PATH tampering. Update the run invocation in the copy logic to resolve the cp binary via an absolute path (or an explicit, trusted executable location) while keeping the same arguments and check=True behavior, so the existing copy flow in the build_wheel script remains unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build_wheel.py`:
- Around line 845-851: The copy fallback in the wheel build logic is too broad
because `FileNotFoundError` is redundant with `Exception`, and the current
`except (FileNotFoundError, Exception)` around the `run(...)` call in
`build_wheel.py` will also hide real `CalledProcessError` failures from `cp`.
Update the `copytree` fallback in this block to catch only the specific
missing-command or missing-path case you actually want to recover from, and let
other subprocess failures propagate instead of being silently retried.
- Around line 836-840: The directory up-to-date check in the copy/sync logic can
incorrectly skip real updates because it only compares the top-level mtime for
resolved_src and dst_path. Update the logic in the directory-handling path of
the wheel build copy routine to use a recursive fingerprint of the directory
contents or a generated stamp file instead of relying on directory mtimes, so
nested changes under cutlass, cuda_ptx, or trtllm are detected before returning
early.
---
Nitpick comments:
In `@scripts/build_wheel.py`:
- Around line 846-849: The subprocess call in the build_wheel copy step uses a
bare "cp" executable, which should be hardened against PATH tampering. Update
the run invocation in the copy logic to resolve the cp binary via an absolute
path (or an explicit, trusted executable location) while keeping the same
arguments and check=True behavior, so the existing copy flow in the build_wheel
script remains unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 41381f2d-2335-4a01-903a-c345cc208fec
📒 Files selected for processing (1)
scripts/build_wheel.py
Replace mtime-on-dst check with a stamp file so partial/interrupted copies
are not incorrectly skipped on the next build. Narrow the except clause to
FileNotFoundError only so cp errors propagate instead of silently falling
back to copytree. Use shutil.which("cp") to resolve the binary path and
skip the subprocess path entirely when cp is not available.
Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot run |
|
PR_Github #57878 [ run ] triggered by Bot. Commit: |
|
PR_Github #57878 [ run ] completed with state |
…_symlink (NVIDIA#16020) Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
…_symlink (NVIDIA#16020) Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
3 participants
Summary
copy_resolving_symlinkwas unconditionally deleting and re-copying large directory trees (e.g. CUTLASS headers) on every build. On network-mounted filesystems this can take several minutes even when nothing changed.cp -rLinstead of Pythoncopytree.cpusescopy_file_range(2)on Linux, which triggers a server-side copy on NFSv4.2 servers, and uses larger I/O buffers than Python shutil. Falls back tocopytreeifcpis unavailable.Test plan
copytreewhencpis unavailableSummary by CodeRabbit