feat(mocker): add explicit max_model_len support to vLLM mocker#11069
Conversation
dreamtalen
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
WalkthroughAdds an optional Changesmax_model_len prompt-length enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/src/dynamo/mocker/utils/kv_cache.py (1)
40-46: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse lazy logging for this warning.
logger.warning("Could not determine max context length from model config: %s", e)avoids eager formatting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/mocker/utils/kv_cache.py` around lines 40 - 46, The warning in the max context length lookup uses eager f-string formatting, so update the exception handling in the helper around get_model_info to use lazy logger formatting instead. In the function that returns max_context_length, keep the same message context but pass the exception as an argument to logger.warning rather than interpolating it into the string directly.Sources: Coding guidelines, Path instructions, Linters/SAST tools
components/src/dynamo/mocker/tests/unit/test_config.py (1)
411-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove these imports to module scope.
Lines 412, 424, 464, 483, and 502 import regular test dependencies inside the test bodies. These are not optional-dependency collection guards, so they should live at the top of the file instead of hiding dependencies inside individual tests. As per coding guidelines, "Keep imports at the top of the file; always flag import statements inside function bodies." As per path instructions,
.ai/python-guidelines.mdsays to "keep all imports at the top of the file (no in-function/class imports)."Also applies to: 463-519
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/src/dynamo/mocker/tests/unit/test_config.py` around lines 411 - 431, The test file has several regular dependency imports inside individual test bodies, which should be moved to module scope. Update the test module by hoisting the imports used by these tests out of the functions and placing them with the other top-level imports, then remove the in-test import statements from the affected tests such as test_model_max_context_length_uses_shared_model_info and test_model_max_context_length_returns_none_on_failure. Keep the tests using the same symbols (for example kv_cache, SimpleNamespace, and any other shared fixtures/helpers) but ensure all imports are declared once at the top of the file.Sources: Coding guidelines, Path instructions
lib/mocker/src/scheduler/vllm/core.rs (1)
1401-1422: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the max-model-length gate in
policy.rs. This vLLM-only check still sits incore.rs, which splits the admission rules across files. Fold it into the policy-owned admission helper so all vLLM waiting admission paths go through one gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/mocker/src/scheduler/vllm/core.rs` around lines 1401 - 1422, Move the vLLM max-model-length rejection out of the FreshKv branch in core.rs and into the policy-owned waiting admission logic so all vLLM waiting admission decisions flow through one place. Update the helper around policy::decide_waiting_admission to perform the prompt_exceeds_max_model_len check internally when scheduling_policy is SchedulingPolicy::Vllm, and keep AdmissionStage::FreshKv in core.rs focused on delegating to policy.rs. Ensure the existing behavior remains unchanged for non-vLLM policies while consolidating the gate in policy.rs.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/src/dynamo/mocker/args.py`:
- Around line 214-220: The --max-model-len CLI option in the argument parser
currently accepts 0 and negative values, but MockEngineArgs only supports values
>= 1, so validate it during parsing instead of deferring the failure. Update the
parser setup in args.py where add_argument for --max-model-len is defined to
enforce a minimum of 1, using the existing argument parsing flow so invalid
input is rejected immediately. Keep the behavior for omitted values unchanged
and make sure the validation happens before MockEngineArgs construction.
---
Nitpick comments:
In `@components/src/dynamo/mocker/tests/unit/test_config.py`:
- Around line 411-431: The test file has several regular dependency imports
inside individual test bodies, which should be moved to module scope. Update the
test module by hoisting the imports used by these tests out of the functions and
placing them with the other top-level imports, then remove the in-test import
statements from the affected tests such as
test_model_max_context_length_uses_shared_model_info and
test_model_max_context_length_returns_none_on_failure. Keep the tests using the
same symbols (for example kv_cache, SimpleNamespace, and any other shared
fixtures/helpers) but ensure all imports are declared once at the top of the
file.
In `@components/src/dynamo/mocker/utils/kv_cache.py`:
- Around line 40-46: The warning in the max context length lookup uses eager
f-string formatting, so update the exception handling in the helper around
get_model_info to use lazy logger formatting instead. In the function that
returns max_context_length, keep the same message context but pass the exception
as an argument to logger.warning rather than interpolating it into the string
directly.
In `@lib/mocker/src/scheduler/vllm/core.rs`:
- Around line 1401-1422: Move the vLLM max-model-length rejection out of the
FreshKv branch in core.rs and into the policy-owned waiting admission logic so
all vLLM waiting admission decisions flow through one place. Update the helper
around policy::decide_waiting_admission to perform the
prompt_exceeds_max_model_len check internally when scheduling_policy is
SchedulingPolicy::Vllm, and keep AdmissionStage::FreshKv in core.rs focused on
delegating to policy.rs. Ensure the existing behavior remains unchanged for
non-vLLM policies while consolidating the gate in policy.rs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75d2c6ff-84e8-45f6-a771-7f2a1f9983b4
📒 Files selected for processing (11)
components/src/dynamo/mocker/args.pycomponents/src/dynamo/mocker/config.pycomponents/src/dynamo/mocker/tests/unit/test_config.pycomponents/src/dynamo/mocker/utils/kv_cache.pycomponents/src/dynamo/replay/main.pylib/bindings/python/rust/llm/replay.rslib/bindings/python/src/dynamo/_core.pyilib/mocker/src/common/protocols.rslib/mocker/src/scheduler/vllm/core.rslib/mocker/src/scheduler/vllm/policy.rslib/mocker/src/scheduler/vllm/policy/tests.rs
dreamtalen
force-pushed
the
yongmingd/mocker-max-model-len-reject
branch
from
3121b8a to
116822b
Compare
This comment has been minimized.
This comment has been minimized.
dreamtalen
force-pushed
the
yongmingd/mocker-max-model-len-reject
branch
2 times, most recently
from
aa4b293 to
660a386
Compare
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
There was a problem hiding this comment.
One caveat: this is based on a brief review and my current understanding of how these flows should work. Please double-check these conclusions against the upstream vLLM source and the semantics Dynamo intends to expose; semantic correctness for this integration should take priority if they differ.
I think the scalar plumbing is useful, but this needs one consistent semantic flow before merge:
requested max_model_len
→ backend-specific resolution
→ effective max_model_len
→ scalar passed to and enforced by mocker
The architectural context length is only an input to backend resolution; it is not necessarily the effective runtime limit. Physical max_kv_tokens is a separate worker-capacity concept. lib/mocker should remain scalar-only and must not acquire LLM/backend/runtime resolver dependencies.
At request level, max_model_len is a total prompt-plus-output limit. The request’s original max_output_tokens should remain unchanged, with generation stopping independently when either the requested output limit or effective max_model_len is reached.
A prompt with no output position remaining should be rejected before generation. Reaching the context limit after generation starts is instead a successful length-capped completion. No new public finish reason or replay terminal status is required: both output-limit and context-limit completion map to finish_reason="length", while replay records Completed.
Only an effective limit genuinely enforced by the running engine should be published as ModelRuntimeConfig.context_length. For a focused version of this PR, I suggest keeping the new scalar plumbing, accepting an explicitly resolved max_model_len, enforcing it end to end, and deferring automatic derivation until there is a canonical backend-compatible resolver.
That end-to-end enforcement also needs to cover the existing consumers: live mocker must not treat context-capped completion as an early-completion error, and replay must distinguish the requested output length from the number of tokens actually emitted.
PeaBrane
dismissed
their stale review
Converting this review to non-blocking feedback at the reviewer's request.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Yongming Ding <yongmingd@nvidia.com>
dreamtalen
force-pushed
the
yongmingd/mocker-max-model-len-reject
branch
from
660a386 to
162b60b
Compare
dreamtalen
changed the title
PeaBrane
left a comment
PeaBrane
left a comment
There was a problem hiding this comment.
Rechecked the updated head against the prior top-level architectural review and inline comments. The explicit-only derivation, total-sequence enforcement, live length completion, disaggregated validation, and replay accounting concerns are addressed.
|
/ok to test 162b60b |
2 participants
Summary
Add explicit
max_model_lensupport to vLLM mocker and replay so they model the same total sequence-length behavior as Dynamo frontend/router plus a vLLM worker.Requests with no output position remaining are rejected before generation. Requests that reach the context limit after generation starts complete normally with
finish_reason="length".Details
max_model_lenscalar across the CLI, Python bindings, JSON engine args, and Rust mock engine args.prompt_len >= max_model_len.max_model_len.lengthfinish reason instead of treating them as early-completion errors.requested_output_lengthin replay while deriving actualoutput_lengthfrom emitted token signals.ModelRuntimeConfig.context_lengthonly when an explicit limit is enforced, while keeping physicalmax_kv_tokensseparate.Where should the reviewer start?
lib/mocker/src/scheduler/vllm/policy.rsandlib/mocker/src/scheduler/vllm/core.rsfor admission and generation-stop semantics.lib/llm/src/mocker.rsfor live context-capped completion handling.lib/mocker/src/replay/collector.rsandlib/mocker/src/replay/offline/for requested-versus-actual output accounting.components/src/dynamo/mocker/config.pyandcomponents/src/dynamo/replay/main.pyfor explicit scalar propagation and runtime capability publication.Related Issues
This PR is NOT linked to an issue:
Validation
cargo fmt --all -- --checkcargo clippy -p dynamo-mocker --lib -- -D warningscargo test -p dynamo-mocker(488 passed,1 ignored)dynamo/bin/pytest -q components/src/dynamo/mocker/tests/unit/test_config.py -k max_model_len(7 passed,27 deselected)Real-GPU validation on an H100 with Dynamo frontend + KV router + vLLM worker:
nvcr.io/nvidia/ai-dynamo/vllm-runtime:1.3.0-dev.1-cuda131.3.0.dev10.22.0Qwen/Qwen3-0.6Bmax_model_len:32lengthlength