Repository navigation
Fix test_joint_config_validation_uses_gathered_backends when no GPUs are visible - #7719
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
|
| Layer / File(s) | Summary |
|---|---|
Control visible CUDA device count tests/comm/test_ulysses_communicator.py |
The test uses monkeypatch to set torch.cuda.device_count() to one before encoding cuda:0. The existing validation assertions are unchanged. |
Priority:
Estimated code review effort: 1 (Trivial) | ~3 minutes
Change: Bug fix · Severity of issue fixed: Low
Merge Risk: 
Minimal · previous review ee552
-
The targeted test change is low risk and addresses zero-GPU CI behavior.
Pre-merge checks: 
5
-
Full details: Description check
-
Explanation
The description explains the failure, the monkeypatch-based fix, the related issue, and reported validation results. It omits some template checklist details, such as full-scope CI justification and the complete pre-commit setup confirmations, but it is sufficiently complete for this focused test fix.
Full details: Linked Issues check
-
Explanation
Issue
#7683requires the affected test to run when no CUDA devices are visible and reach the intended gathered-backend validation. The test now patchestorch.cuda.device_count()to return 1 before encodingcuda:0. This removes the host GPU-count dependency while preserving the existing validation cases.
Full details: Out of Scope Changes check
-
Explanation
The diff changes only
tests/comm/test_ulysses_communicator.py. It adds themonkeypatchfixture and a targetedtorch.cuda.device_count()patch for the test named in issue#7683. The change supports the issue objective and contains no unrelated implementation changes.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
@kangbintNV could you authorize the CI run when you get a chance? First-time contributor |
|
@flashinfer-bot run |
|
/bot run tests/comm/test_ulysses_communicator.py |
7ca063e to
ee552e8
Compare
|
@kangbintNV rebased on main — ready for review. |
|
[FAILED] Pipeline #77571011 — 34/41 executed test jobs passed Compared with nightly #77255012 (different CI configuration). Unit Tests
✅ Pass · 🟡 Old failure · ❌ New failure · ⏱ Test timeout · Multi-GPU and Multi-Node Tests — 5/12 passed
Failure detailsPR-related regressions
|
…GPUs _encode_config range-checks 'cuda:0' against torch.cuda.device_count(), so test_joint_config_validation_uses_gathered_backends failed on jobs with no visible device (Spark CI). The property under test does not depend on hardware, so pin the device count in the test. Fixes flashinfer-ai#7683
ee552e8 to
4250dec
Compare
|
@kangbintNV I’ve force-pushed since pipeline #77571011 started (now a single commit, 4250dec, rebased on current main). The multi-GPU test_ulysses_communicator failures (“forced nvlink must fail when init faults”) are in tests this PR doesn’t touch; the diff only changes test_joint_config_validation_uses_gathered_backends (+5/-1). Could you re-run /bot run tests/comm/test_ulysses_communicator.py on the new head, and could you check whether the same tests fail on main? |
|
@flashinfer-bot run tests/comm/test_ulysses_communicator.py |
|
/bot run tests/comm/test_ulysses_communicator.py |
|
[FAILED] Pipeline #77720163 — 30/32 executed test jobs passed Compared with nightly #77547447 (different CI configuration). Unit Tests
✅ Pass · 🟡 Old failure · ❌ New failure · ⏱ Test timeout · Multi-GPU and Multi-Node Tests — 10/12 passed
Failure detailsTimeouts, infrastructure, or incomplete jobs
|
|
Re-run #77720163 on 4250dec: Spark and all multi-GPU jobs pass, including test_ulysses_communicator, and the GitHub “Test Results Summary” is green. The only non-green jobs are the two GB300 multi-node ones (“script failed before producing a JUnit report”). I can’t see those logs, and the PR only changes one single-process test, so I don’t think they’re related. Could you take a look or re-run them? Otherwise this should be ready for review. |
|
@akshay919191 don't worry, this is node env issue, I reran your failed GB300 (multi-node) config and multi_node_test_gb300: [cu129] & multi_node_test_gb300: [cu130] are getting passed. |
|
@kangbintNV thanks for the re-runs. All checks are green now, and the only blocker is code owner review. Could you point me to, or request, the right owner for tests/comm/? |
|
@Anerudhan could you please review and approve this focused test-only fix for issue #7683? The exact testcase now passes on Spark CI with no visible CUDA devices (unit_test_spark: [cu130]). |
📌 Description
The test
test_joint_config_validation_uses_gathered_backendswas failing onzero-GPU CI machines because encoding
cuda:0raised a device visibilityerror before the dtype validation could run. This PR patches
torch.cuda.device_count()to return 1 viamonkeypatchso the testvalidates the actual logic (dtype checking) independently of host GPU count.
🔍 Related Issues
Closes #7683
🚀 Pull Request Checklist
Summary by CodeRabbit