Skip to content

[misc] fix: Cap server requests by token budget - #5594

Open
yaoyu-33 wants to merge 1 commit into
mainfrom
fix/inference-server-token-budget
Open

[misc] fix: Cap server requests by token budget#5594
yaoyu-33 wants to merge 1 commit into
mainfrom
fix/inference-server-token-budget

Conversation

@yaoyu-33

Copy link
Copy Markdown
Contributor

What changed

Cap OpenAI-server request capacity at an explicit active-token budget while preserving tensor-parallel divisibility. When neither --max_batch_size nor a prompt count supplies a request limit, Bridge now derives a request ceiling only if the user explicitly set --max_tokens; otherwise the existing KV-memory auto-sizing path remains unchanged.

Supported trigger and user impact

The maintained OpenAI-server example supports Qwen/Qwen2.5-1.5B and exposes --max_tokens. With --max_tokens 128 and the documented unset --max_batch_size, Bridge passed max_requests=None into pinned MCore. MCore independently auto-sized 2,924 requests from the default 20 GiB KV buffer, then aborted context construction because 128 active tokens cannot support 2,924 active requests. The HTTP server therefore failed before binding its port.

This is the server-side omitted sibling of the prompt-derived capacity invariant fixed in #5577.

Root cause and minimal fix

build_inference_config() reconciled explicit and prompt-derived request counts with the effective token budget, but skipped the cap when both request inputs were unset. The shared builder is the owning Bridge-to-MCore boundary. It now selects the largest TP-divisible request ceiling within an explicitly supplied token budget. No-limit server startup still leaves max_requests=None for KV-based sizing.

Regression evidence

Focused contract test:

uv run --no-sync python -m pytest --confcutdir=tests/unit_tests/scripts tests/unit_tests/scripts/test_text_generation.py::test_build_inference_config_caps_auto_sized_server_requests_at_token_budget -q
  • Before: 1 failed because max_requests remained None under an explicit 128-token server budget.
  • After: 1 passed; request capacity is non-null, TP-divisible, and no greater than 128.

Adjacent validation:

uv run --no-sync python -m pytest --confcutdir=tests/unit_tests/scripts tests/unit_tests/scripts/test_text_generation.py -k build_inference_config -q
# 8 passed, 12 deselected

uv run --no-sync python -m pytest --noconftest tests/unit_tests/scripts/test_openai_server_entrypoint.py tests/unit_tests/scripts/test_async_text_generation_entrypoint.py tests/unit_tests/scripts/test_text_generation_entrypoint.py -q
# 9 passed

uv run --no-sync pre-commit run --all-files
# passed

git diff --check
# passed

A CPU-only dependency preload was used locally to isolate the real shared helper from unrelated optional CUDA import initialization; no MCore behavior was mocked in the request/token contract itself.

Scope

  • No public API or CLI signature changes.
  • No dependency, workflow, lockfile, or MCore submodule changes.
  • Default server KV auto-sizing is preserved when --max_tokens is unset.
  • No GPU execution, model-quality, convergence, performance, or full-suite validation is claimed.

Signed-off-by: Yu Yao <yaoyu.094@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@yaoyu-33 yaoyu-33 added area:misc Cross-cutting utilities, logging, helpers, and other changes bug Something isn't working needs-review PR is ready for code review and waiting on a reviewer labels Aug 15, 2026
@yaoyu-33

Copy link
Copy Markdown
Contributor Author

/ok to test deb4cc2

@claude

claude Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Light review of the build_inference_config change.

The change adds a server auto-sizing path: when both max_batch_size and num_prompts are None but an explicit --max_tokens is given, max_requests is now derived from the token budget (floored to a multiple of tp) instead of being left None for the KV-cache engine to size. Logic is correct and the docstring was updated to match.

Findings:

  • The new guard if max_requests == 0: raise ValueError(...) at text_generation.py:350-351 (server auto-size path, max_tokens < tp) is not exercised by any test. Both happy-path server cases are covered, but this rejection branch is not.
  • No correctness or typo issues otherwise. The // tp * tp flooring matches the existing capping branch, and the max_tokens is None fall-through is guarded by test_build_inference_config_preserves_kv_auto_sizing_without_token_limit.

Suggested test cases (no perf configs touched; unit-level):

  • test_build_inference_config_caps_auto_sized_server_requests_at_token_budget
  • test_build_inference_config_preserves_kv_auto_sizing_without_token_limit
  • test_build_inference_config_caps_default_requests_at_token_budget
  • test_build_inference_config_rejects_explicit_batch_above_token_budget
  • (new) server auto-size rejection: max_batch_size=None, num_prompts=None, tp=2, max_tokens=1 -> ValueError '--max_tokens (1) must be at least --tp (2).'


assert config.kwargs["max_requests"] is not None
assert config.kwargs["max_requests"] <= 128
assert config.kwargs["max_requests"] % 2 == 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new server auto-size path adds a raise ValueError(... must be at least --tp ...) when max_tokens // tp * tp == 0 (both max_batch_size and num_prompts None, max_tokens < tp). That branch (text_generation.py:350-351) is not covered by any test. Consider adding a case, e.g. max_batch_size=None, num_prompts=None, tp=2, max_tokens=1, asserting the ValueError is raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:misc Cross-cutting utilities, logging, helpers, and other changes bug Something isn't working needs-review PR is ready for code review and waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant