[CI] Bazel: Phase 3 — manifest job + bin_pack_shards.py (demo, stage 3 of 3) - #4
Draft
Kangyan-Zhou wants to merge 4 commits into
Draft
[CI] Bazel: Phase 3 — manifest job + bin_pack_shards.py (demo, stage 3 of 3)#4Kangyan-Zhou wants to merge 4 commits into
Kangyan-Zhou wants to merge 4 commits into
Conversation
Adds the compute-test-manifest pipeline that replaces (eventually) both
sglang's heuristic check-changes job and run_suite.py's auto_partition
bin-packing. This commit lands the building blocks; wiring downstream
shard jobs to consume the manifest is Phase-3.5 work.
Components:
- tools/bin_pack_shards.py: reads `bazel cquery --output=jsonproto`,
groups targets by sgl-suite-* tag, bin-packs each suite into N shards
via greedy longest-processing-time-first (LPT). Same algorithm as
python/sglang/test/ci/ci_register.py:auto_partition. Outputs JSON
keyed by suite name, value is a list of {id, targets, est_time}
dicts directly consumable as a GHA matrix.
- .github/workflows/pr-test.yml: new compute-test-manifest top-level
job. Runs on ubuntu-latest, installs bazelisk, runs cquery, bin-packs
via the script, uploads manifest.json as an artifact for inspection.
Currently no downstream consumer — that's the Phase-3.5 hookup that
has to wait until parity with auto_partition is verified.
Validated on H100:
- bazel cquery 'kind(py_test, //test/...)' --output=jsonproto runs
cleanly, producing target metadata for the 9 tagged tests.
- bin_pack_shards.py spreads them into 8 shards LPT-balanced: shard 0 =
345s (test_gpt_oss_sm120 alone), shard 1 = 134s (test_srt_endpoint
alone), etc.
- INFO messages for unconfigured suites surface them without erroring.
- Hard error when --shards configures a suite with zero matching
targets (typo / stale config detection).
Behavioral notes:
- Targets without both sgl-suite-* and est_time:* tags are silently
skipped (not part of any runnable suite under our convention).
- Empty shards (shard_count > target_count) dropped from output so
matrix expansion doesn't spawn empty jobs.
- Shard ids are deterministic (sorted by id) so matrix consumers can
rely on shard 0 always existing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three fixes from the silent-failure-hunter agent on 44d6450, all gated against the same failure mode (a wired BUILD target silently disappearing from the test manifest): CRITICAL — convert silent skip to hard error in _extract_targets: - Target carrying sgl-suite-X but missing or malformed est_time:N is now a hard error with a "Re-run codegen" hint. The prior `continue` would have produced a "looks-wired, isn't-wired" bug: BUILD shows the suite tag, bazel query lists the test, but the manifest excludes it and CI never runs it. - Asymmetric: missing-suite is still a legitimate skip (target isn't claimed by any CI suite); only suite-without-est_time is fatal. IMPORTANT — JSON load and missing-results-key error UX: - Wrap json.loads with json.JSONDecodeError and OSError handlers. Each path exits with file path, byte size, and likely cause. - Validate "results" in payload — Bazel's jsonproto for zero matches is {"results": []}, never {}. Missing key is a truncated payload. IMPORTANT — drift detection via GHA workflow annotations: - "suite has targets but no --shards entry" was an INFO line on stderr, invisible in PR Checks. Promote to ::warning:: format on stderr (stdout is reserved for the JSON manifest). Surfaces on the GitHub Actions PR page. IMPORTANT — bazel jsonproto schema fragility: - If the tags attribute exists but lacks stringListValue, append to errors instead of silently treating as no-tags. YAML belt-and-suspenders: - Explicit `set -euo pipefail` at the top of the Build manifest step. GHA's bash default already includes -eo pipefail, but documents intent against future edits splitting the cquery + python3 pair. Validated on H100: - Happy path: stdout = pure JSON, stderr = ::warning:: lines. - Synthetic sgl-suite-X-without-est_time exits 1 with attributable msg. - Synthetic empty-{} jsonproto exits 1 with truncation hint. - Synthetic non-JSON file exits 1 with size + decode error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the manifest-driven matrix that PR description listed as out-of-
scope follow-up. Includes it in this PR per reviewer request since
stage-b-test-1-gpu-small is where the bulk of the wired tests live.
compute-test-manifest now exposes `shards` as a job output (in addition
to the artifact upload), so downstream matrices can read it without an
extra download-artifact round-trip.
New job stage-b-test-1-gpu-small-bazel-sharded:
- needs: [..., compute-test-manifest]
- matrix.shard expands over fromJSON(needs...outputs.shards)['stage-b-...']
- continue-on-error so a Bazel-side failure can never block the existing
run_suite.py-driven shard jobs during shadow-running.
- Coexists with the existing stage-b-test-1-gpu-small (which still runs
the run_suite.py 8-way partition AND the partition-0 parallel-shipping
bazel step from feat/bazel-pr-test-yml). Both can be dropped once
parity is verified for one or two release cycles.
- Each shard runs `bazel test ${{ matrix.shard.targets }}` — the
space-separated label list that bin_pack_shards.py packed into this
shard.
- Coredump suffix is `bazel-shard-<id>` so artifacts don't collide with
the existing partition-id-suffixed uploads.
Validated on H100 by simulating one shard's worth of work:
- bazel cquery → bin_pack_shards.py → manifest.json
- Extract shard 7's targets via jq-equivalent in python
- bazel test $TARGETS --cache_test_results=no
- 2 packed tests run, 2 pass in 22.7s (est_time was 16s for the shard).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two important fixes from the code-reviewer agent on 35bea9c: - Bazelisk install in stage-b-test-1-gpu-small-bazel-sharded was using `sudo mv` but the existing partition-0 install in stage-b-test-1-gpu- small (the proven path on this runner class) uses bare `mv`. The 1-gpu-5090 self-hosted runners are configured with the runner user owning /usr/local/bin, so sudo would fail. Switched to bare `mv` to match. The compute-test-manifest job stays on `sudo mv` because it runs on ubuntu-latest (GitHub-hosted) where sudo is required. All three install blocks now agree by runner type. Comment added flagging the divergence so a future composite-action lift handles both variants. - Added max-parallel cap on the new sharded job, mirroring the predecessor stage-b-test-1-gpu-small (`fromJson(needs.check-changes. outputs.max_parallel_small)` = 3 for filtered runs, 8 for full). Without the cap, full runs would have spawned 8 unthrottled bazel shards on top of the existing 8 run_suite.py partitions, starving the 1-gpu-5090 pool for other in-flight PRs. The cap returns the bazel path to roughly the same runner footprint as the existing flow during the shadow-running window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Demo stack — stage 3 of 3 (final)
This stage adds the Phase-3 building blocks: a manifest job that emits est_time-bin-packed shards consumable as a GHA matrix. Replaces (eventually) both the heuristic check-changes job and run_suite.py's auto_partition.
Stack:
Components
Validation on H100
Review history (in chat)
code-reviewer: ship it (no Critical/Important).
silent-failure-hunter found two CRITICAL test-disappearance failure modes plus two IMPORTANT UX issues. All addressed in 33fb929:
🤖 Generated with Claude Code