[Feature] Added parallelism to ventis build for speedup - #22
Conversation
…docker buildx bake
📝 WalkthroughWalkthrough
ChangesDocker image build flow
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ventis_build
participant Docker
participant BuildxBakeFile
User->>ventis_build: run ventis build
ventis_build->>Docker: probe Docker and Buildx
alt Buildx available
ventis_build->>BuildxBakeFile: write bake targets
ventis_build->>Docker: run docker buildx bake
else Buildx unavailable
ventis_build->>Docker: run sequential docker build commands
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 (5)
.gitignore (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider committing
uv.lockinstead. For an installable application, a tracked lock file gives contributors and CI reproducible dependency resolution; ignoring it is usually reserved for libraries.🤖 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 @.gitignore at line 43, Update the .gitignore entry for uv.lock so the lock file is no longer ignored, allowing it to be committed and used for reproducible dependency resolution.ventis/cli.py (2)
321-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
docker_contextleaks across loop iterations.
docker_contextis assigned inside both branches, but everycontinuepath (missingworkflow_file, missing entrypoint, no matching YAML, etc.) skips the assignment while leaving the previous iteration's value bound. Today eachcontinuealso skips this append, so it is not currently reachable — but the append sits outside theif/else, so any future early-exit refactor silently registers the wrong context. Initializingdocker_context = Noneper iteration and appending inside each branch would remove the hazard.🤖 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 `@ventis/cli.py` around lines 321 - 327, Initialize docker_context to None at the start of each loop iteration and restructure the branch-specific flow so bake_targets.append executes only within the branch that successfully assigns a valid context. Preserve all existing continue paths for missing workflow files, entrypoints, or matching YAML.
248-250: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStep numbering skips 3. Steps go 1, 2, 4, 5 — worth renumbering while this block is being touched.
🤖 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 `@ventis/cli.py` around lines 248 - 250, Correct the step numbering in the surrounding CLI workflow comments so the sequence is consecutive, adding or restoring Step 3 before the existing Step 4 section near bake_targets and updating subsequent labels as needed.tests/test_cli.py (2)
240-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssertions are weaker than intended. Both tests assert
call[0] == "docker"is never seen, which passes trivially even if the build loop malfunctions — the only recorded call is the protoc invocation. Assertingdocker_callscontains exactly the protoc call would pin the no-op behavior.🤖 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 `@tests/test_cli.py` around lines 240 - 270, Strengthen the assertions in test_build_with_no_agents_builds_nothing and test_build_skips_agent_without_entrypoint to require docker_calls exactly matches the expected protoc invocation, rather than merely containing no Docker call. Preserve the existing no-op build behavior while ensuring unexpected or missing recorded calls fail these tests.
130-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne mock for both
_docker_availableprobes hides the docker-missing case.
cmd_buildcalls_docker_available()twice with different probe commands (docker info, thendocker buildx version). Patching with a flatreturn_valuemakes both return the same thing, so the only fallback scenario exercised is "Docker entirely absent" — the realistic "Docker present, buildx missing" case is never covered, and neither is the abort behavior I flagged onventis/cli.pylines 332-358.Consider a
side_effectkeyed onprobe_cmd.🧪 Suggested probe-aware fake
- def _run_build( - self, project_dir, agent_yaml_paths, buildx_available, platform="linux/amd64" - ): + def _run_build( + self, + project_dir, + agent_yaml_paths, + buildx_available, + docker_available=True, + platform="linux/amd64", + ):- patch("ventis.cli._docker_available", return_value=buildx_available), + patch( + "ventis.cli._docker_available", + side_effect=lambda probe_cmd=("docker", "info"): ( + buildx_available + if "buildx" in probe_cmd + else docker_available + ), + ),🤖 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 `@tests/test_cli.py` at line 130, Update the cmd_build test mock for ventis.cli._docker_available to use a probe-aware side_effect keyed by probe_cmd, returning Docker availability for “docker info” and an independently configured buildx result for “docker buildx version”. Add coverage for Docker present with buildx missing, including the expected abort behavior, while preserving coverage for Docker entirely absent.
🤖 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 `@ventis/cli.py`:
- Around line 332-358: The Docker availability flow in cmd_build must
distinguish Docker absence from buildx absence: update ventis/cli.py lines
332-358 to guard Docker availability separately with an accurate no-Docker
message, then nest the buildx probe so it selects bake when available or
sequential docker build otherwise. Update the test double at tests/test_cli.py
lines 130-130 to use a side_effect that inspects probe_cmd, and add coverage for
Docker present with buildx missing.
- Around line 88-101: The bake target construction around target["name"] must
normalize names into Buildx-safe slugs containing only letters, digits,
underscores, or hyphens, and enforce uniqueness after normalization. Update the
target-generation logic that derives names from agent_name.lower() to sanitize
spaces, dots, and other invalid characters, then validate normalized names are
unique before adding targets so collisions fail explicitly instead of
overwriting dictionary entries.
---
Nitpick comments:
In @.gitignore:
- Line 43: Update the .gitignore entry for uv.lock so the lock file is no longer
ignored, allowing it to be committed and used for reproducible dependency
resolution.
In `@tests/test_cli.py`:
- Around line 240-270: Strengthen the assertions in
test_build_with_no_agents_builds_nothing and
test_build_skips_agent_without_entrypoint to require docker_calls exactly
matches the expected protoc invocation, rather than merely containing no Docker
call. Preserve the existing no-op build behavior while ensuring unexpected or
missing recorded calls fail these tests.
- Line 130: Update the cmd_build test mock for ventis.cli._docker_available to
use a probe-aware side_effect keyed by probe_cmd, returning Docker availability
for “docker info” and an independently configured buildx result for “docker
buildx version”. Add coverage for Docker present with buildx missing, including
the expected abort behavior, while preserving coverage for Docker entirely
absent.
In `@ventis/cli.py`:
- Around line 321-327: Initialize docker_context to None at the start of each
loop iteration and restructure the branch-specific flow so bake_targets.append
executes only within the branch that successfully assigns a valid context.
Preserve all existing continue paths for missing workflow files, entrypoints, or
matching YAML.
- Around line 248-250: Correct the step numbering in the surrounding CLI
workflow comments so the sequence is consecutive, adding or restoring Step 3
before the existing Step 4 section near bake_targets and updating subsequent
labels as needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 525a1dc0-3f5f-4e6d-a407-2f96c59745de
📒 Files selected for processing (6)
.dockerignore.gitignoreREADME.mdtests/test_cli.pyventis/cli.pyventis/stub_generator.py
| bake_config = { | ||
| "target": { | ||
| target["name"]: { | ||
| "context": os.path.abspath(target["context"]), | ||
| "dockerfile": "Dockerfile", | ||
| "tags": [target["image_name"]], | ||
| "platforms": [platform], | ||
| "output": ["type=docker"], | ||
| # type=docker could be changed to tarring it up, which would be | ||
| # faster but skipped because that change would alter ventis deploy | ||
| } | ||
| for target in bake_targets | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
docker buildx bake target name allowed characters validation
💡 Result:
Docker Buildx Bake targets are strictly validated to ensure compatibility across various systems and naming conventions [1]. The allowed characters for a target name are defined by the regular expression [a-zA-Z0-9_-]+ [2][1]. This means a target name must consist only of: - Uppercase and lowercase letters (a-z, A-Z) - Digits (0-9) - Underscores (_) - Hyphens (-) If a target name includes characters outside of this set—such as dots (.)—docker buildx bake will return an error stating that only those specific characters are allowed [1][3]. In contexts like Docker Compose, where service names might include characters (like dots) that are valid in the Compose specification but invalid for Bake targets, the tooling performs a sanitization step [3][4]. This often involves replacing restricted characters with underscores (e.g., converting service.name to service_name) to ensure they can be processed as Bake targets [2][4]. When working with matrix strategies that dynamically generate names, you can use the sanitize() function within your HCL configuration to ensure generated names comply with these requirements [5].
Citations:
- 1: bake: restrict target name docker/buildx#929
- 2: https://github.com/docker/buildx/blob/master/bake/bake.go
- 3: [BUG]
-.json:10,5-19: Invalid name; only "[a-zA-Z0-9_-]+" are alloweddocker/compose#12919 - 4: sanitize service name so they can be used as bake targets docker/compose#12925
- 5: Matrix Target; Invalid name; only "[a-zA-Z0-9_-]+" are allowed docker/buildx#3082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching cli.py:\n'
fd -a 'cli\.py$' . || true
printf '\nVentis cli.py outline/snippet:\n'
if [ -f ventis/cli.py ]; then
wc -l ventis/cli.py
sed -n '300,360p' ventis/cli.py
sed -n '70,110p' ventis/cli.py
else
echo "ventis/cli.py not found"
fi
printf '\nSearch agent_name and bake target usages:\n'
rg -n 'agent_name|target\["name"\]|target_names|bake_target|image_name' ventis -SRepository: CanyonCodeCoreAI/canyoncodecore
Length of output: 12162
Sanitize bake target names and enforce uniqueness.
target["name"] is built directly from agent_name.lower(), while Buildx target names are limited to [A-Za-z0-9_-]; names with spaces or dots will make the generated bake file fail. Case-only differences also become duplicate dict keys, silently dropping one target from the build command path. Normalize to a safe slug and assert that normalized target names are unique before appending targets.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 101-101: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(bake_file_path, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@ventis/cli.py` around lines 88 - 101, The bake target construction around
target["name"] must normalize names into Buildx-safe slugs containing only
letters, digits, underscores, or hyphens, and enforce uniqueness after
normalization. Update the target-generation logic that derives names from
agent_name.lower() to sanitize spaces, dots, and other invalid characters, then
validate normalized names are unique before adding targets so collisions fail
explicitly instead of overwriting dictionary entries.
| if not bake_targets: | ||
| logger.info("No Docker images to build.") | ||
| elif _docker_available() and _docker_available(("docker", "buildx", "version")): | ||
| docker_container_dir = os.path.join(project_dir, "docker_container") | ||
| os.makedirs(docker_container_dir, exist_ok=True) | ||
| bake_file_path = os.path.join(docker_container_dir, "docker-bake.json") | ||
| _write_bake_file(bake_targets, bake_file_path, _docker_platform()) | ||
|
|
||
| target_names = [target["name"] for target in bake_targets] | ||
| logger.info( | ||
| "Building %d Docker image(s) via `docker buildx bake`.", | ||
| len(bake_targets), | ||
| ) | ||
| subprocess.run( | ||
| ["docker", "buildx", "bake", "--file", bake_file_path, *target_names], | ||
| check=True, | ||
| ) | ||
| else: | ||
| logger.info( | ||
| "docker buildx unavailable; falling back to sequential `docker build`." | ||
| ) | ||
| for target in bake_targets: | ||
| logger.info("Building Docker image: %s", target["image_name"]) | ||
| subprocess.run( | ||
| _docker_build_cmd("-t", image_name, docker_context), check=True | ||
| _docker_build_cmd("-t", target["image_name"], target["context"]), | ||
| check=True, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The two _docker_available probes are collapsed into one condition in both the build logic and the test double. cmd_build distinguishes "Docker present" from "buildx present" only inside a single and, so the else branch cannot tell them apart — and the test mock returns one flat value for both probes, so the divergence is never exercised.
ventis/cli.py#L332-L358: split theelifinto a separate_docker_available()guard that skips builds with an accurate message, and a nested buildx check that chooses bake vs sequentialdocker build.tests/test_cli.py#L130-L130: replace the flatreturn_valuewith aside_effectthat inspectsprobe_cmd, and add a case covering Docker present with buildx missing.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 354-357: Use of unsanitized data to create processes
Context: subprocess.run(
_docker_build_cmd("-t", target["image_name"], target["context"]),
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 344-347: Command coming from incoming request
Context: subprocess.run(
["docker", "buildx", "bake", "--file", bake_file_path, *target_names],
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 354-357: Command coming from incoming request
Context: subprocess.run(
_docker_build_cmd("-t", target["image_name"], target["context"]),
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
📍 Affects 2 files
ventis/cli.py#L332-L358(this comment)tests/test_cli.py#L130-L130
🤖 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 `@ventis/cli.py` around lines 332 - 358, The Docker availability flow in
cmd_build must distinguish Docker absence from buildx absence: update
ventis/cli.py lines 332-358 to guard Docker availability separately with an
accurate no-Docker message, then nest the buildx probe so it selects bake when
available or sequential docker build otherwise. Update the test double at
tests/test_cli.py lines 130-130 to use a side_effect that inspects probe_cmd,
and add coverage for Docker present with buildx missing.
iidsample
left a comment
iidsample
left a comment
There was a problem hiding this comment.
One comment about bake config, let me know what you think
| for target in bake_targets | ||
| } | ||
| } | ||
| with open(bake_file_path, "w") as f: |
There was a problem hiding this comment.
What is the point of this bake config and what does it do ? How is it configured.
2 participants
Ventis build now builds all agent and workflow Docker images in a single parallel docker buildx bake pass instead of sequential docker build calls, cutting build time significantly and letting BuildKit share cached layers (e.g. pip install) across agents with identical dependencies.
If Buildx isn't available on the host, it falls back to today's sequential build behavior. Every generated bake target sets output: type=docker so images always land in the local image store, matching what ventis deploy expects.
Also fixed: agent/workflow Dockerfiles now use JSON-array (exec) form for CMD instead of shell form, so containers respond correctly to OS signals.
Summary by CodeRabbit
New Features
Improvements