Improve CI#1174
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR consolidates CI binary builds under a single Bazel target ChangesBazel CI binary consolidation
Test port, concurrency, and metadata updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as run-unit-tests.yml
participant CLI as das-cli
participant MORK1 as MORK Server 40022
participant MORK2 as MORK Server 40032
participant Bazel as bazel_exec.sh
Workflow->>CLI: restart Attention Broker
Workflow->>MORK1: make run-mork-server
Workflow->>MORK2: make run-mork-server
Workflow->>MORK1: poll /status/-
Workflow->>MORK2: poll /status/-
Workflow->>Bazel: setup_bazel.sh
Workflow->>Bazel: test //tests/cpp/...
Workflow->>Bazel: build //:ci_binaries
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (2)
Makefile (1)
115-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare new targets
.PHONY.Static analysis flags
ci-unit-testsas not declaredPHONY; the same applies tobuild-ci-binariesandrun-tests-nativesince none produce a file matching their name.🔧 Proposed fix
+.PHONY: build-ci-binaries run-tests-native ci-unit-tests + build-ci-binaries: `@cd` src && ./scripts/bazel_exec.sh build --noshow_progress //:ci_binaries🤖 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 `@Makefile` around lines 115 - 122, The Makefile targets build-ci-binaries, run-tests-native, and ci-unit-tests are being treated as real files, so declare each of these as .PHONY to match their intent. Update the Makefile’s phony target declaration section to include these names alongside the existing targets, keeping the target names themselves unchanged.Source: Linters/SAST tools
src/BUILD (1)
218-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
filegroupinstead ofgenrulefor this aggregation target.The genrule with
cmd = "touch $@"works to force these binaries to build, but Bazel's own docs describe exactly this use case forfilegroup: gathering the outputs of a set of targets under a single label, without an extra local action or stamp-file artifact.♻️ Proposed refactor
-genrule( - name = "ci_binaries", - srcs = [ - ":word_query", - ":word_query_evolution", - ":attention_broker_service", - ":attention_broker_client", - ":das", - ":db_loader", - ":busnode", - ":busclient", - ":database_adapter", - ], - outs = ["ci_binaries.stamp"], - cmd = "touch $@", - visibility = ["//visibility:public"], -) +filegroup( + name = "ci_binaries", + srcs = [ + ":word_query", + ":word_query_evolution", + ":attention_broker_service", + ":attention_broker_client", + ":das", + ":db_loader", + ":busnode", + ":busclient", + ":database_adapter", + ], + visibility = ["//visibility:public"], +)🤖 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 `@src/BUILD` around lines 218 - 237, Replace the ci_binaries aggregation genrule in src/BUILD with a filegroup that directly collects the binary targets such as word_query, attention_broker_service, db_loader, and the rest. The current ci_binaries target uses a touch stamp only to force builds, but this should be expressed as a label-only aggregation target with filegroup so CI can depend on it without creating an extra action or artifact.
🤖 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/run-unit-tests.yml:
- Around line 60-80: The MORK startup check in the workflow is too permissive
and can miss failed launches. Update the readiness probe in the “Starting MORK
Servers” step to use curl with fail behavior so HTTP 4xx/5xx does not count as
ready, and replace the bare wait after the two make run-mork-server background
jobs with explicit background job handling so a failed startup surfaces
immediately. Use the existing loop and the run-mork-server commands as the main
anchors when adjusting the readiness and failure handling.
In `@src/.bazelrc`:
- Around line 12-13: The comment above the jobs setting is stale because it says
“half” while common --jobs is set to HOST_CPUS*0.85. Update the comment in the
.bazelrc entry to match the current value, keeping it aligned with the jobs
configuration so the intent is clear when locating the common --jobs setting.
In `@src/atomdb/morkdb/MorkDB.cc`:
- Around line 82-104: The retry loop in MorkDB::request-style HTTP handling can
block startup for too long when the MORK endpoint is unreachable, especially
through MorkClient usage from MorkDB::mork_setup() and
MorkConnection::connect(). Update the retry behavior in this path to either fail
fast on connection errors or make the retry budget/backoff configurable, and
ensure the logic around the transient status handling and max_attempts check in
MorkDB.cc is adjusted so a dead endpoint cannot stall synchronous startup for
the full window by default.
---
Nitpick comments:
In `@Makefile`:
- Around line 115-122: The Makefile targets build-ci-binaries, run-tests-native,
and ci-unit-tests are being treated as real files, so declare each of these as
.PHONY to match their intent. Update the Makefile’s phony target declaration
section to include these names alongside the existing targets, keeping the
target names themselves unchanged.
In `@src/BUILD`:
- Around line 218-237: Replace the ci_binaries aggregation genrule in src/BUILD
with a filegroup that directly collects the binary targets such as word_query,
attention_broker_service, db_loader, and the rest. The current ci_binaries
target uses a touch stamp only to force builds, but this should be expressed as
a label-only aggregation target with filegroup so CI can depend on it without
creating an extra action or artifact.
🪄 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: Pro Plus
Run ID: 20cb0ee2-5253-4269-b086-14c631944a2e
📒 Files selected for processing (12)
.github/scripts/bazel_build.sh.github/workflows/run-unit-tests.ymlMakefilesrc/.bazelrcsrc/BUILDsrc/atomdb/morkdb/MorkDB.ccsrc/scripts/mork_server.shsrc/tests/cpp/BUILDsrc/tests/cpp/adapterdb_test.ccsrc/tests/cpp/morkdb_test.ccsrc/tests/cpp/morkwrapper_test.ccsrc/tests/integration/cpp/BUILD
3 participants
Summary
Speeds up and stabilizes the self-hosted unit-test CI pipeline by running Bazel natively on the runner (instead of inside
das-builderDocker), failing fast on test regressions, and fixing MORK server orchestration for parallel test workloads.bazel_exec.shon the host aftersetup_bazel.sh; dropsbazel_build.sh+make run-tests-only(Docker round-trip) from the unit-test workflow.//:ci_binariesmeta-target — single Bazel target that forces all production binaries to compile/link; shared by CI andbazel_build.sh(publish workflow).//tests/cpp/...first, then//:ci_binaries(reuses Bazel cache from the test build).40022(morkdb) and40032(adapterdb / morkwrapper); fixedmork_server.shto run detached with stable per-port container names.//tests/cpp/...only; integration tests stay tagged and excluded (they require/opt/dasDocker layout).CI workflow changes (
run-unit-tests.yml)bazel_build.sh(full packaging script)make run-tests-only→ Dockerdas-builder./scripts/bazel_exec.sh test //tests/cpp/...on host//:ci_binariesOther changes
src/BUILD—ci_binariesgenrule listing all productioncc_binary/ shared-lib targets.Makefile—build-ci-binaries,run-tests-native,ci-unit-testsfor local parity with CI.src/.bazelrc—--jobs=HOST_CPUS*0.85(was0.5) on dedicated baremetal runners.mork_server.sh—docker run -d, container namedas-mork-server-${PORT}, remove stale container before start.MorkDB.cc— retry on connection failures alongside transient HTTP 401/409.adapterdb_test/morkwrapper_testuse MORK port 40032; reducemorkdb_testthread counts; bumpadapterdb,morkwrapper,postgreswrappertest size tomedium.inference_integration_test— taggedintegration.Why native Bazel?
setup_bazel.shalready installs Bazelisk (/opt/bazel/bazelisk) and native deps (hiredis, mongocxx, libpqxx, httplib) on self-hosted runners — the same toolchain baked intodas-builder. Running tests through Docker duplicated compilation and required/opt/dasbind mounts that integration tests depended on. Unit tests only need the host toolchain + service fixtures (Redis, Mongo, MORK, Postgres).