[https://nvbugs/6414760][fix] Rewrite the test with the PyTorch LLM API (LLM + SamplingParams(n=4…#16014
[https://nvbugs/6414760][fix] Rewrite the test with the PyTorch LLM API (LLM + SamplingParams(n=4…#16014trtllm-agent wants to merge 1 commit into
Conversation
…LLM API Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
📝 WalkthroughWalkthroughThe test test_qwen_e2e_cpprunner_large_new_tokens in test_e2e.py was rewritten to use tensorrt_llm.LLM and SamplingParams for generation instead of convert_weights, trtllm-build, and ModelRunnerCpp. The unused convert_weights import was removed and the test signature simplified. ChangesQwen e2e test rewrite
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/defs/test_e2e.py (1)
75-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
llm_venvfixture is now unused.The rewritten test runs generation in-process via
LLM, so thellm_venvfixture is no longer exercised. Requesting it still triggers workspace creation and teardown (seellm_venvinconftest.py) for no purpose. Consider dropping it from the signature if parametrization/markers don't depend on it. Note:model_nameshould stay if it's supplied by a@pytest.mark.parametrizedecorator (lines 31-74, not shown).🤖 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 `@tests/integration/defs/test_e2e.py` around lines 75 - 92, The test test_qwen_e2e_cpprunner_large_new_tokens no longer uses the llm_venv fixture because generation now runs directly through LLM in-process. Remove llm_venv from the test function signature so pytest does not create and tear down the unused workspace, and keep model_name only if it is still needed by the existing parametrization in this test module.
🤖 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 `@tests/integration/defs/test_e2e.py`:
- Around line 75-92: The test test_qwen_e2e_cpprunner_large_new_tokens no longer
uses the llm_venv fixture because generation now runs directly through LLM
in-process. Remove llm_venv from the test function signature so pytest does not
create and tear down the unused workspace, and keep model_name only if it is
still needed by the existing parametrization in this test module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2c9155d-d12d-4cb0-b09b-7190fe8eea68
📒 Files selected for processing (1)
tests/integration/defs/test_e2e.py
|
I will include the fix in #15907 to save CI resources. |
Summary
Test plan
Links
Summary by CodeRabbit