[https://nvbugs/6417978][fix] Mirror the medusa/whisper precedent: replace test_gptj's body with an…#16007
[https://nvbugs/6417978][fix] Mirror the medusa/whisper precedent: replace test_gptj's body with an…#16007trtllm-agent wants to merge 1 commit into
Conversation
PR NVIDIA#15763 removed examples/models/contrib/gptj/ as part of the legacy TensorRT-backend cleanup, but tests/integration/defs/triton_server/build_model.sh still ran 'pushd examples/models/contrib/gptj' for the gptj target, causing the Triton smoke test to fail with 'pushd: examples/models/contrib/gptj: No such file or directory'. Replace the body of test_gptj with a lightweight assertion that examples/models/contrib/gptj/ remains absent under llm_root. This keeps the pytest node id resolvable (the harness runs the test by exact node id) while removing the broken build/test.sh invocation. Mirrors the fixes for test_medusa (b08ce26) and test_whisper (b325280). The dead 'gptj' branches in build_model.sh and test.sh are no longer reachable and can be pruned in a follow-up cleanup. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
📝 WalkthroughWalkthroughThe test_gptj function in tests/integration/defs/triton_server/test_triton.py was changed from building and running a GPT-J Triton server test to a guard assertion verifying that the legacy GPT-J example directory does not exist, with model_path and engine_dir parameters removed. ChangesGPT-J Triton Test Update
Estimated code review effort: 1 (Trivial) | ~5 minutes Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 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/triton_server/test_triton.py (1)
135-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
tritonserver_test_rootfixture.test_nameis required by the indirect parametrization, buttritonserver_test_rootisn’t referenced here and can be removed unless it’s needed for fixture setup.🤖 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/triton_server/test_triton.py` at line 135, Remove the unused tritonserver_test_root fixture from test_gptj in the triton server integration test, since the function does not reference it and only test_name is required by the indirect parametrization. Keep the remaining fixtures that are actually used, and if tritonserver_test_root was only present for setup, move that setup into the appropriate fixture or autouse fixture instead of the test signature.
🤖 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/triton_server/test_triton.py`:
- Line 135: Remove the unused tritonserver_test_root fixture from test_gptj in
the triton server integration test, since the function does not reference it and
only test_name is required by the indirect parametrization. Keep the remaining
fixtures that are actually used, and if tritonserver_test_root was only present
for setup, move that setup into the appropriate fixture or autouse fixture
instead of the test signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c37c8685-34d4-492a-961c-f1418831cf15
📒 Files selected for processing (1)
tests/integration/defs/triton_server/test_triton.py
Summary
pushd examples/models/contrib/gptjfor the gptj target, soset -epropagates the failure to pytest.os.path.existsassertion onexamples/models/contrib/gptj/and drop the now-unused model_path/engine_dir fixture params.Test plan
Links
Summary by CodeRabbit