Add vLLM token-native generate endpoint#1113
Conversation
Signed-off-by: Biswa Panda <biswa.panda@gmail.com>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@f30d835ff995caa665e75c12c3d266e348b9bfd3Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@f30d835ff995caa665e75c12c3d266e348b9bfd3Last updated for commit: |
WalkthroughThis PR adds a new ChangesvLLM Generate Endpoint
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
🚥 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.
🧹 Nitpick comments (1)
src/aiperf/endpoints/vllm_generate.py (1)
98-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated token-id validation logic vs.
_valid_token_ids.The int/bool type-check here mirrors
_valid_token_ids(lines 134-142) but is copy-pasted instead of reused (only difference is the non-empty check, since an empty completion should stay valid). Extract a shared helper for the type-check portion to avoid the two checks drifting apart.♻️ Proposed refactor
`@staticmethod` - def _valid_token_ids(value: Any) -> bool: - return ( - isinstance(value, list) - and bool(value) - and all( - isinstance(token_id, int) and not isinstance(token_id, bool) - for token_id in value - ) - ) + def _is_int_list(value: Any) -> bool: + return isinstance(value, list) and all( + isinstance(token_id, int) and not isinstance(token_id, bool) + for token_id in value + ) + + `@classmethod` + def _valid_token_ids(cls, value: Any) -> bool: + return bool(value) and cls._is_int_list(value)Then in
_parse_response, replace lines 99-103 withif not self._is_int_list(completion_ids): return None.🤖 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 `@src/aiperf/endpoints/vllm_generate.py` around lines 98 - 103, There is duplicated token-id type validation in _parse_response that mirrors the logic already present in _valid_token_ids, which risks the two checks drifting apart. Extract the shared int/bool list validation into a helper on the same class (for example, a reusable list-type check used by both _parse_response and _valid_token_ids), then update _parse_response to call that helper while preserving the separate empty-list behavior for completions.
🤖 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.
Nitpick comments:
In `@src/aiperf/endpoints/vllm_generate.py`:
- Around line 98-103: There is duplicated token-id type validation in
_parse_response that mirrors the logic already present in _valid_token_ids,
which risks the two checks drifting apart. Extract the shared int/bool list
validation into a helper on the same class (for example, a reusable list-type
check used by both _parse_response and _valid_token_ids), then update
_parse_response to call that helper while preserving the separate empty-list
behavior for completions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a6e5e673-f94e-473c-a570-270bf92e95d8
📒 Files selected for processing (4)
src/aiperf/endpoints/vllm_generate.pysrc/aiperf/plugin/plugins.yamltests/unit/common/enums/test_endpoints_enums.pytests/unit/endpoints/test_vllm_generate.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
biswapanda
changed the title
biswapanda
requested review from
FrankD412,
ajcasagrande,
debermudez,
ganeshku1,
ilana-n,
lkomali and
matthewkotila
as code owners
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| } | ||
| return ParsedResponse( | ||
| perf_ns=response.perf_ns, | ||
| data=BaseResponseData(), |
There was a problem hiding this comment.
This endpoint returns only empty BaseResponseData while use_server_token_count defaults to false, so the default parser path tokenizes an empty output instead of the reconstructed usage and drops token-throughput metrics. Fix: force server token-count parsing for vllm_generate or for token-native endpoints with produces_tokens=true and tokenizes_input=false.
🤖 AI Fix
Update src/aiperf/records/inference_result_parser.py in InferenceResultParser so endpoints with metadata produces_tokens=True and tokenizes_input=False use _compute_server_token_counts() by default, and add a unit test covering VllmGenerateEndpoint token counts without setting endpoint.use_server_token_count.
| or not isinstance(choices[0], dict) | ||
| ): | ||
| return None | ||
| completion_ids = choices[0].get("token_ids") |
There was a problem hiding this comment.
_parse_response only reads choices[0] even though sampling_params can request multiple choices, so n > 1 responses undercount generated tokens. Fix: reject sampling_params.n != 1 or aggregate token counts across every choice.
🤖 AI Fix
In src/aiperf/endpoints/vllm_generate.py VllmGenerateEndpoint.format_payload, validate sampling_params.get("n", 1) == 1 and raise ValueError otherwise, or update _parse_response() to validate and sum token_ids across all choices.
| isinstance(value, list) | ||
| and bool(value) | ||
| and all( | ||
| isinstance(token_id, int) and not isinstance(token_id, bool) |
There was a problem hiding this comment.
_valid_token_ids accepts negative integers even though vLLM rejects negative token_ids, so invalid datasets fail only after dispatch as server errors. Fix: require every token id to be non-negative during local validation.
🤖 AI Fix
Update src/aiperf/endpoints/vllm_generate.py VllmGenerateEndpoint._valid_token_ids() to include token_id >= 0, update the validation error text in format_payload(), and add a unit test rejecting negative token ids.
|
Hi @biswapanda, thanks for the PR. We can add incremental support if it is easier, but I think there is more involved to get the ideal native solution. I think ideally this is how I would want aiperf to support something like this:
I made a proof of concept of some of this a while back. It may be worth taking a look at it for inspiration: |
2 participants
Summary
Add a non-streaming AIPerf endpoint adapter for the canonical vLLM token-in/token-out API:
POST /inference/v1/generate
The adapter lets the same pretokenized request payload run against vLLM, Dynamo, or another server implementing the vLLM Generate contract. It exercises exact token IDs rather than chat messages and does not execute or emulate tools.
Changes
This PR intentionally contains only the endpoint adapter. The earlier local frozen-trace commits are excluded because Exgentic replay support has since landed upstream through #1064 and #1078.
Validation
Validation on the exact publication head:
Provenance
Related work
Summary by CodeRabbit
New Features
Bug Fixes