Skip to content

Give every subcommand the same runtime flags - #1368

Merged
justinchuby merged 2 commits into
mainfrom
unify/subcommand-options
Aug 19, 2026
Merged

justinchuby merged 2 commits into
mainfrom
unify/subcommand-options

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

The problem

serve answers the same four questions as generate, run and transcribe --
which backend, which device, how much VRAM, how much host RAM -- in a different
vocabulary. Before this change:

flag generate run transcribe serve
--backend yes yes yes no
--device yes yes yes no (spelled --native-device)
--host-ram-limit yes yes yes no
--cpu-cores yes yes yes no
model positionally yes yes n/a no (--model only)

So a command line learned from one subcommand is rejected by another.
onnx-genai serve MODEL --backend native --device cuda:6 -- the obvious
extrapolation from run -- fails twice in a row, once on the positional model
and once on --backend, and clap's usage text lists neither. The flag that
does work, --native-device, appears in no other subcommand's help, so there
is nowhere to discover it from.

The change

The flags now live once, in onnx-genai-server::runtime_args, and are
flattened into all four subcommands. That crate is the lowest layer both the
unified CLI and the standalone server binary already depend on, and it already
carries clap with the env feature. A flag added there appears everywhere with
the same name, grammar and help text, so the surfaces cannot drift apart again.

Duplicated definitions removed along the way: EngineArgs, CpuArgs, the
device enum, and the --backend, --device and memory-limit parsers each
existed twice, and parse_native_device had two implementations that disagreed
about whether auto and gpu were valid.

Behaviour changes

Each has a test:

  • serve accepts the model positionally. Both spellings share the model-source
    arg group, so giving it twice is refused while parsing rather than after the
    process has started.
  • Naming a --device implies the native backend on every subcommand. serve
    already worked this way through --native-device; previously
    generate --device cuda:0 parsed, ran on ORT and ignored the device.
  • --vram-limit and friends gain the env fallbacks serve already had.
  • --model-id alongside --models-dir/--models-config is an error instead of
    being silently ignored, which hid typo'd invocations.

--native-device keeps working and is hidden from help, so existing service
units are unaffected. --device wins if both are given.

Verification

  • cargo test -p onnx-genai-server --lib --features native-backend — 16 tests
    across cli:: and runtime_args::, all passing; full lib 237 passed.
  • cargo test -p onnx-genai-cli --lib --features native-cuda — 118 passed.
  • cargo fmt --all --check and cargo clippy clean.
  • End to end on a real native CUDA model: onnx-genai serve ~/models/... --backend native --device cuda:6 --addr 127.0.0.1:18082
    loads and serves a chat completion, which is the invocation that failed before.

justinchuby and others added 2 commits August 19, 2026 01:54
`serve` answered the same four questions as `generate`, `run` and
`transcribe` -- which backend, which device, how much VRAM, how much host
RAM -- and answered them in a different vocabulary. It had no `--backend`
and no `--device`; instead it inferred the backend from `--native-device`,
a name that existed on no other subcommand and was therefore undiscoverable
from any of their help output. It also took the model only as `--model`
while every other subcommand takes it positionally.

The practical cost is that a command line learned from one subcommand is
rejected by another. `onnx-genai serve MODEL --backend native --device
cuda:6` -- the obvious extrapolation from `run` -- failed twice in a row,
once on the positional model and once on `--backend`, with clap's usage
text listing neither.

The flags now live once, in `onnx-genai-server::runtime_args`, and are
flattened into all four subcommands. That crate is the lowest layer both
the unified CLI and the standalone server binary already depend on, and it
already carries clap with the `env` feature. A flag added there appears
everywhere with the same name, grammar and help text, so the surfaces
cannot drift apart again.

Behaviour changes that follow from unifying, each covered by a test:

- `serve` accepts the model positionally, sharing the model-source arg
  group so giving it twice is refused while parsing rather than after the
  process has started.
- Naming a `--device` implies the native backend on every subcommand.
  `serve` already worked this way via `--native-device`; previously
  `generate --device cuda:0` parsed, ran on ORT, and ignored the device.
- `--vram-limit` and the rest gain the env fallbacks `serve` had.
- `--model-id` alongside `--models-dir`/`--models-config` is now an error
  instead of being silently ignored, which hid typo'd invocations.

