[https://nvbugs/5546507][test] Remove obsolet… - #14995
Conversation
…e eagle modelopt and phi lora tests Remove test_llm_eagle_1gpu_modelopt_ckpt (waived under nvbug 5546507) and the unused legacy test_llm_phi_lora_1gpu, along with their test-list entries, waive, and now-orphaned imports. Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoves two integration tests (Eagle modelopt and Phi LoRA), updates copyright headers and imports in test modules, and removes the Eagle test from test lists and waive records. ChangesIntegration test removal and cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
xinhe-nv
changed the title
|
/bot run --stage-list "" |
|
PR_Github #52280 [ run ] triggered by Bot. Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/integration/defs/examples/test_phi.py (1)
53-133: Test coverage note after removal oftest_llm_phi_lora_1gpu.The removal eliminates the TensorRT-LLM engine-based LoRA test for Phi models. Coverage status:
- ✓ Quantization (FP8):
test_llm_phi_quantization_1gpucovers multiple Phi models⚠️ LoRA: Onlytest_phi_4_mini_instruct_with_bf16_lora_torchremains, which uses torch backend and is currently skipped (line 111-112)If the removed test was truly obsolete per PR objectives, this coverage level may be intentional. Otherwise, consider whether Phi + LoRA with TRT engines needs coverage restoration outside this PR.
🤖 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/examples/test_phi.py` around lines 53 - 133, Removed test_llm_phi_lora_1gpu eliminated TRT-engine LoRA coverage for Phi models; restore or add a test that exercises Phi + LoRA using the TensorRT-LLM engine. Reintroduce a test (or create one) modeled after the removed test name test_llm_phi_lora_1gpu that runs conversion/quantization (see test_llm_phi_quantization_1gpu), builds TRT engines (build_cmd / check_call usage), then loads the engine and applies multiple LoRAs to verify inference accuracy; alternatively, add a new parametrized test that complements test_phi_4_mini_instruct_with_bf16_lora_torch but uses the TRT backend and engine_dir to ensure TRT LoRA coverage. Ensure the test invokes the same helpers (venv_check_call, check_call, and engine_dir handling) and is not skipped so CI covers Phi + LoRA with TensorRT engines.tests/integration/defs/examples/test_eagle.py (1)
28-96: Test coverage note after removal oftest_llm_eagle_1gpu_modelopt_ckpt.The remaining
test_llm_eagle_1gputest provides baseline Eagle speculative decoding coverage across dynamic_tree modes and batch sizes. Since the removed test was waived (nvbug 5546507), the current coverage appears sufficient for this integration test module.🤖 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/examples/test_eagle.py` around lines 28 - 96, Add a brief inline note above the test_llm_eagle_1gpu function stating that test_llm_eagle_1gpu now provides the baseline Eagle speculative-decoding coverage after removal of test_llm_eagle_1gpu_modelopt_ckpt, so reviewers know the removal was intentional; locate the test by the function name test_llm_eagle_1gpu and add the comment near its `@pytest.mark.parametrize` decorators (no code logic changes required).
🤖 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/examples/test_eagle.py`:
- Around line 28-96: Add a brief inline note above the test_llm_eagle_1gpu
function stating that test_llm_eagle_1gpu now provides the baseline Eagle
speculative-decoding coverage after removal of
test_llm_eagle_1gpu_modelopt_ckpt, so reviewers know the removal was
intentional; locate the test by the function name test_llm_eagle_1gpu and add
the comment near its `@pytest.mark.parametrize` decorators (no code logic changes
required).
In `@tests/integration/defs/examples/test_phi.py`:
- Around line 53-133: Removed test_llm_phi_lora_1gpu eliminated TRT-engine LoRA
coverage for Phi models; restore or add a test that exercises Phi + LoRA using
the TensorRT-LLM engine. Reintroduce a test (or create one) modeled after the
removed test name test_llm_phi_lora_1gpu that runs conversion/quantization (see
test_llm_phi_quantization_1gpu), builds TRT engines (build_cmd / check_call
usage), then loads the engine and applies multiple LoRAs to verify inference
accuracy; alternatively, add a new parametrized test that complements
test_phi_4_mini_instruct_with_bf16_lora_torch but uses the TRT backend and
engine_dir to ensure TRT LoRA coverage. Ensure the test invokes the same helpers
(venv_check_call, check_call, and engine_dir handling) and is not skipped so CI
covers Phi + LoRA with TensorRT engines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7ee2c42a-f3b9-4d17-b289-de448a7532c4
📒 Files selected for processing (4)
tests/integration/defs/examples/test_eagle.pytests/integration/defs/examples/test_phi.pytests/integration/test_lists/test-db/l0_h100.ymltests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (2)
- tests/integration/test_lists/waives.txt
- tests/integration/test_lists/test-db/l0_h100.yml
|
PR_Github #91 [ run ] triggered by Bot. Commit: |
|
PR_Github #91 [ run ] completed with state |
|
PR_Github #52280 [ run ] completed with state |
|
/bot reuse-pipeline |
|
@coderabbitai why didn't you review the pr as a QA engineer? |
🧩 Analysis chain⚙️ CodeRabbit configuration inspected Length of output: 16673
Would you like me to kick off a full review right now? I'll apply the QA engineer instructions to all the 🐰 ears perk up sorry for the miss — on it now! ✅ Action performedReview finished.
|
|
PR_Github #52294 [ reuse-pipeline ] triggered by Bot. Commit: |
|
@coderabbitai why was the PR's first review missed or skipped? No actionable comments were generated in the recent review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain⚙️ CodeRabbit configuration inspected Length of output: 16673 🏁 Script executed: #!/bin/bash
# Get the PR details including timeline to understand review history
gh pr view 14995 --repo NVIDIA/TensorRT-LLM --json createdAt,commits,reviews,comments,state,baseRefName,headRefName
# Get PR review events
gh api /repos/NVIDIA/TensorRT-LLM/pulls/14995/reviewsLength of output: 37995
The first review was NOT missed. It fired automatically at
You can see that review here: PR Review Why the recent review had no actionable comments: When you asked me why the QA review didn't happen, I triggered an incremental review. Because So in summary: the QA-persona review did run on the full PR diff the first time around, the second run just had nothing new to review. 🐰 thumps foot apologetically — sorry for the confusion in my earlier response! |
|
/bot reuse-pipeline |
|
PR_Github #52294 [ reuse-pipeline ] completed with state |
|
PR_Github #52302 [ reuse-pipeline ] triggered by Bot. Commit: |
|
PR_Github #52302 [ reuse-pipeline ] completed with state |
NVIDIA#14995) Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com> Signed-off-by: NVFB <186336021+NVFB@users.noreply.github.com>
NVIDIA#14995) Signed-off-by: Xin He (SW-GPU) <200704525+xinhe-nv@users.noreply.github.com>
3 participants
…e eagle modelopt and phi lora tests
Remove test_llm_eagle_1gpu_modelopt_ckpt (waived under nvbug 5546507) and the unused legacy test_llm_phi_lora_1gpu, along with their test-list entries, waive, and now-orphaned imports.
Summary by CodeRabbit
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.