[https://nvbugs/6335726][fix] In test_qwen_moe_routed_expert_multi_lora_varying_ranks, drop ranks… - #15488
[https://nvbugs/6335726][fix] In test_qwen_moe_routed_expert_multi_lora_varying_ranks, drop ranks…#15488tensorrt-cicd wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe test ChangesMoE LoRA varying-ranks test fix and re-enablement
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unittest/llmapi/test_llm_pytorch.py (1)
1164-1164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale adapter count in docstring.
The docstring mentions "Five dummy adapters" but the implementation now uses three adapters with ranks
[8, 32, 64](changed at line 1198).📝 Proposed fix
- Five dummy adapters of varying rank target the routed experts (moe_h_to_4h, + Three dummy adapters of varying rank target the routed experts (moe_h_to_4h,🤖 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/unittest/llmapi/test_llm_pytorch.py` at line 1164, The docstring starting at line 1164 mentions "Five dummy adapters" but the actual implementation uses three adapters with ranks [8, 32, 64] as per the changes made around line 1198. Update the docstring to accurately reflect that three adapters are being used and consider mentioning their ranks to provide clearer documentation of what the test is doing.
🤖 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.
Outside diff comments:
In `@tests/unittest/llmapi/test_llm_pytorch.py`:
- Line 1164: The docstring starting at line 1164 mentions "Five dummy adapters"
but the actual implementation uses three adapters with ranks [8, 32, 64] as per
the changes made around line 1198. Update the docstring to accurately reflect
that three adapters are being used and consider mentioning their ranks to
provide clearer documentation of what the test is doing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 41a537ca-5d53-4dde-b24f-7847870865be
📒 Files selected for processing (2)
tests/integration/test_lists/waives.txttests/unittest/llmapi/test_llm_pytorch.py
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
07a5eff to
0603cb2
Compare
… pytest-timeout The H100_PCIe-PyTorch-Ray-1 stage runs the unittest harness with --timeout=2400 (40 min) for the inner pytest, while the test list's TIMEOUT (90) annotation is consumed by the integration harness and not propagated. test_qwen_moe_routed_expert_multi_lora_varying_ranks parametrizes 3 routed-expert MoE LoRA execution paths (host_path, device_path_eager, device_path_cudagraph), and each item rebuilds Qwen1.5-MoE-A2.7B-Chat (~14B) plus 5 dummy LoRA adapters and runs generate() twice; the combined cost exceeds 2400 s and pytest-timeout fires inside _write_routed_expert_lora_adapter. Reduce per-item compute by: - Dropping max_tokens 20 -> 5. The only generate-side assertion is out_tokens[i+1] != base_tokens, which holds at the first divergent token under greedy temperature=0.0 with nonzero LoRA-B weights. - Dropping the duplicated rank-16 adapter and one extra: ranks [8, 16, 32, 16, 64] -> [8, 32, 64]. This still exercises the slot-indexed device-path's per-slot rank dispatch (min, mid, max) with a no-LoRA row mixed into the same decode batch, and still builds adapters across every routed expert and MoE layer. Total wall-clock per item drops from >2400 s timeout to ~120 s (verified: 3 passed in 348 s for all three modes). Also remove the now-stale waiver entry so the test runs again in CI. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
0603cb2 to
199145a
Compare
Summary
Test plan
Links
Summary by CodeRabbit