[None][test] Fix the multinode test case on DGX-Spark(perf skipped and func hang)#16209
[None][test] Fix the multinode test case on DGX-Spark(perf skipped and func hang)#16209JennyLiu-nv wants to merge 2 commits into
Conversation
Signed-off-by: Jenny Liu <JennyLiu-nv+JennyLiu@users.noreply.github.com>
📝 WalkthroughWalkthroughThe changes revise GPU availability detection for performance-test skipping and replace a subprocess-based multinode quickstart test with direct PyTorch backend LLM execution and generation validation. ChangesIntegration test updates
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/defs/test_e2e.py (1)
2224-2241: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftNo timeout safeguard on the call that previously hung.
The PR's own objective is fixing a functional hang in this exact test. The new direct
LLM(...)/generate()call still has no explicit timeout, so if it hangs again (e.g. an MPI synchronization stall across multi-node ranks), CI will block indefinitely rather than failing fast with a diagnosable error.Per
tests/README.md, this repo supports aTIMEOUT (N)marker settable per test case in the relevant test-list yml specifically for hang-prone cases — worth confirming one is applied here given this test's history.🤖 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 2224 - 2241, Ensure the test invoking LLM and generate has an explicit timeout safeguard so MPI or synchronization hangs fail fast instead of blocking CI indefinitely. Use the repository’s supported TIMEOUT (N) mechanism by adding or confirming the appropriate timeout marker for this test in its relevant test-list configuration, and verify it covers the direct LLM(...)/generate() call.
🤖 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.
Inline comments:
In `@tests/integration/defs/perf/test_perf.py`:
- Around line 841-859: Update the GPU-count detection around
get_mpi_world_size() and get_device_count() to fail closed: catch only the
expected device/MPI availability exceptions, and default a failed device count
to 0 so single-node detection cannot leave total_gpus as None and bypass the
pytest.skip guard. Preserve the TRTLLM_TOTAL_GPU_COUNT override and ensure the
existing self.num_gpus comparison skips when detection fails.
---
Nitpick comments:
In `@tests/integration/defs/test_e2e.py`:
- Around line 2224-2241: Ensure the test invoking LLM and generate has an
explicit timeout safeguard so MPI or synchronization hangs fail fast instead of
blocking CI indefinitely. Use the repository’s supported TIMEOUT (N) mechanism
by adding or confirming the appropriate timeout marker for this test in its
relevant test-list configuration, and verify it covers the direct
LLM(...)/generate() call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 47e2939c-95e3-4a20-be26-e2a3c9c5dccf
📒 Files selected for processing (2)
tests/integration/defs/perf/test_perf.pytests/integration/defs/test_e2e.py
|
/bot run |
|
PR_Github #58563 [ run ] triggered by Bot. Commit: |
|
PR_Github #58563 [ run ] completed with state |
2 participants
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.