`--native-device` keeps working and is hidden from help, so existing
service units are unaffected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
The interactive-session test asserted a `--device cuda:3` survives into the
engine config, but the flag is only applied under `native-backend`: without
the native decoder compiled in there is nothing that could honor a device,
so the config keeps `None` and the test failed on a default build.

Assert the backend unconditionally, since that part holds either way, and
the device only where a device means something.

Also drop an empty test that was left behind as a placeholder; an empty
body asserts nothing while reading in output as a passing case.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
@justinchuby
justinchuby merged commit 261b211 into main Aug 19, 2026
5 checks passed
@justinchuby
justinchuby deleted the unify/subcommand-options branch August 19, 2026 05:47
@github-actions

Copy link
Copy Markdown

🔴 Benchmark Regression Detected

Comparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).

ℹ️ Absolute times are informational only — they vary with runner load. The % change column is the reliable signal because both sides ran under identical conditions.

Status Scenario Base PR Change
🔴 block_quantized_matmul_cached_dense/mxfp4_uncached_dequant_each_call/1x1024x1024 519.85 µs 1.35 ms +158.9%
🔴 matmul/large_generic_bf16_threads=8/32x1024x1024 1.30 ms 2.00 ms +54.0%
🔴 block_quantized_matmul_cached_dense/mxfp4_cached_dense_repeated_call/1x1024x1024 40.76 µs 59.45 µs +45.8%
🔴 gather/medium_f16_threads=1-internal/32768 2.67 µs 3.71 µs +38.8%
🔴 matmul/medium_generic_f16_threads=1/32x512x512 30.08 µs 40.92 µs +36.0%
🔴 block_quantized_matmul_cached_dense/mxfp4_preexpanded_dense_oncelock_like_proxy/1x1024x1024 40.63 µs 53.29 µs +31.1%
🔴 block_quantized_moe_cached_dense/mxfp4_cached_dense_expert_repeated_call/rows=1,H=256,I=256,E=4,top_k=1 55.68 µs 72.99 µs +31.1%
⚠️ matmul/medium_generic_f16_threads=8/32x512x512 29.49 µs 37.80 µs +28.2%
⚠️ gather/medium_bf16_threads=1-internal/32768 2.57 µs 3.11 µs +21.1%
⚠️ matmul/large_generic_f32_threads=1/32x1024x1024 9.19 ms 11.03 ms +20.0%
⚠️ block_quantized_moe_cached_dense/mxfp4_uncached_expert_dequant_each_call/rows=1,H=256,I=256,E=4,top_k=1 354.01 µs 424.52 µs +19.9%
⚠️ sampling_latency/top_k_per_token 53.51 µs 63.67 µs +19.0%
✅ matmul/large_generic_f16_threads=1/32x1024x1024 80.35 µs 90.80 µs +13.0%
✅ gather/small_f16_threads=1-internal/4096 529.5 ns 597.5 ns +12.8%
✅ matmul/large_generic_f16_threads=8/32x1024x1024 95.66 µs 107.41 µs +12.3%
✅ sampling_latency/top_p_per_token 390.09 µs 436.73 µs +12.0%
✅ matmul/large_generic_bf16_threads=1/32x1024x1024 2.01 ms 2.23 ms +10.7%
✅ matmul/small_generic_f32_threads=8/1x256x256 59.78 µs 65.87 µs +10.2%
✅ gather/small_bf16_threads=1-internal/4096 478.0 ns 524.9 ns +9.8%
✅ reduce_mean/large_f32_threads=1-internal/262144 1.07 ms 1.17 ms +9.7%
✅ qwen3_sampling_processors/top_p_full_sort_after_top_k_baseline 3.62 ms 3.93 ms +8.5%
✅ qwen3_sampling_processors/top_p_fast_after_top_k 564.64 µs 598.16 µs +5.9%
✅ gather/small_f32_threads=1-internal/4096 970.7 ns 1.02 µs +5.3%
✅ matmul/medium_generic_bf16_threads=1/32x512x512 550.64 µs 570.25 µs +3.6%
✅ gather/medium_f32_threads=1-internal/32768 4.14 µs 4.27 µs +3.3%
✅ sampling_latency/min_p_per_token 232.69 µs 237.97 µs +2.3%
✅ matmul/small_generic_bf16_threads=8/1x256x256 39.03 µs 39.76 µs +1.9%
✅ logit_processing/seven_processor_chain_per_step 322.18 µs 325.54 µs +1.0%
✅ matmul/large_generic_f32_threads=8/32x1024x1024 4.78 ms 4.80 ms +0.4%
✅ grammar_masking/llguidance_compute_mask/32 76.89 µs 76.54 µs -0.5%
✅ matmul/small_generic_bf16_threads=1/1x256x256 39.78 µs 39.13 µs -1.6%
✅ matmul/medium_generic_f32_threads=1/32x512x512 2.62 ms 2.55 ms -2.4%
✅ tokenization/decode_tokens_per_second 7.64 ms 7.38 ms -3.4%
✅ matmul/small_generic_f32_threads=1/1x256x256 48.92 µs 46.72 µs -4.5%
✅ gather/large_bf16_threads=1-internal/131072 16.74 µs 15.98 µs -4.5%
✅ kv_cache/alloc_dealloc_pages 40.85 µs 38.92 µs -4.7%
✅ qwen3_sampling_processors/top_k_full_sort_baseline 2.80 ms 2.66 ms -5.0%
✅ qwen3_sampling_processors/top_k_top_p_full_sort_baseline 6.56 ms 6.20 ms -5.6%
✅ sampling_latency/greedy_per_token 3.75 µs 3.49 µs -7.1%
✅ reduce_mean/medium_f32_threads=1-internal/65536 284.81 µs 261.96 µs -8.0%
✅ matmul/small_generic_f16_threads=1/1x256x256 48.20 µs 43.79 µs -9.2%
✅ qwen3_sampling_processors/top_k_partial_selection 189.09 µs 171.75 µs -9.2%
✅ qwen3_sampling_processors/top_k_top_p_fast 827.79 µs 745.41 µs -10.0%
✅ tokenization/encode_tokens_per_second 460.34 µs 410.32 µs -10.9%
🟢 gather/large_f16_threads=1-internal/131072 15.55 µs 13.21 µs -15.0%
🟢 matmul/medium_generic_bf16_threads=8/32x512x512 579.98 µs 487.72 µs -15.9%
🟢 gather/large_f32_threads=1-internal/131072 37.34 µs 30.02 µs -19.6%
🟢 add/small_f16_threads=1-internal/1024 541.7 ns 419.1 ns -22.6%
🟢 matmul/medium_generic_f32_threads=8/32x512x512 1.32 ms 1.01 ms -23.5%
🟢 matmul/small_generic_f16_threads=8/1x256x256 45.01 µs 34.18 µs -24.1%
🟢 add/large_bf16_threads=1-internal/4194304 2.09 ms 1.55 ms -25.9%
🟢 add/large_f16_threads=1-internal/4194304 2.13 ms 1.54 ms -27.8%
🟢 reduce_mean/small_f32_threads=1-internal/4096 19.52 µs 14.09 µs -27.9%
🟢 add/small_bf16_threads=1-internal/1024 644.4 ns 447.4 ns -30.6%
🟢 add/small_f32_threads=1-internal/1024 329.5 ns 203.6 ns -38.2%
🟢 add/large_f32_threads=1-internal/4194304 983.92 µs 565.98 µs -42.5%
🟢 add/medium_f32_threads=1-internal/262144 42.32 µs 22.35 µs -47.2%
🟢 add/medium_bf16_threads=1-internal/262144 201.23 µs 96.10 µs -52.2%
🟢 add/medium_f16_threads=1-internal/262144 277.02 µs 99.19 µs -64.2%

Visual flags: ⚠️ ≥ 15% slower, 🔴 ≥ 30% slower — calibrated against measured runner noise (~27% worst-case on multi-threaded matmul)

Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.5.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 3.51 3.60 5.91 }
What this cannot catch
  • Regressions in code paths not covered by these benchmarks (e.g., end-to-end decode with a real model)
  • Sub-threshold regressions that compound over multiple PRs
  • Performance changes that only manifest under GPU execution
  • Latency changes in the ORT integration path (these benchmarks exercise the native Rust kernels)

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.

1 participant