perf(ci): warm root bazel cache on main; rename matrix row to root#280
Conversation
The `umbrella-phase1` id dates from when the root Bazel module only covered Phase 1 native Go (nvcf-cli, go lib/worker). The root module now builds the whole root workspace, Go and Java (nv-boot-parent), while the other matrix rows are per-subtree nested modules. Rename the row to `root` so the check reads `bazel (root)` and reads correctly next to `bazel (nvca)` etc. Also rename the UMBRELLA_GLOBS variable to ROOT_GLOBS and de-stale the "Phase 1" comments. No behavior change: same path (`.`), same tests_skip, same trigger globs. NOTE FOR MERGE: the status-check name changes from `bazel (umbrella-phase1)` to `bazel (root)`. If `bazel (umbrella-phase1)` is a required check on main, update the branch-protection required-check name to `bazel (root)` in lockstep, or PRs will wait on a check that no longer reports. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Bazel workflow detect job now selects the root workspace, uses root-module globs for build inclusion, and gates remote cache uploads using shell environment variables for pushes to ChangesRoot workspace detection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/bazel.yml:
- Around line 243-245: Update the conditional in the cache-upload step to avoid
interpolating github.event_name, github.ref, and github.head_ref directly into
shell syntax. Expose these GitHub context values through environment variables
or evaluate the branch condition with a workflow-level expression, then compare
safely within the script while preserving the existing push-to-main and
ci/rename-root-matrix-row behavior.
🪄 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: 92f6b75d-3ae9-4af2-b9fa-734903fd2dfd
📒 Files selected for processing (1)
.github/workflows/bazel.yml
Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/bazel.yml:
- Around line 246-248: Update the conditional in the workflow’s cache-warm
upload block to stop interpolating github.event_name and github.ref directly
into shell source. Use environment variables for both GitHub context values and
compare those variables safely at runtime, while preserving the existing
push-to-main or HEAD_REF test-branch behavior.
🪄 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: 1e84b9bb-574e-4739-9fb8-2d2cc6d26657
📒 Files selected for processing (1)
.github/workflows/bazel.yml
…polation (coderabbit) Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed: the condition now uses the auto-provided |
The HEAD_REF upload override was a diagnostic to prove root cache warm-persistence; it is not part of the matrix rename. Root builds remain read-only on PRs and upload only on push to main. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The root matrix row builds the full ~7k-target closure but only ran on main when a ROOT_GLOB path changed (~1 in 10 merges), so the root closure was almost never uploaded to the remote cache. PRs and later merges then paid the full cold build (~626s, 5172 local actions, 0 hits) every time, while frequently-touched rows like nvca stayed warm (~86% hits). Force the root row to run on every push to main so the build step uploads the whole closure each merge. Per-service rows and PRs read those warm results for the shared Go/Java closure. PR behavior is unchanged: PRs stay read-only and root still runs on PRs only when a ROOT_GLOB changed. Also echo the resolved cache mode (read-write vs read-only, upload=...) in the Prepare-remote-cache step so a missing warm-write is visible in the log instead of an after-the-fact guess. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Part 2 of #283. On a pull_request, the root row builds the full ~7k-target closure even for a one-file change. Narrow it: diff the PR against its base, and if every change is a modification to an existing .go source, build only rdeps(//..., changed) instead of //.... Safety (cannot under-build, only over-build): - Only the root row on pull_request is a narrowing candidate. Main pushes (which warm+upload the whole closure), dispatch, schedule, and every per-service row build //... unchanged. - Any add/delete/rename, any global file (MODULE.bazel, .bazelrc, *.bzl, go.work, ...), any non-Go file, an unavailable base, or a failed diff falls back to //.... - The rdeps query runs without --keep_going, so an unresolvable changed-file label forces the full build rather than silently dropping a target. - The test step selects tests(set(affected)); a tests() query failure falls back to the full test set. Requires fetch-depth: 0 on the bazel-job checkout to diff against the base. Verified with a stubbed git/bazel harness across narrow, added-file, global, non-Go, main-push, non-root, query-fail, and empty cases. Co-authored-by: Balaji Ganesan <bganesan@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Why
A PR or merge that builds the root Bazel row compiles the full ~7,000-target
closure. Measured cold, that is ~626s / 5,172 local actions / 0 remote-cache
hits. It stayed cold because the root row only ran on
mainwhen aROOT_GLOBpath changed (roughly 1 in 10 merges), so the root closure was almost never
uploaded to the remote cache. Frequently-touched rows like
nvcawarmednaturally (~86% hits); root did not.
What changed
umbrella-phase1matrix row toroot(andUMBRELLA_GLOBStoROOT_GLOBS). The row builds the root module (Go and Java), sorootis theaccurate name;
umbrella-phase1was a rollout-era label.rootrow to run on every push tomain, regardless of whether aROOT_GLOBchanged. The build step already uploads results only on mainpushes, so this warms the whole closure each merge. Per-service rows and PRs
then read those warm results for the shared closure.
rootrow to only the targets a changeaffects: diff the PR against its base, and when every change is a
modification to an existing
.gosource, buildrdeps(//..., changed)instead of the full ~7k-target closure. Conservative by construction (any
add/rename/delete, global file, non-Go file, or query failure falls back to
//...; the rdeps query is strict so an unresolvable label forces full).Main pushes and per-service rows are unchanged.
read-write/read-only,upload=...)in the Prepare-remote-cache step, so a missing warm-write shows up in the log
instead of being an after-the-fact guess.
${{ github.* }}interpolations into$GITHUB_EVENT_NAME/$GITHUB_REFenv reads (CodeRabbit shell-injection findings).PR behavior is unchanged: PRs stay read-only and still run root only when a
ROOT_GLOBchanged.Customer Release Notes
Not customer visible (CI only).
Plan Summary
Not applicable.
Usage
Not applicable.
Testing
YAML parses; every
run:block passesbash -n. The warm/read-only mode is nowlogged, so the first merge to
mainafter this lands will showupload=trueonthe root row and the following PR should show a high remote-cache hit ratio on
the shared closure.
Notes
Both halves of #283 are in this PR: warm root on main (so PRs read a warm
closure) and narrow root PRs to affected targets (so cold PRs touching a
sliver of
src/libraries/do not rebuild everything). The narrow path usesbazel queryrdeps, not target-determinator, so no bazel-ci image change isneeded. This PR itself edits a non-Go file, so its own root build exercises
the full
//...path; the narrow path is unit-tested with a stubbedgit/bazel harness.
References
Relates to #283 (warm root on main + narrow root PRs to affected targets).
Related Merge Requests/Pull Requests
None
Dependencies
None