Improve build caching - #879
Conversation
📝 WalkthroughWalkthroughAdds Buildx registry-backed caching to GitHub Actions steps and a new Dockerfile builder stage that isolates dependency-definition files into a separate layer to improve rebuild caching and avoid rebuilding dependencies when source changes. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Buildx as Docker Buildx
participant Registry as Container Registry
participant Docker as Dockerfile build stages
GH->>Buildx: Trigger build (Test or Build)
Buildx->>Registry: pull cache-from: buildcache
Buildx->>Docker: execute Dockerfile
Docker->>Docker: use agent-deps stage (copy dependency files)
Docker->>Docker: main build copies from agent-deps, installs deps
Buildx->>Registry: push cache-to: buildcache (mode=max)
Buildx->>Registry: push image (non-pull_request)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
build/docker/agent-ros2/Dockerfile (1)
346-355: Clarify rsync filter logic for maintainability.The rsync command uses both
--include="*/"(to preserve directory structure) and--exclude="*"(to filter out unwanted files), which is correct but the intent could be clearer with a brief comment explaining that only the listed dependency file types are copied while maintaining the source directory hierarchy.RUN --mount=type=bind,target=/workspace/code,source=code \ mkdir /arlab-deps && \ # Copy only dependency definition files to preserve directory structure for caching rsync -r --prune-empty-dirs --include="*/" \ --include="requirements.txt" \ --include="requirements.*.txt" \ --include="package.xml" \ --include="setup.cfg" \ --include="setup.py" \ --include="CMakeLists.txt" \ --exclude="*" "/workspace/code" "/agent-deps/".github/workflows/build.yml (1)
54-55: LGTM, but note registry cache accumulation strategy.The cache configuration correctly uses registry-backed caching with
mode=max. The Test build will load locally (load: true, line 47) and also push cache layers to the registry for reuse in subsequent builds. This integrates well with the Dockerfile's newagent-depscaching strategy.One consideration: There is no explicit cleanup or retention policy for the buildcache tag. Over time, old cache layers will accumulate in the registry. Monitor registry storage and consider adding a cache cleanup job if needed.
Consider documenting or implementing a periodic cache cleanup strategy (e.g., monthly pruning of old cache entries) to manage registry storage growth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(2 hunks)build/docker/agent-ros2/Dockerfile(3 hunks)
🔇 Additional comments (4)
build/docker/agent-ros2/Dockerfile (3)
415-416: Verify the src removal doesn't corrupt the development workflow.Line 416 removes the src directory after pip install, and Line 422 immediately recreates it as a symlink to the volume mount (
/workspace/code). This pattern allows live code changes during development. Confirm this doesn't cause issues with:
- Partially installed packages in build caches
- Container startup after the symlink is created
- Dependency resolution across rebuilds
The removal and recreation pattern is intentional for the dev container (allowing live edits), but verify it doesn't interfere with cached dependency layers. Consider if any stale state from line 411-413's pip install could persist in earlier cache layers.
393-394: No action needed - the COPY source path is correct.The rsync command in the
agent-depsstage (line 346-355) copies/workspace/codeto/agent-deps/, resulting in/agent-deps/code/.... The COPY command at line 393 correctly references/agent-deps/codeas the source, properly matching the rsync output structure.Likely an incorrect or invalid review comment.
332-356: Bind mount and rsync configuration verified.The
codedirectory exists at the repository root and contains all expected dependency files (requirements.txt, package.xml, setup.py/cfg, CMakeLists.txt across 13 packages). The rsync command without a trailing slash on the source will correctly generate the/agent-deps/code/<path-to-files>structure as intended for caching dependency definitions..github/workflows/build.yml (1)
73-74: LGTM, but be aware of concurrent build race conditions.The Build+Push step uses the same buildcache tag as the Test step, allowing both PR and main branch builds to share cached layers. However, if multiple builds run simultaneously (e.g., two PRs or a PR + main push), they could race when writing to the buildcache tag, potentially causing inconsistent cache state.
This is generally acceptable in most CI environments, but worth monitoring if concurrent builds are frequent.
Verify that your GitHub Actions runner (self-hosted, build) is configured to serialize builds or handle concurrent cache writes gracefully. You may want to check if Docker registry locking mechanisms protect against concurrent tag overwrites.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @build/docker/agent-ros2/Dockerfile:
- Around line 342-356: The rsync invocation that builds /agent-deps currently
includes patterns "--include='requirements.txt'" and
"--include='requirements.*.txt'", which misses files with underscores like
requirements_infrastructure.txt; update the rsync include filters in that RUN
block (the rsync command that copies from "/workspace/code" to "/agent-deps/")
to either add "--include='requirements_infrastructure.txt'" or more generically
replace the two patterns with "--include='requirements*.txt'" so that both
dotted and underscored variants are copied.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
build/docker/agent-ros2/Dockerfile
🔇 Additional comments (3)
build/docker/agent-ros2/Dockerfile (3)
332-341: LGTM! Clear documentation of the caching strategy.The comment effectively explains the rationale for this optimization stage. Using a minimal ubuntu:24.04 base is appropriate since this stage only performs file operations without requiring the full agent runtime environment.
393-393: LGTM! Dependency-based caching correctly implemented.This change replaces the previous approach with copying from the
agent-depsstage, which will only invalidate the cache when dependency definition files change (not when source code changes). This is the core optimization for the build cache improvement.
415-416: LGTM! Proper cleanup of temporary dependency files.Removing the
srcdirectory after dependency installation is correct because:
- The dependency files were only needed for rosdep and pip installation (lines 397-413)
- The actual source code will be mounted at runtime via the
/workspacevolume (line 420)- The symlink at line 422 will point to the runtime-mounted code
- This prevents stale or partial files from persisting in the image layer
| RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \ | ||
| --mount=type=cache,target=/var/lib/apt,sharing=locked \ | ||
| apt-get update && apt-get install -y rsync | ||
|
|
||
| RUN --mount=type=bind,target=/workspace/code,source=code \ | ||
| mkdir /agent-deps && \ | ||
| rsync -r --prune-empty-dirs --include="*/" \ | ||
| --include="requirements.txt" \ | ||
| --include="requirements.*.txt" \ | ||
| --include="package.xml" \ | ||
| --include="setup.cfg" \ | ||
| --include="setup.py" \ | ||
| --include="CMakeLists.txt" \ | ||
| --exclude="*" "/workspace/code" "/agent-deps/" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure
git ls-files | head -30Repository: una-auxme/paf
Length of output: 768
🏁 Script executed:
# Check the Dockerfile context - find all files in or related to the agent-ros2 build
fd -t f 'Dockerfile|requirements|setup|pyproject|MANIFEST|CMakeLists' --max-results=50Repository: una-auxme/paf
Length of output: 1808
🏁 Script executed:
# Get more context on the Dockerfile around line 393 mentioned in the comment
sed -n '390,400p' build/docker/agent-ros2/DockerfileRepository: una-auxme/paf
Length of output: 510
🏁 Script executed:
# Search for all dependency files that exist in the repo
echo "=== Python dependency files ===" && \
fd -t f -n 'pyproject\.toml|MANIFEST\.in' && \
echo -e "\n=== Requirements files ===" && \
fd -t f -n 'requirements.*\.txt|requirements\.txt' && \
echo -e "\n=== Setup files ===" && \
fd -t f -n 'setup\.py|setup\.cfg' && \
echo -e "\n=== Package manifests ===" && \
fd -t f -n 'package\.xml' && \
echo -e "\n=== CMake files ===" && \
fd -t f -n 'CMakeLists\.txt'Repository: una-auxme/paf
Length of output: 336
🏁 Script executed:
# Fix the command - search for dependency files properly
echo "=== Python dependency files ===" && \
fd 'pyproject\.toml' && \
fd 'MANIFEST\.in' && \
echo -e "\n=== Requirements files ===" && \
fd 'requirements.*\.txt' && \
echo -e "\n=== Setup files ===" && \
fd 'setup\.py|setup\.cfg' && \
echo -e "\n=== Package manifests ===" && \
fd 'package\.xml' && \
echo -e "\n=== CMake files ===" && \
fd 'CMakeLists\.txt'Repository: una-auxme/paf
Length of output: 2121
🏁 Script executed:
# Specifically check in code/ directory for dependency files
echo "=== All dependency-related files in code/ directory ===" && \
fd -t f '' code/ | grep -E '(requirements|setup|pyproject|MANIFEST|CMakeLists|package\.xml)' | head -50Repository: una-auxme/paf
Length of output: 1180
🏁 Script executed:
# Check what files the rsync is supposed to copy - look at the code/ directory structure
echo "=== Directory structure of code/ ===" && \
fd -t f -d 2 '' code/ | head -40Repository: una-auxme/paf
Length of output: 1232
🏁 Script executed:
# Verify which requirements files match the rsync pattern "requirements.*.txt"
echo "=== Testing rsync glob pattern 'requirements.*.txt' ===" && \
echo "Files that should match 'requirements.*.txt' pattern:" && \
cd code && ls -1 requirements*.txt && cd - && \
echo -e "\n=== Testing with bash glob matching ===" && \
python3 << 'EOF'
import fnmatch
import os
pattern = "requirements.*.txt"
test_files = [
"requirements.txt",
"requirements.cpu.txt",
"requirements.cuda.txt",
"requirements.rocm.txt",
"requirements_infrastructure.txt"
]
print(f"Pattern: {pattern}\n")
for f in test_files:
matches = fnmatch.fnmatch(f, pattern)
print(f" {f:<35} -> {'✓ MATCHES' if matches else '✗ DOES NOT MATCH'}")
EOFRepository: una-auxme/paf
Length of output: 143
🏁 Script executed:
# Check if requirements_infrastructure.txt is actually needed/used
echo "=== Checking usage of requirements_infrastructure.txt ===" && \
rg -l 'requirements_infrastructure' --type txt --type py --type shRepository: una-auxme/paf
Length of output: 177
🏁 Script executed:
# Look at the full Dockerfile to understand the context better
sed -n '342,400p' build/docker/agent-ros2/DockerfileRepository: una-auxme/paf
Length of output: 2693
🏁 Script executed:
# Check if requirements_infrastructure.txt is required for the build
cat build/docker/agent-ros2/scripts/install-python-requirements.shRepository: una-auxme/paf
Length of output: 2368
🏁 Script executed:
# Verify the exact rsync filter matching behavior
python3 << 'EOF'
import fnmatch
# Test files from the codebase
files = [
"requirements.txt",
"requirements.cpu.txt",
"requirements.cuda.txt",
"requirements.rocm.txt",
"requirements_infrastructure.txt"
]
# Rsync patterns from the Dockerfile
patterns = [
"requirements.txt",
"requirements.*.txt"
]
print("Rsync filter matching analysis:\n")
for f in files:
matches = any(fnmatch.fnmatch(f, p) for p in patterns)
status = "✓ COPIED" if matches else "✗ NOT COPIED"
print(f" {f:<35} -> {status}")
if not matches:
print(f" (not matched by any pattern: {patterns})")
EOFRepository: una-auxme/paf
Length of output: 420
Add requirements_infrastructure.txt to rsync filters.
The rsync glob pattern requirements.*.txt only matches files with dots (e.g., requirements.cpu.txt), not underscores. However, requirements_infrastructure.txt is actively used in the build process and will not be copied to /agent-deps/code/, causing infrastructure dependencies to be missing during the build.
Update the rsync filters to explicitly include the infrastructure requirements file:
Fix rsync filters
RUN --mount=type=bind,target=/workspace/code,source=code \
mkdir /agent-deps && \
rsync -r --prune-empty-dirs --include="*/" \
--include="requirements.txt" \
--include="requirements.*.txt" \
+ --include="requirements_infrastructure.txt" \
--include="package.xml" \
--include="setup.cfg" \
--include="setup.py" \
--include="CMakeLists.txt" \
--exclude="*" "/workspace/code" "/agent-deps/"Alternatively, use requirements*.txt instead of both requirements.txt and requirements.*.txt to capture all variants (dot and underscore).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN --mount=type=cache,target=/var/cache/apt,sharing=locked \ | |
| --mount=type=cache,target=/var/lib/apt,sharing=locked \ | |
| apt-get update && apt-get install -y rsync | |
| RUN --mount=type=bind,target=/workspace/code,source=code \ | |
| mkdir /agent-deps && \ | |
| rsync -r --prune-empty-dirs --include="*/" \ | |
| --include="requirements.txt" \ | |
| --include="requirements.*.txt" \ | |
| --include="package.xml" \ | |
| --include="setup.cfg" \ | |
| --include="setup.py" \ | |
| --include="CMakeLists.txt" \ | |
| --exclude="*" "/workspace/code" "/agent-deps/" | |
| RUN --mount=type=bind,target=/workspace/code,source=code \ | |
| mkdir /agent-deps && \ | |
| rsync -r --prune-empty-dirs --include="*/" \ | |
| --include="requirements.txt" \ | |
| --include="requirements.*.txt" \ | |
| --include="requirements_infrastructure.txt" \ | |
| --include="package.xml" \ | |
| --include="setup.cfg" \ | |
| --include="setup.py" \ | |
| --include="CMakeLists.txt" \ | |
| --exclude="*" "/workspace/code" "/agent-deps/" |
🤖 Prompt for AI Agents
In @build/docker/agent-ros2/Dockerfile around lines 342 - 356, The rsync
invocation that builds /agent-deps currently includes patterns
"--include='requirements.txt'" and "--include='requirements.*.txt'", which
misses files with underscores like requirements_infrastructure.txt; update the
rsync include filters in that RUN block (the rsync command that copies from
"/workspace/code" to "/agent-deps/") to either add
"--include='requirements_infrastructure.txt'" or more generically replace the
two patterns with "--include='requirements*.txt'" so that both dotted and
underscored variants are copied.
2 participants
Description
Improves build caching both in the Dockerfile and in the github action.
Type of change
Does this PR introduce a breaking change?
No
Most important changes
Checklist:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.