ci: production-model matrix — build-once fan-out, gemma-4 + gpt-oss, concurrency stress suite#497
ci: production-model matrix — build-once fan-out, gemma-4 + gpt-oss, concurrency stress suite#497Gajesh2007 wants to merge 2 commits into
Conversation
…concurrency stress suite Rearchitects integration CI around the models the fleet actually serves. Qwen3.5 (retired) is gone from all defaults; every heavy job now runs against gemma-4-26b-qat-4bit (15.6 GB) and gpt-oss-20b (12.1 GB), solo, in parallel. Workflow (integration.yml + new .github/actions/e2e-setup composite): - build once (SwiftPM cache keyed off Package.resolved; metallib cached by mlx version so warm runs skip pip) -> artifact -> 7 fan-out jobs - integration + stress matrices over both prod models; profile on gemma; PR-only benchmark matrix with a single aggregated PR comment (benchmark-report job merges per-model markdown artifacts) - test jobs checkout WITHOUT submodules and use the prebuilt binary via DARKBLOOM_PROVIDER_BINARY/MLX_METALLIB_PATH (plus a copy into .build/debug so the testbed's cached-binary short-circuit holds) - models download via the provider's real path: darkbloom models download <id> --coordinator https://api.darkbloom.dev — R2 CDN with per-file and aggregate SHA-256 verification, i.e. the exact fleet code path - concurrency group cancels superseded runs per ref Testbed (e2e/): - TESTBED_MODEL / TESTBED_MODEL_GB env knobs; DefaultModelID() is the single resolution point (TESTBED_MODEL > legacy TESTBED_MODEL_ID > gpt-oss-20b default); prod model sizes registered; all 8 hardcoded Qwen literals in benchmark_test.go routed through it - Provider.Alive()/ExitCode() liveness accessors (atomics fed by the existing wait goroutine) NEW e2e/stress_test.go — TestStress_ConcurrentAdmission: 3 waves x 8 concurrent streams with 25% mid-stream cancels timed to race the next wave's batch admission. Asserts: provider process survives, zero hangs, no non-503 5xx clusters, no truncated non-victim streams, >=80% completion, accounting integrity. Local smoke against a dev engine build reproduced the broadcast_shapes fatal deterministically at concurrency >=2 — exactly the crash class this suite exists to catch. This PR's own stress legs on the master engine + prod models arbitrate whether the shipped engine is affected.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No threat-model-covered files were changed; this PR adds E2E/integration test infrastructure and a new Swift unit test that fall entirely outside current Trust boundaries touchedNone of the existing TB-001 through TB-009 boundaries are directly modified by this PR. Threat coverageNo T-xxx threats are affected — ✅/ New attack surface / threat-model gaps to noteTwo areas are worth a brief threat-model triage: 1.
2. 3. SEC-* findings resolvedNone. RecommendationNo security action required for this PR. Suggested threat-model maintenance:
🔐 Threat model: |
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
.github/actions/e2e-setup/action.yml:62-64— Hardcoded coordinator URL in shell command- Suggestion: Use environment variable or input parameter for coordinator URL to avoid hardcoding production endpoints
Performance — ✅ No issues found
Type_diligence — ✅ No issues found
Additive_complexity — ✅ No issues found
1 finding(s) total, 1 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
|
Found 6 test failures on Blacksmith runners: Failures
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7abbbc983b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runBenchmark(t, fmt.Sprintf("%d-provider-scaling", numProviders), | ||
| testbed.SuiteConfig{ | ||
| ModelSpecs: []testbed.ModelSpec{{ModelID: "mlx-community/Qwen3.5-0.8B-MLX-4bit", NumProviders: numProviders}}, | ||
| ModelSpecs: []testbed.ModelSpec{{ModelID: testbed.DefaultModelID(), NumProviders: numProviders}}, |
There was a problem hiding this comment.
When the PR benchmark matrix sets TESTBED_MODEL to gemma-4-26b-qat-4bit or gpt-oss-20b, this subtest now runs the scaling loop with up to 5 separate provider processes for that production model. Suite.startProviders starts one darkbloom process per NumProviders, and each process loads its own model weights on first routed request, so the gemma leg can require roughly 5 copies of a 15.6 GB model instead of the single-copy coverage the new matrix is meant to exercise; this is likely to OOM or invalidate the benchmark on CI runners that can only load one production model copy. Keep the multi-provider scaling cases on the tiny model or cap provider count for production-model matrix legs.
Useful? React with 👍 / 👎.
…rameterize coordinator URL - e2e/stress_test.go: the >=80%-of-non-cancelled completion floor was miscalibrated — on a 48GB virtual M4 Pro the coordinator correctly shed 21/24 gemma-4-26b requests with 429/503 (token-budget admission working, not failing). Shed is now unbounded; instead, >=90% of non-cancelled ACCEPTED streams (2xx, stream started) must complete cleanly, and at least one request overall must complete. Crash detection (provider Alive()), zero-hang, 5xx-cluster, and truncated-stream assertions are unchanged. - e2e/integration_test.go: TestIntegration_E2EEncryptionCorrectness now accepts non-empty content OR reasoning as proof of decryption — gpt-oss is a reasoning model and can spend the whole max_tokens budget on the reasoning channel, leaving content empty. Also bumps that test's max_tokens 20 -> 128 to give the final channel a chance. - ChunkSenderTests: the coalescing test asserted 'enqueue 5 -> 1 batch', but ChunkBatcher coalescing is best-effort per dispatch turn and a loaded runner can split the batch (observed 3+2 in CI). The test now gates the sink on a primer frame so the remaining 4 enqueues pile up behind the blocked serial queue and MUST coalesce — deterministic: exactly [0] then [1,2,3,4], order preserved. No production changes. - .github/actions/e2e-setup: add a coordinator-url input (default https://api.darkbloom.dev) consumed via env-var indirection instead of a hardcoded URL interpolated into the script body (review 3510753763).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47e4f9859f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| go test ./e2e/ -count=1 -v -timeout 30m -p=1 \ | ||
| -run 'TestIntegration' |
There was a problem hiding this comment.
When this matrix leg sets TESTBED_MODEL to gemma-4-26b-qat-4bit or gpt-oss-20b, this command runs every TestIntegration_*; those tests call startSuite, which starts a fresh provider process per test, and the provider lazily loads the selected model on the first routed request. That makes a single integration leg cold-load the 12–16 GB production model for nearly every integration test, so the 30m Go timeout/40m job timeout can be consumed by repeated model loads instead of test coverage. Please either keep the full integration suite on a tiny model and add a small production-model smoke, or share one warmed suite/provider for the production-model job.
Useful? React with 👍 / 👎.
ethenotethan
left a comment
ethenotethan
left a comment
There was a problem hiding this comment.
Automated Code Review — Layr-Labs/d-inference#
Verdict: REQUEST_CHANGES
Security — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
.github/actions/e2e-setup/action.yml:64-73— Shell injection via environment variables in model download command- Suggestion: Use array-based command execution or properly quote/escape the variables in the shell command
Performance — 1 finding(s) (1 blocking)
- 🟡 [MEDIUM]
e2e/stress_test.go:89— Stress test launches 24 goroutines without context cancellation or errgroup- Suggestion: Use errgroup.WithContext or add proper context cancellation to the goroutines in startStressWave to prevent goroutine leaks if the test context is cancelled
Type_diligence — ✅ No issues found
Additive_complexity — ✅ No issues found
2 finding(s) total, 2 blocking. Verdict: REQUEST_CHANGES.
🤖 Automated review by Centaur · DAR-186
| env: | ||
| # Env-var indirection (not raw ${{ }} interpolation into the script | ||
| # body) so the input values are never parsed as shell. | ||
| MODEL_ID: ${{ inputs.model }} | ||
| COORDINATOR_URL: ${{ inputs.coordinator-url }} | ||
| run: | | ||
| # Zero-config: `models download` falls back to hardware defaults when | ||
| # no ~/.darkbloom config exists, and --coordinator overrides the URL | ||
| # (see provider-swift/Sources/darkbloom/ModelsCommand.swift). | ||
| /tmp/provider-artifact/darkbloom models download "$MODEL_ID" \ |
There was a problem hiding this comment.
🟡 [MEDIUM] 🔒 Shell injection via environment variables in model download command
💡 Suggestion: Use array-based command execution or properly quote/escape the variables in the shell command
📊 Score: 3×4 = 12 · Category: command-injection
| ctx := context.Background() | ||
| s := testbed.NewSuite(testbed.SuiteConfig{ | ||
| NumUsers: 1, | ||
| SeedBalance: 500_000_000, |
There was a problem hiding this comment.
🟡 [MEDIUM] ⚡ Stress test launches 24 goroutines without context cancellation or errgroup
💡 Suggestion: Use errgroup.WithContext or add proper context cancellation to the goroutines in startStressWave to prevent goroutine leaks if the test context is cancelled
📊 Score: 3×4 = 12 · Category: unbounded_goroutines
2 participants
Summary
Integration CI now tests the models the fleet serves —
gemma-4-26b-qat-4bit(15.6 GB) andgpt-oss-20b(12.1 GB), solo per job, in parallel — instead of the retired Qwen3.5-0.8B. Build-once/fan-out architecture, aggressive caching, and a new concurrency stress suite that turns thebroadcast_shapesengine-crash class from a random flake into a deterministic signal.Before / After
flowchart LR subgraph Before A1["2 serial jobs<br/>each: full Swift rebuild (~8m)<br/>+ duplicated setup"] --> B1["tests vs Qwen3.5-0.8B only<br/>(retired model, 0.5 GB)"] B1 --> C1["~22m wall · no prod-model coverage<br/>no stress · flaky engine crashes"] end subgraph After A2["build once (SwiftPM+metallib cached)<br/>-> binary artifact"] --> B2["7 parallel jobs:<br/>integration x2 models · stress x2<br/>profile · benchmark x2 -> 1 PR comment"] B2 --> C2["~12-17m wall · both prod models<br/>R2+SHA-256 download path exercised<br/>deterministic crash detection"] endArchitecture
provider-binaryartifactfail-fast: false) over both prod models; profile (gemma); benchmark matrix (PR-only) → benchmark-report merges per-model markdown into one PR commentDARKBLOOM_PROVIDER_BINARY/MLX_METALLIB_PATH, plus a.build/debugcopy for the testbed's cached-binary short-circuit)darkbloom models download <id> --coordinator https://api.darkbloom.dev— R2 CDN with per-file + aggregate SHA-256 verification, no config bootstrap needed (verified: catalog is public,loadRuntimeSnapshotfalls back to hardware defaults)concurrency:group cancels superseded runs; shared setup lives in a composite action (.github/actions/e2e-setup)Testbed changes
TESTBED_MODEL/TESTBED_MODEL_GBknobs;DefaultModelID()is the single resolution point (default flipped togpt-oss-20b; all 8 hardcoded Qwen literals routed through it).gemma-3-270mstays only as the multi-model swap-mechanics target.Provider.Alive()/ExitCode()liveness accessors.New:
TestStress_ConcurrentAdmission3 waves × 8 concurrent streams; 25% of each wave cancelled mid-stream, timed so batch leaves race the next wave's batch joins. Asserts: provider process survives, zero hangs, no non-503 5xx clusters, no truncated non-victim streams, ≥80% completion, accounting integrity. Fail-fast on process death (~24s instead of 200s).
broadcast_shapesfatal deterministically at concurrency ≥2 (Shapes (1,8,22,64) and (1,1,14,64) cannot be broadcast). That was a WIP engine binary — this PR's own stress legs are the arbiter for whether master's engine + prod models are affected. If they come back red, that's the suite working as designed (deterministic evidence of the engine bug on shipped models), and the fix belongs in a follow-up engine PR — we can then decide whether stress blocks merge or runscontinue-on-erroruntil the engine lands.Second first-run validation: 15.6 GB of wired Metal memory in a virtualized M4 Pro (today's CI loads 0.5 GB).
Verification
gofmt/go vetclean; e2e + testbed unit tests green; workflow + composite YAML validated;actionlintclean (except pre-existing Blacksmith runner-label false positives)TestDefaultModelIDEnvOverride,TestKnownModelSizesProdEntries); prereq-free outcome-classifier testsNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.