[danielbanu]

What

Adds DEFAULT_MAX_NEW_TOKENS (default 65536, empty = old behavior) - a server-side default for requests that omit max_tokens, installed via --override-generation-config {"max_new_tokens": N} on both head and worker inner scripts (HF generation_config.json semantics: max_new_tokens == vLLM max_tokens; applies to both TP ranks).

Why

The OpenAI layer backs an omitted max_tokens with max_model_len - prompt (~1M on the default recipe). One client that never sets max_tokens can decode effectively unbounded and grow KV until it preempts every other session on a shared server. The bound is decode hygiene; explicit client max_tokens always wins, and per-request overrides are unaffected.

Scope note (measured, not assumed)

Admission in this vLLM build is chunk-based and not gated on max_tokens - we verified concurrent admission with ~1M output-budget claims. Long-context concurrency is governed by the effective KV pool (nominal 1.41M tokens; a 258K-token request reads ~31% kv_cache_usage_perc -> ~830K effective with mamba/SWA/draft overhead) and by chunked-prefill token budgets. Full data and analysis in #43 (comment with measurements from a second 2x GB10 kit, including the reason="capacity" catch-all-label finding).

Changes

• start.sh: DEFAULT_MAX_NEW_TOKENS default + --override-generation-config wiring on both ranks; passed via head -e block and worker serve_env loop; on_ready logs mt_default=
• .env.example: documented block
• README.md: env-var table row with scope note
• tests/test_gen_defaults.py: static-anchor regression (default bounded, flag on both ranks, env passthrough on head + worker, docs present)

Verified live on 2x GB10 TP=2: boot log shows Default vLLM sampling parameters ... overridden ... {'max_tokens': 65536}; /health, chat completions and metrics normal after restart.

[plotarmordev]

Reviewed 187eb02a1edbfba02d2022759ceb4d87d783d9d9. Both-rank forwarding is present, but the advertised contract and configuration handling need corrections before approval.

1. The implementation is a hard cap, not an omitted-only default. start.sh:1012,1121 uses --override-generation-config {"max_new_tokens": ...}. In the Dockerfile-pinned image's digest-verified vLLM-install-layer source, chat serving assigns that value to override_max_tokens, and get_max_tokens includes it in the minimum even when the client supplies an explicit limit. Executing the extracted limiter with ample remaining context gives 65536 for both omitted max_tokens and explicit max_tokens=200000. This contradicts .env.example:197 and README.md:655 (explicit client limits win). Please implement the stated omitted-only contract, or separately propose/agreedly scope a hard-cap feature; do not silently change the promised semantics by relabeling it.

2. Empty does not disable it. start.sh:166 uses ${DEFAULT_MAX_NEW_TOKENS:-65536}, which maps explicit empty to 65536. Reproduced in bash. Preserve explicit empty if retaining the documented opt-out (unset-only default expansion), including caller overrides over .env.

3. Validate/canonicalize before lifecycle actions. The new field is not checked by validate_numeric_config. Executing that guard with nope, 001 and -1 returns success; the first two then become invalid JSON at the argument-construction sites. Use the existing optional-positive-integer validation conventions and reject malformed settings before restart stops healthy containers. No live failure was induced for this review.

4. The new test fails on this head. python3 tests/test_gen_defaults.py fails at line 34 because the expected contiguous source substring no longer exists. Please replace the wording/wiring assertions with behavioral checks for empty opt-out, numeric serialization, pre-stop rejection, and the distinction between omitted and explicit request limits. Merely repinning the source substring would miss the defects above.

Please also remove/split the unrelated template and benchmark-auth hunks; the auth work overlaps #136. The template's string-arguments-to-{} fallback loses arguments in direct rendering, but I verified that the pinned vLLM preprocessing normalizes valid JSON argument strings into dictionaries before rendering, so this is not a demonstrated normal OpenAI-serving failure. It does not belong in this token-default change. Clarify the two-node-only scope if start-tp4.sh remains unwired.

Review scope: static source review plus isolated CPU reproductions, including extracted functions from the digest-verified runtime install layer. No model requests, GPU qualification, deployment changes or full image boot.

[plotarmordev]

Reviewed and merged — #51

Normally merged as b3bd1f692893 after publishing the source-only fixes to this PR. Reviewed merge head: 7c6f5e43e27a331b7435bec55775d5c44b497850, refreshed onto main 4e023d0a150a5c51e3750b124bbebcad5e51fbbd. My previous changes-requested review is superseded by an exact-head approval.

Exact-head CPU checks passed: token-default/Pydantic behavioral regressions, numeric configuration, launcher rank parity and pre-stop ordering, caller overrides, and shell syntax. No hosted checks were configured/reported. Scope remains the two-node launcher; this does not claim TP3/TP4 functional coverage or performance improvement.

Historical live test evidence: the unchanged token-default implementation at local candidate aeb585f0d0ce9ca3596d061048200f1e98ac0ede passed 72/72 checks: 66 successful generations plus 6 expected HTTP 400 context-overflow rejections, 1,328 completion tokens. Covered JSON/SSE chat/completions, omitted/explicit/null limits, precedence, independent caps, and context bounds. Live testing exposed the Pydantic before-validator manufacturing an explicit field for omission; the published fix preserves omission while retaining explicit-null stock behavior. The initial failed run remains retained.

These live receipts predate the latest main refresh; no new GPU run on the merge head is claimed. The scope is functional semantics, not a TheGrill performance approval. No deployment rollout or service changes were made for this merge.

[plotarmordev]

Reviewed the updated source-only head 7c6f5e43e27a331b7435bec55775d5c44b497850, incorporating current main 4e023d0a150a5c51e3750b124bbebcad5e51fbbd. The previous requested changes are addressed: true omitted-only semantics without weakening independent caps; explicit-empty opt-out; canonical numeric validation before lifecycle actions; behavioral regressions instead of source-substring assertions; unrelated template/auth changes absent from the current PR diff.

Exact-head CPU checks PASS: token-default/Pydantic behavioral regressions, numeric configuration, launcher rank parity/pre-stop ordering, caller overrides, and shell syntax. Prior bounded live evidence for the unchanged token-default implementation: 72/72 checks, 66 successful generations and 6 expected context-overflow HTTP 400 responses. Those live receipts belong to candidate aeb585f, before the latest main refresh; no claim of a fresh GPU run on this merge head.

Scope remains the two-node start.sh path, not new TP3/TP4 coverage or performance approval. Source-only scope and focused worktree/commit-range credential scans passed. No configured hosted checks are reported for this PR. Approving the addressed functional change for the owner's requested normal merge.