Skip to content

[Rust graph stack #8] Add a per-phase worker benchmark harness - #329

Merged
NSagan271 merged 7 commits into
rust-stack/08-rust-defaultfrom
rust-stack/09-bench
Sep 29, 2026
Merged

NSagan271 merged 7 commits into
rust-stack/08-rust-defaultfrom
rust-stack/09-bench

Conversation

@NSagan271

Copy link
Copy Markdown
Collaborator

What does this PR do?

Final rust graph PR, benchmarking code.

benchmark/worker_phases wraps mstar serve with MSTAR_PHASE_TIMING set, parses the worker's periodic phase-timing lines and prints one table per steady-state segment. Clients for Orpheus, Qwen3-Omni and BAGEL drive it closed-loop.

The worker now reports the mean in-flight batch size with each flush (bs=), which is what the harness segments on: ramp-up, steady state and drain are separate regimes, and one mean across them describes none. It also times the pieces of postprocess an A/B between graph runtimes needs apart: the completion sync, the stop check, storing outputs, the routing call, registration, and building vs. making the send.

How was it tested?

(testing in progress)

Checklist

  • ruff check . passes
  • Added or updated tests / docs where relevant

@NSagan271
NSagan271 added this pull request to stack #323 September 24, 2026 16:56
@NSagan271 NSagan271 changed the title [Rust graph stack #9] Add a per-phase worker benchmark harness [Rust graph stack #8] Add a per-phase worker benchmark harness Sep 24, 2026
@NSagan271
NSagan271 force-pushed the rust-stack/09-bench branch 2 times, most recently from 0692dc0 to 21393ce Compare September 25, 2026 05:01
@NSagan271
NSagan271 force-pushed the rust-stack/09-bench branch 2 times, most recently from 1857c7c to 3a6e6d7 Compare September 25, 2026 06:35
@NSagan271
NSagan271 force-pushed the rust-stack/09-bench branch 3 times, most recently from bf665e6 to ab0bd2d Compare September 26, 2026 02:04
@merceod

merceod commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator

Used it for the #328 A/B and it does the job. The parser and segmenter tests pass, the client scripts line up with the runner's flags, and on two real three pass closed loop runs it split each worker's log into batch 1, 7.5, 28 and 47 regimes with the defaults and no hand tuning. I checked the numbers by hand. For every segment where the worker was continuously busy the sum of iter_total over the segment matches the wall clock between its first and last flush within 3 percent, and the segments that do not match are the ones spanning a gap between two runner passes, which the README already explains. The postprocess split is what was needed to see where the Rust runtime saves its time (route, send, speculate) and what it does not touch (register_outputs, check_stop). Merge.

Two notes. Segmentation compares each record against the segment's running mean, so a slow ramp where each flush moves under 15 percent would stay in one segment. It never showed here because the ramps are fast, but comparing against the segment's first record or capping a segment's bs range would make it robust. And a line in the README saying that TTFT and ITL come from the runner's closed loop, not from the harness, would save a reader the question.

NSagan271 and others added 4 commits September 29, 2026 03:04
benchmark/worker_phases wraps `mstar serve` with MSTAR_PHASE_TIMING set,
parses the worker's periodic phase-timing lines and prints one table per
steady-state segment. Clients for Orpheus, Qwen3-Omni and BAGEL drive it
closed-loop.

The worker now reports the mean in-flight batch size with each flush
(`bs=`), which is what the harness segments on: ramp-up, steady state and
drain are separate regimes, and one mean across them describes none. It also
times the pieces of postprocess an A/B between graph runtimes needs apart:
the completion sync, the stop check, storing outputs, the routing call,
registration, and building vs. making the send.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@Gaurav-Shah05 Gaurav-Shah05 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instrumentation side holds up. Two nits: the clients default to port 8000 while every server command here uses 8100, and qwen3omni_t2s.sh's ${CONC:-4} is dead because _common.sh already set CONC=16, so it runs 16 concurrent against 12 requests and never holds a steady batch.

print(f"[worker_phases] server exited ({proc.returncode}) before "
f"becoming ready", file=sys.stderr, flush=True)
return
if _healthy(port):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A server left over from an earlier run answers this, so SERVER READY can fire before the child is up. The proc.poll() check catches a child that died, not one that never owned the port.

"records": len(a["p50s"]),
# us of this phase per 1000 request-steps; 0.0 when the log
# has no bs (older workers), which the renderer omits.
"us_per_1k": a["sum"] / work * 1e6 if work else 0.0,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a["sum"] counts phases sampled on passes that never increment phase_iter, like worker.schedule on an idle poll, but request_steps only counts iterations that did. So us/1k reads high for the scheduler phases and right for the postprocess ones, with nothing in the table saying which.

proc = subprocess.Popen(
args.command, shell=True, env=env, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT, text=True, bufsize=1,
start_new_session=True, # so the whole tree can be signalled

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing installs a SIGTERM or SIGHUP handler, so either signal skips the finally and orphans the workers with their GPU memory. Same failure the killpg below is there to prevent.

Comment thread benchmark/worker_phases/README.md Outdated
A closed-loop run ramps up, holds at max concurrency, then drains stragglers.
Those are three different regimes, and one mean across all of them describes
none of them. Each record carries `bs=`, the mean in-flight requests over its
window; a record whose `bs` differs from the running mean by more than

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

segment compares against cur.records[0].bs now, not the running mean.

@NSagan271
NSagan271 merged commit 65faaa1 into main Sep 29, 2026
4 checks passed
@NSagan271
NSagan271 deleted the rust-stack/09-bench branch September 29, 2026 06:32
NSagan271 added a commit that referenced this pull request Oct 2, 2026
Main landed the Rust graph stack (#320-#329), which moved worker graph
state behind a GraphRuntime contract, and several fixes this PR had made
independently. Resolutions:

- cuda_graph_config / cuda_graph_runner: take main's `input_seq_dims`
  (#251's form of the same fix) and drop this PR's `static_seq_dims`.
  The denoise loop's config and its test move to the kept name; the
  PR's test_cuda_graph_static_seq_dims.py goes -- test_cuda_graph_capture.py
  already covers the override, the collision it exists for, and the reslice.

- graph/base.py: main's inline `carried_names` and this PR's
  `persisted_input_names()` are the same gate. Keep the named helper and
  call it from the runtime's spec-input gather too, which had a third copy.

- worker.py: main moved input gathering and the spec-prep helpers into the
  runtime, so `_get_input_tensors`' persisted-input carry-over is already
  there (runtime/python.py). `_build_executing_batch`'s loop-counter refresh
  gives way to main's batched one in the exec loop. `_speculative_fwd_info`
  stays -- #275 has not landed -- rebased onto request_state / the batched
  `get_dynamic_loop_iters`, and now builds the whole batch's info in one
  crossing. Its test follows the new API.

- The persist-registration gate drops out: `_register_outputs` now takes
  uuids the runtime already routed, so the check belongs in the runtime
  (both impls) rather than the worker. Reinstated separately, which is
  also where the review comment asking for a narrower check lands.

- client.py, test_client_sdk.py, test_openai_router.py: additive, both
  sides kept. `_build_files` takes main's stricter named-bytes test.

- ragged attention dtype: already identical to #291 on both sides.

CI core list green: 745 passed, 1 skipped.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants