test: Remove completions endpoint from vllm tests.#1575
Conversation
Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com>
Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com>
WalkthroughThe test configurations in Changes
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/serve/test_dynamo_serve.py (2)
28-28: Consider cleanup: Completions response handler import may be underutilized.After removing completions endpoints from several VLLM deployment graphs, the
completions_response_handlerimport is now only used by sglang, vllm_v1, and tensorrt_llm deployments. While not incorrect, this reduces the utility of the import.
72-78: Consider cleanup: Completions payload definition now unused by modified deployment graphs.The
payload_completionsportion oftext_payloadis no longer used by the VLLM deployment graphs that were modified (agg, disagg, agg_router, disagg_router, multimodal_agg), though it's still needed for other deployment types.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/serve/test_dynamo_serve.py(5 hunks)
🔇 Additional comments (6)
tests/serve/test_dynamo_serve.py (6)
90-93: LGTM: Completions endpoint correctly removed for VLLM agg deployment.The change aligns with the PR objective to remove failing completions endpoints from VLLM tests. The endpoints and response_handlers lists remain properly synchronized.
117-120: LGTM: Completions endpoint correctly removed for VLLM disagg deployment.Consistent with the agg deployment change, maintaining proper alignment between endpoints and handlers.
130-133: LGTM: Completions endpoint correctly removed for VLLM agg_router deployment.The change maintains consistency across VLLM deployment variants.
147-150: LGTM: Completions endpoint correctly removed for VLLM disagg_router deployment.Consistent application of the completions endpoint removal for VLLM deployments.
164-167: LGTM: Completions endpoint correctly removed for multimodal VLLM deployment.The multimodal deployment configuration correctly follows the same pattern as other VLLM deployments.
84-250: Verify selective application aligns with deployment capabilities.The changes target only VLLM deployment graphs while preserving completions endpoints for sglang, vllm_v1, and tensorrt_llm deployments. Ensure this selective approach matches the actual endpoint support capabilities of these different LLM backends.
#!/bin/bash # Description: Verify that the deployment graphs still retaining completions endpoints actually support them # Expected: Find evidence that sglang, vllm_v1, and tensorrt_llm configs support completions endpoint echo "=== Checking configuration files for completions endpoint support ===" echo "VLLM configs (should not mention completions after this change):" fd -e yaml . "/workspace/examples/llm/configs" --exec cat {} \; 2>/dev/null || echo "VLLM configs not found" echo -e "\nSGLang configs (should support completions):" fd -e yaml . "/workspace/examples/sglang/configs" --exec cat {} \; 2>/dev/null || echo "SGLang configs not found" echo -e "\nVLLM v1 configs (should support completions):" fd -e yaml . "/workspace/examples/vllm_v1/configs" --exec cat {} \; 2>/dev/null || echo "VLLM v1 configs not found" echo -e "\nTensorRT-LLM configs (should support completions):" fd -e yaml . "/workspace/examples/tensorrt_llm/configs" --exec cat {} \; 2>/dev/null || echo "TensorRT-LLM configs not found"
Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com>
Overview:
Removing the completions/ endpoint from llm tests. The completions/ endpoint are expected to fail and the solution is to use dynamo run for ingress and registering endpoints.
TRTLLM, VLLM v0 and vLLM v1 are using dynamo run for ingress.
Summary by CodeRabbit