Repository navigation
build(bazel): verify Blacksmith remote cache hits and collect perf pairs (#5) - #426
Conversation
Same-output-base warm re-runs hide Blacksmith remote cache hits behind the local action cache. Prime and warm across fresh output bases, and collect ≥10 cold/warm pairs once hits are observed. Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughThe Bazel cache performance harness now isolates cold and warm output bases, measures representative targets, collects cold/warm pairs, validates remote-cache evidence, and writes updated evidence JSON. A new ChangesBazel cache performance collection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant collect_pairs
participant measure_representative
participant Bazel
participant Evidence_JSON
Operator->>collect_pairs: request paired collection
collect_pairs->>measure_representative: run isolated cold and warm measurements
measure_representative->>Bazel: execute representative targets
Bazel-->>measure_representative: timing and remote-hit evidence
measure_representative-->>collect_pairs: measurement result
collect_pairs->>Evidence_JSON: write pairs, metadata, gates, and status
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ci/bazel-cache-perf.py`:
- Around line 450-468: Update the pair-record construction to persist explicit
cold and warm compute evidence, including CPU time, action counts, and storage
metrics from each leg’s existing results. Replace the current
warm["wall_seconds"] assignment for compute_proxy_seconds with a value derived
from those compute metrics, while retaining wall-time fields for timing context.
- Around line 476-512: Reset evidence["status"] to "collecting" unconditionally
before calling evaluate_evidence() in the collection flow. Keep the existing
transition to "complete" only inside the passing gate branch, so failed
evaluations cannot preserve a prior completed status.
In `@scripts/ci/test-bazel-cache-perf.py`:
- Around line 78-91: Update test_observe_warm_uses_distinct_output_bases to mock
or capture measure_bazel, invoke mode_observe_warm, and assert that the two
recorded --output_base values are distinct. Remove the source-text checks while
preserving the existing Blacksmith Bazel Bootstrap execution and
bazel-warm-observation.json upload as integration evidence.
🪄 Autofix
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: Pro
Run ID: 699f662f-c598-42d6-8ccf-261e7285930f
⛔ Files ignored due to path filters (2)
.github/workflows/test.ymlis excluded by!**/.github/**docs/development/bazel-migration-perf.mdis excluded by!**/*.md,!**/docs/**
📒 Files selected for processing (2)
scripts/ci/bazel-cache-perf.pyscripts/ci/test-bazel-cache-perf.py
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Blocked on GitHub Actions major outage (no |
|
Blocked on GitHub Actions major outage (no |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Temporarily closing to retrigger Test Suite after GitHub Actions outage (webhook throttle). |
|
Reopening to retrigger CI for #5 Blacksmith cache evidence. |
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Blacksmith remote hits are confirmed; make affected-inputs use distinct output bases so isolation is measurable, allow the collected sample artifact, and satisfy ruff format/check. Co-authored-by: Cursor <cursoragent@cursor.com>
UpdateRemote cache hits are observed on Blacksmith ( That run still failed on:
Pushed a fix for those three so Bazel Bootstrap can proceed to |
Record 10 paired runs with remote cache hits from Bazel Bootstrap, mark perf-sample complete, and update harness tests for the close gate. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci/bazel-cache-perf.py (1)
476-484: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBind reused observations to the measured commit.
Line 476 retains prior observation booleans without checking their commit SHA. Line 484 then records the current SHA. A new commit can therefore reuse old
cache_unavailable_cold_correctandaffected_inputs_isolationproofs, whileevaluate_evidence()accepts those booleans as current evidence.Store provenance for each observation, or clear reused observations when the measured SHA changes.
Proposed fix
evidence = json.loads(evidence_path.read_text(encoding="utf-8")) + measured_sha = git_sha(root) pairs: list[dict[str, Any]] = [] remote_hits_seen = False ... obs = evidence.setdefault("observations", {}) + if evidence.get("git_sha_measured") != measured_sha: + for key in ( + "remote_cache_hits_on_identical_sha", + "cache_unavailable_cold_correct", + "affected_inputs_isolation", + ): + obs.pop(key, None) ... - "git_sha": git_sha(root), + "git_sha": measured_sha, ... - evidence["git_sha_measured"] = git_sha(root) + evidence["git_sha_measured"] = measured_sha🤖 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/ci/bazel-cache-perf.py` around lines 476 - 484, Update the evidence reuse flow around observations and git_sha(root) so cache_unavailable_cold_correct, affected_inputs_isolation, and related proof booleans cannot carry across measured commits. Track each observation’s originating SHA and only preserve it when it matches the current measured commit; otherwise clear the stale observation values before evaluate_evidence() consumes them.
🤖 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/ci/bazel-cache-perf.py`:
- Around line 604-613: Remove the final local-work/process-count fallback from
the `ok` computation. Rely on `local_fallback_ok` for runs without an execution
log, ensuring every accepted path still rejects runs where `core_rebuilt` is
true.
---
Outside diff comments:
In `@scripts/ci/bazel-cache-perf.py`:
- Around line 476-484: Update the evidence reuse flow around observations and
git_sha(root) so cache_unavailable_cold_correct, affected_inputs_isolation, and
related proof booleans cannot carry across measured commits. Track each
observation’s originating SHA and only preserve it when it matches the current
measured commit; otherwise clear the stale observation values before
evaluate_evidence() consumes them.
🪄 Autofix
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: Pro
Run ID: 8e4b79bd-e675-415f-aee5-e80432d9854b
⛔ Files ignored due to path filters (1)
.github/workflows/test.ymlis excluded by!**/.github/**
📒 Files selected for processing (3)
scripts/ci/bazel-cache-perf.pyscripts/ci/test-bazel-cache-perf.pyscripts/ci/test-ci-storage-policy.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci/test-bazel-cache-perf.py
Distinct output bases under remote cache masked source mutations as remote hits. Warm-then-mutate on one output_base again proves local rebuild of the changed crate only. Co-authored-by: Cursor <cursoragent@cursor.com>
Match the probe that already passed in Bootstrap: warm then mutate against the job default output base populated by earlier smoke builds. Co-authored-by: Cursor <cursoragent@cursor.com>
A fixed comment marker was uploaded by earlier probe runs, so warm→mutate got a remote cache hit instead of a local sandbox rebuild. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
--output_baseprime/warm).perf-sample.json; strict evaluate passes (warm p50 speedup ≈97%, compute reduction ≈98%, cold regression negative).--remote_cache.Test plan
observe-warm: remote cache hits observedcollect-pairs≥10 pairs → checked-inperf-sample.jsonstatuscompleteevaluatewithout--allow-pendingpassesCloses #5