feat(bench): rocm bench load — GPU-saturating load generator for the dashboard#90
Open
michaelroy-amd wants to merge 6 commits into
Open
feat(bench): rocm bench load — GPU-saturating load generator for the dashboard#90michaelroy-amd wants to merge 6 commits into
michaelroy-amd wants to merge 6 commits into
Conversation
Rewrite the bench load-generator plan after an adversarial review pass against current main. Tightened to ~360 LOC / one new file: writer lives in collectors (daemon has no csv dep), top-level 'rocm bench load' verb (Dash untouched), one aggregate row per cell, distinct opt-in output CSV, http-only with a clear https rejection, and a fixed not-an-official- benchmark caveat in --help and stdout. Zero shipped deps; one dev-dep (wiremock). Supersedes the 2026-06-22 draft. Signed-off-by: Michael Roy <michael.roy@amd.com>
Phase 1 of the bench load generator: the consumer pipeline (CSV tailer -> daemon -> BenchRing -> bench tab) already shipped; this adds the missing producer. 'rocm bench load' fans out concurrent /chat/completions requests at a local OpenAI-compatible endpoint, measures client-side gen_tps and prompt_tps over a wall makespan, and appends one aggregate BenchmarkRow per concurrency cell to a distinct CSV a running daemon tails -- so the bench tab lights up live with zero daemon/protocol/UI changes. - collectors/bench_load.rs: LoadSpec, run_cell (JoinSet + owned-permit Semaphore, hand-serialized POST like lemonade.rs, per-request Result isolation, pinned makespan), run_and_append_csv (O_APPEND single newline-terminated write_all per row, header-if-empty, refuses on header mismatch). thiserror errors; no new shipped deps. - top-level Command::Bench/Load (Dash untouched); http-only with a clear https rejection; distinct opt-in --out (never the shared results dir); raw-throughput-vs-agent-workload caveat in --help and stdout. - tests T1-T5 (wiremock dev-dep) under --test-threads=1; T2 verifies the concurrency cap is actually enforced. Quality fields default to Unknown so throughput rows never claim pass/fail. Signed-off-by: Michael Roy <michael.roy@amd.com>
…gger (2) Phase 1.5 and Phase 2 of 'rocm bench load', making it feature-complete. Phase 1.5 (collectors, vLLM-only, zero new deps): each cell scrapes the endpoint's /metrics before/after for cumulative TTFT/TPOT histogram deltas, and a lightweight mid-cell poller (250ms, 1s scrape timeout, AtomicBool stop + join) tracks true peak running/waiting reqs. CSV header extended 14->18 (max_running_reqs, max_waiting_reqs, ttft_ms, tpot_ms); server-side gen_tps discarded in favour of the client-measured value. Non-vLLM endpoints degrade to None, never stalling the sweep. Latency shows in the bench detail panel. Phase 2: - auto-ramp (--auto-ramp): doubling sweep 1..128 that stops on a pure should_stop_ramp (plateau <5% gain, or queue saturation guarded on running>0, or hard cap); rows append incrementally so the tab fills live. - TUI trigger: press 'b' on Observe to open a bench-run form that spawns 'rocm bench load' via the existing job system, defaulting --out to the daemon-tailed CSV so rows stream into the bench tab. No protocol/server/ runner changes (Command::RunBench deliberately not built). Tests: Prometheus delta + non-vLLM degradation + 18-col round-trip + header guard; deterministic should_stop_ramp cases incl. an at-rest-peaks regression guard; overlay invariants + SpawnJob args + https rejection. All under --test-threads=1; clippy --all-targets -D warnings clean. Signed-off-by: Michael Roy <michael.roy@amd.com>
Member
Author
CodeQL "uncontrolled data in path expression" — assessed and dismissedAll 18
Each alert carries this rationale in its dismissal note. Happy to revisit if a future change routes an untrusted/remote value into the output path. |
…pulates bench_results_dir (the file the daemon's CsvBenchTailer tails for normalized benchmark rows) defaulted to None in both the unified rocm-core config and the standalone rocm-dash config, so the Bench panel stayed empty even after running rocm bench load, unless a user hand-edited a config file to point at a CSV. Separately, rocm bench load with no --out wrote each run to a distinct self-labeled ~/.rocm/data/bench/rocm-bench-<timestamp>.csv file rather than the shared tailed path, so a plain CLI invocation was never visible to the daemon even when bench_results_dir was set. - Default bench_results_dir to <data_dir>/bench/results.csv in both DashboardDaemonConfig (rocm-core) and DaemonConfig (rocm-dash-core). - Default rocm bench load's --out (when omitted) to that same file so a plain CLI run appends to what the daemon already tails. - Fix the Bench tab's empty-state copy, which referenced a nonexistent --bench-csv daemon flag (bench_results_dir is config-only; there is no such CLI flag on any binary). Relates to EAI-7361 Signed-off-by: Michael Roy <michael.roy@amd.com>
…dir override Adversarial-review follow-up for EAI-7361: - Fix cross-binary path mismatch: rocm-dash-core's default resolved to $HOME/.rocm/data/bench/results.csv (extra /data/), but rocm-core's default_data_dir() is $HOME/.rocm itself, so the producer writes to $HOME/.rocm/bench/results.csv. Remove the stray /data/ segment so both configs resolve to the same absolute path the producer uses. - Honor ROCM_CLI_DATA_DIR on the daemon side: rocm-core's default now resolves through the same AppPaths::discover() the producer uses (not bare default_data_dir(), which ignores the override), and rocm-dash-core's default honors the override too. Previously, with the override set, a plain `rocm bench load` wrote a file the daemon never tailed. - Add a cross-binary regression test asserting the producer default (default_bench_csv_path) and the daemon-tailed default (DashboardDaemonConfig::bench_results_dir) resolve to the same path, plus an override-precedence test in rocm-dash-core. - Reword the Bench empty-state copy: "bench directory" -> "bench results file" (bench_results_dir is a single tailed file, not a directory). - Fix the --out help text's default path (drop the stray /data/). Relates to EAI-7361 Signed-off-by: Michael Roy <michael.roy@amd.com>
CI follow-up for EAI-7361: - Regenerate MANIFEST.md for the dependency additions on this branch (wiremock dev-dep tree: assert-json-diff, deadpool, num_cpus, etc.). Verified with `cargo xtask manifest --check`. - Harden CsvBenchTailer::drain against the CodeQL rust/path-injection alert at the File::open sink: the tailed path is derived from config / $ROCM_CLI_DATA_DIR, so route it through validated_read_path(), which canonicalizes the path (resolving symlinks and `..`) and rejects traversal before opening. The bench results file is intentionally user-configurable to any location, so canonicalization — not a fixed-root containment check — is the correct barrier. Canonicalization fails on an absent file exactly as the previous bare File::open did, so the daemon tick loop keeps tolerating a not-yet-created CSV. Relates to EAI-7361 Signed-off-by: Michael Roy <michael.roy@amd.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
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.
Summary
Makes the bench section of the dashboard's Observe pane functional. The consumer half (CSV tailer → daemon →
BenchRing→ bench tab) already shipped; this adds the missing producer:rocm bench load, a minimal GPU-saturating load generator, then layers on server-side latency/peaks and a one-click TUI trigger.The design was kept deliberately low-blast-radius: the generator appends normalized rows to the CSV a running daemon already tails, so the bench tab lights up live with no daemon/protocol/server/runner changes.
rocm bench load(Phase 1)Fans out concurrent
/chat/completionsrequests at a local OpenAI-compatible endpoint, measures client-sidegen_tps/prompt_tpsover a wall makespan, and writes one aggregateBenchmarkRowper concurrency cell.tokioJoinSet+ owned-permitSemaphore; per-requestResultisolation (one refusal never zeroes a cell); makespan pinned before fan-out.O_APPEND, one newline-terminatedwrite_allper row (survives the live tailer), header-once, refuses to append on header mismatch (won't corrupt an external agent-bench CSV).Command::Bench/bench load(thedashcommand is untouched).http://-only with a clearhttps://rejection. Quality fields default toUnknown, so throughput rows never claim pass/fail.Server-side latency + peaks (Phase 1.5, vLLM-only)
/metricsbefore/after each cell for cumulative TTFT/TPOT histogram deltas, and runs a lightweight mid-cell poller (250 ms, 1 s scrape timeout) to capture true peak running/waiting requests.max_running_reqs,max_waiting_reqs,ttft_ms,tpot_ms); shown in the bench detail panel. Non-vLLM endpoints degrade toNoneand never stall the sweep.Auto-ramp + one-click TUI trigger (Phase 2)
--auto-ramp: doubling sweep 1→128 that stops on a pureshould_stop_ramp(throughput plateau <5%, queue saturation guarded onrunning > 0, or hard cap); rows append incrementally so the tab fills live.bon the Observe tab to open a bench-run form that launchesrocm bench loadvia the existing job system, defaulting--outto the daemon-tailed CSV so results stream into the bench tab. Implemented withSpawnJob— no protocol change (an in-processCommand::RunBenchwas evaluated and deliberately dropped for buying only ~1 s over the CSV tailer).Dependencies
Zero new shipped deps. One new dev-dep:
wiremock(HTTP stub for tests).Test plan
cargo test -p rocm-dash-collectors -- --test-threads=1— generator fields/tps, concurrency-cap enforcement, CSV round-trip + header guard, Prometheus delta + non-vLLM degradation, 18-col round-trip, deterministicshould_stop_ramp(incl. an at-rest-peaks regression guard)cargo test -p rocm-dash-tui -- --test-threads=1— overlay invariants,SpawnJobargs,https://rejection, tailed-path defaultcargo build -p rocmcargo clippy --all-targets -- -D warnings— cleanrocm served model (#[ignore]in-tree)Notes for reviewers
plans/rocm-bench-load-generator.md./metrics); throughput works on any OpenAI-compatible endpoint.--helpand stdout.EAI-7361 addendum — populate the dashboard Bench panel
Later commits on this branch fix the observability defect that left the dashboard's Bench panel permanently empty:
bench_results_dir(the CSV file the daemon'sCsvBenchTailertails) defaulted toNonein both the unifiedrocm-coreconfig (DashboardDaemonConfig) and the standalonerocm-dash-coreconfig (DaemonConfig), so the daemon tailed nothing unless a user hand-edited a config file — even after runningrocm bench load. Additionally,rocm bench loadwith no--outwrote each run to a distinct timestamped file, so a plain CLI run was never on the tailed path.bench_results_dirto<data_dir>/bench/results.csv, resolved through the sameAppPaths::discover()data-dir resolution the producer uses (so theROCM_CLI_DATA_DIRoverride and host path normalization are honored identically on both sides).rocm bench load's no---outdefault now appends to that same file. A cross-binary regression test asserts producer default == daemon-tailed default resolve to the same absolute path.--bench-csvdaemon flag;bench_results_diris config-only) and reworded "directory" → "results file" since it is a single tailed file.Intentional behavior change (reviewer note)
With a shared default output path (
~/.rocm/bench/results.csv), a pre-existing CSV at that path whose header does not match the normalized benchmark schema now yields a loudHeaderMismatchrefusal on the nextrocm bench load, instead of the old behavior of silently writing a fresh timestamped file. This is intentional: a single canonical, daemon-tailed file is what makes the panel populate, and a schema mismatch should surface rather than fragment results across files. Users who want an ad-hoc separate file can still pass--out <path>.Note: an earlier commit message on this branch stated the two configs' defaults were "identical / picked up here too" before they actually were — the extra
/data/segment in the rocm-dash-core path made them diverge. That mismatch is corrected by the alignment commit above; the defaults now genuinely resolve to the same path.