[https://nvbugs/6111076][fix] ulysses+sage#13440
Conversation
📝 WalkthroughWalkthroughThese changes update Sage Attention multi-GPU tests by removing skip directives from the test waiver list and updating test code to initialize and supply attention metadata state objects to the attention module during test execution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/_torch/visual_gen/multi_gpu/test_ulysses_sage_attention.py (1)
230-271: QA list update is unnecessary for this PR scope.This change is limited to
tests/unittest/...and does not add/alter integration test definitions.As per coding guidelines "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unittest/_torch/visual_gen/multi_gpu/test_ulysses_sage_attention.py` around lines 230 - 271, The PR touches only unit tests (tests/unittest/_torch/visual_gen/multi_gpu/test_ulysses_sage_attention.py) so explicitly state in the PR description that QA list updates are unnecessary; update the PR body or the commit message to include a short note like "QA list update unnecessary — changes limited to tests/unittest/ only" and ensure any PR template checkbox about QA lists is marked or commented accordingly so reviewers know no integration-test or release QA changes are required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unittest/_torch/visual_gen/multi_gpu/test_ulysses_sage_attention.py`:
- Around line 230-271: The PR touches only unit tests
(tests/unittest/_torch/visual_gen/multi_gpu/test_ulysses_sage_attention.py) so
explicitly state in the PR description that QA list updates are unnecessary;
update the PR body or the commit message to include a short note like "QA list
update unnecessary — changes limited to tests/unittest/ only" and ensure any PR
template checkbox about QA lists is marked or commented accordingly so reviewers
know no integration-test or release QA changes are required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 37138d8c-9109-46d5-aebb-046ac320d406
📒 Files selected for processing (2)
tests/integration/test_lists/waives.txttests/unittest/_torch/visual_gen/multi_gpu/test_ulysses_sage_attention.py
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
|
/bot run --add-multi-gpu-test |
|
PR_Github #45412 [ run ] triggered by Bot. Commit: |
|
PR_Github #45412 [ run ] completed with state
|
xrq-phys
force-pushed
the
ruqingx/visual_gen/sage_attn
branch
from
963204c to
6343c0a
Compare
|
/bot run --add-multi-gpu-test |
|
PR_Github #45424 [ run ] triggered by Bot. Commit: |
|
PR_Github #45424 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test |
|
PR_Github #45449 [ run ] triggered by Bot. Commit: |
|
PR_Github #45449 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test |
|
PR_Github #45476 [ run ] triggered by Bot. Commit: |
|
PR_Github #45476 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #45489 [ run ] triggered by Bot. Commit: |
|
PR_Github #45489 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
/bot run --add-multi-gpu-test |
Head branch was pushed to by a user without write access
xrq-phys
force-pushed
the
ruqingx/visual_gen/sage_attn
branch
from
6343c0a to
c4faee6
Compare
|
/bot kill |
|
PR_Github #45669 [ kill ] triggered by Bot. Commit: |
|
PR_Github #45669 [ kill ] completed with state |
Signed-off-by: Ruqing Xu <7891482+xrq-phys@users.noreply.github.com>
xrq-phys
force-pushed
the
ruqingx/visual_gen/sage_attn
branch
from
c4faee6 to
8bb5c3e
Compare
|
/bot run --add-multi-gpu-test --disable-fail-fast |
|
PR_Github #45741 [ run ] triggered by Bot. Commit: |
|
PR_Github #45741 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test |
|
PR_Github #45963 [ run ] triggered by Bot. Commit: |
|
PR_Github #45963 [ run ] completed with state
|
|
/bot run --add-multi-gpu-test |
|
PR_Github #46047 [ run ] triggered by Bot. Commit: |
|
/bot skip --comment "All related tests (single and multi-gpu) have passed. GB multi-gpu can't pass due to node availability (c.f. status=PENDING log items)." |
|
@zhenhuaw-me we can merge this item now. |
|
PR_Github #46181 Bot args parsing error: Failed to parse bot args |
|
/bot skip --comment "All related tests, single and multi-gpu, have passed. GB multi-gpu cannot pass due to node availability. c.f. status PENDING log items." |
|
PR_Github #46195 [ skip ] triggered by Bot. Commit: |
|
PR_Github #46195 [ skip ] completed with state |
|
PR_Github #46047 has all tests passing except for SBSA multi-GPU cases. SBSA multi-GPU tests are failing / cancelled because they couldn't get nodes allocated: |
4 participants
Summary by CodeRabbit
Description
Fix ulysses+sage test due to metadata mis-configuration. The failure slipped by while working on #12937 .
Will undo #13432
Test Coverage
ulysses+sage tests
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)
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.