Skip to content

test(bench): --no-thinking / --thinking-budget toggles for bench_isl_osl#251

Open
rsafier wants to merge 1 commit into
mainfrom
test/bench-thinking-toggle
Open

test(bench): --no-thinking / --thinking-budget toggles for bench_isl_osl#251
rsafier wants to merge 1 commit into
mainfrom
test/bench-thinking-toggle

Conversation

@rsafier

@rsafier rsafier commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

What

Adds --no-thinking and --thinking-budget N to bench/bench_isl_osl.py. Both inject chat_template_kwargs (enable_thinking, thinking_budget) into each request so streaming TTFT/TPOT can be measured with reasoning off or at a fixed budget.

Why

On thinking-default models (e.g. Holo, max_thinking_budget=768) reasoning tokens dominate TTFT, which swamps the prefill signal you actually want to measure. These toggles let the bench isolate prefill/decode from reasoning overhead.

Tooling-only; no engine changes.


Related PRs — independent, all target main

Now that #229 is merged, these three each cherry-pick cleanly onto current main with zero conflicts and are independent (no stacking) — review/merge in any order:

Follow-ups (not yet opened): Tier-2 FlashInfer-prefill + decode-concurrency reconciliation vs #229 FI (needs a merge, not a clean pick); and closing older stacked PRs folded into #229 (e.g. #220 FlashInfer-GDN AOT) after a rebase-diff confirms no outstanding delta.

@SeedSource

Copy link
Copy Markdown
Collaborator

[BETA] Automated PR review — SeedSource AI pipeline

Advisory only, not a merge gate. AI-generated (internal risk-triage, Atlas serve gate, GLM-5.2 review), attributed per Atlas convention.

Triage

  • Risk: low - serve-test-required (triage advisory): False - path-heuristic (authority): False
  • Rationale: Benchmark script update for thinking control parameters; no production or quantization changes.

Serve gate

  • Skipped - no serving-path files (docs/CLI/test/example). Zero GPU.

GLM-5.2 code review

Correctness: Sound. enable_thinking defaults to True, preserving prior behavior. chat_template_kwargs is the correct vLLM field for thinking controls. Warmup correctly inherits the toggles, ensuring measured runs aren't invalidated by a thinking-on warmup followed by thinking-off measurement.

Risk: Low. Benchmark-only script, no engine code touched. One concern: --no-thinking behavior depends on the server's chat template actually honoring enable_thinking — if the deployed template ignores it, the script will silently report "pure prefill TTFT" while actually measuring time-to-first-answer. No validation or warning if the response contains reasoning tokens when --no-thinking is set.

Quality issues:

  • Line 240 print statement is cramped and hard to read; split across lines.
  • thinking_budget=None in the signature should be Optional[int] = None for type clarity, though this is a bench script.
  • No guard against --thinking-budget being set without --no-thinking being False (i.e., budget with thinking off is contradictory) — minor, server likely ignores it.

Verdict: Approve. Consider adding a sanity check that --no-thinking responses contain no reasoning tokens.

Pipeline: build+serve on GB10, GLM-5.2 via NVIDIA NIM, internal risk-triage classifier. External diff treated as untrusted input. Public repo.

@rsafier rsafier force-pushed the test/bench-thinking-toggle branch from 58728a5 to f7c1649 Compare July 12, 2026 23:06
…_isl_osl

Inject chat_template_kwargs {enable_thinking, thinking_budget} into each
request so streaming TTFT/TPOT can be measured with thinking off or at a
fixed budget (thinking otherwise dominates TTFT on Holo, max budget 768).
@rsafier rsafier force-pushed the test/bench-thinking-toggle branch from f7c1649 to 55c5517 Compare July 13, 2026 00:43
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.

2 participants