[Feat] Add Dynamo provider support to brixbench - #2469
Conversation
eb48fb8 to
6be334c
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces support for the dynamo provider in the brixbench benchmark suite, enabling release-based Dynamo platform deployments via Helm and engine deployments using DynamoGraphDeployment manifests. Key changes include the implementation of the DynamoDeployer (with associated artifact capture and release management), refactoring of the AIBrixDeployer, updated test runner logic, and new test fixtures for Qwen3 models. The review feedback focuses on enhancing the robustness of the new deployer, suggesting improvements such as defensive nil checks, verifying local manifest existence before executing cleanup commands, handling corrupted git checkouts, making artifact capture resilient to individual write failures, and checking for context cancellation within retry and polling loops.
5b05cd8 to
b458058
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds first-class Dynamo support to the brixbench benchmark suite by extending scenario resolution/validation, introducing a release-based Dynamo deployer (Helm + user engine manifest), and adding VKE-focused Dynamo fixtures and comparison scenarios. It also improves runner ergonomics around namespace selection/cleanup and makes figure generation more resilient by supporting system Python fallbacks and warning-only skips when plotting dependencies are missing.
Changes:
- Add
provider: dynamosupport end-to-end (validation, release checkout, Helm install, readiness probing, teardown, and artifact capture). - Extend scenario schema to include
platform.valuesFile, and tighten provider-specific source input validation. - Add VKE Dynamo smoke/comparison scenarios and adjust the benchmark runner + figure generation fallback behavior.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| brixbench/README.md | Documents Dynamo provider behavior, prerequisites, namespaces, and plotting guidance. |
| brixbench/internal/resolver/resolver.go | Adds platform to test schema and routes validation through provider-specific validators. |
| brixbench/internal/resolver/resolver_test.go | Adds resolver tests for controlplane rejection and provider error cases. |
| brixbench/internal/resolver/provider_inputs.go | New provider-specific input validation entrypoint. |
| brixbench/internal/resolver/provider_inputs_test.go | Tests for provider input validation behavior. |
| brixbench/internal/resolver/dynamo_provider.go | Implements Dynamo version normalization and platform values file validation. |
| brixbench/internal/resolver/dynamo_provider_test.go | Tests Dynamo provider validation and normalization. |
| brixbench/internal/resolver/aibrix_provider.go | Introduces stricter AIBrix source-selection validation (exactly one of version/commit/localPath). |
| brixbench/internal/resolver/aibrix_provider_test.go | Tests AIBrix source-selection validation rules. |
| brixbench/internal/deployers/dynamo.go | Implements Dynamo deploy lifecycle (Helm install, apply engine manifest, readiness, teardown, command logging). |
| brixbench/internal/deployers/dynamo_test.go | Comprehensive unit tests for Dynamo deployer, release source behavior, and artifact capture. |
| brixbench/internal/deployers/dynamo_release.go | Adds git-backed Dynamo release preparation + stable tag validation. |
| brixbench/internal/deployers/dynamo_artifacts.go | Adds bounded Dynamo artifact capture and Helm status capture. |
| brixbench/internal/deployers/deployer.go | Extends deployer config with PlatformValuesFile. |
| brixbench/internal/deployers/aibrix.go | Refactors/moves AIBrix source/deploy helpers out of the main file. |
| brixbench/internal/deployers/aibrix_source.go | New file holding extracted AIBrix source preparation and Helm helpers. |
| brixbench/benchmark/testdata/scenarios/dynamo-hello-world-vke.yaml | Adds a Dynamo VKE smoke scenario. |
| brixbench/benchmark/testdata/scenarios/aibrix-dynamo-comparison-vke.yaml | Adds a combined AIBrix vs Dynamo comparison scenario for VKE. |
| brixbench/benchmark/testdata/deployments/dynamo/qwen3-8b-round-robin-1p1d-tp2-vke.yaml | Adds DynamoGraphDeployment + prerequisites fixture for Qwen3-8B VKE smoke. |
| brixbench/benchmark/testdata/deployments/dynamo/qwen3-8b-round-robin-1p1d-tp2-vke-comparison.yaml | Adds Dynamo fixture tuned for comparison runs. |
| brixbench/benchmark/testdata/deployments/dynamo/qwen3-32b-round-robin-4p8d-vke.yaml | Adds a larger Dynamo VKE sample deployment fixture. |
| brixbench/benchmark/testdata/deployments/dynamo/platform-no-pvc.yaml | Adds a minimal platform values override disabling PVC usage. |
| brixbench/benchmark/testdata/deployments/dynamo/platform-aibrix-mirror-dev-operator.yaml | Adds mirror/dev-operator platform override for VKE validation. |
| brixbench/benchmark/testdata/deployments/aibrix/models/qwen3-8b-pd-1p1d-tp2-vke-comparison.yaml | Adds AIBrix comparison deployment fixture to pair with Dynamo scenario. |
| brixbench/benchmark/testdata/benchmarks/vllm-chat-smoke-qwen3-8b-vke.yaml | Adds VKE smoke benchmark config used by the new scenarios. |
| brixbench/benchmark/runner_test.go | Selects benchmark namespace per provider, adds Dynamo stale cleanup, and improves failure cleanup + figure generation reporting. |
| brixbench/benchmark/runner_fallback_test.go | Adds tests for provider-specific runner helper behavior. |
| brixbench/benchmark/runner_artifacts_test.go | Adds system-Python fallback + skip-reason support for figure generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func resolveFigurePython(repoRoot string) (string, bool, error) { | ||
| for _, name := range []string{"python3", "python"} { | ||
| pythonPath, err := exec.LookPath(name) | ||
| if err == nil { | ||
| return pythonPath, true, nil |
| If a run logs that figure generation was skipped because | ||
| `.venv/bin/python` is not available, recreate the local plotting environment: | ||
|
|
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Implement Dynamo release-based deployment lifecycle, dedicated brixbench-dynamo namespace handling, runtime fixtures, and smoke benchmark coverage. Verified with: go test ./internal/deployers ./internal/resolver; go test -v ./benchmark -run TestAIBrixBenchmarkSuite -scenario testdata/scenarios/dynamo-hello-world.yaml -count=1 -timeout 90m. Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Signed-off-by: Misun Park <misuneeh@gmail.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: whalepark <48859248+whalepark@users.noreply.github.com>
d3b03d8 to
2507ff3
Compare
This PR adds Dynamo provider support to
brixbenchand enables AIBrix/Dynamo comparison scenarios.Changes include:
python3/python, with warning-only skips for missing plotting dependencies.