Skip to content

[https://nvbugs/6190759][fix] Promote the multi-node guard to a module-level pytest.mark.skipif so skipping ha - #14533

Closed
tensorrt-cicd wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6190759
Closed

[https://nvbugs/6190759][fix] Promote the multi-node guard to a module-level pytest.mark.skipif so skipping ha#14533
tensorrt-cicd wants to merge 1 commit into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6190759

Conversation

@tensorrt-cicd

@tensorrt-cicd tensorrt-cicd commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Root cause: disagg_server fixture asserts len(NODE_LIST) >= 2 before test body runs, so the in-test pytest.skip on single-node never fires.
  • Fix: Promote the multi-node guard to a module-level pytest.mark.skipif so skipping happens at collection time before fixtures run; remove the redundant in-test skip; drop the stale waive entry left over from the engineer's UCX_TLS fix (90694fe).
  • Automated fix generated by repair-bot

Test plan

  • Verify fix on the same GPU type as the original failure
  • Check for regressions in related tests

Links

Summary by CodeRabbit

  • Tests
    • Removed a waived test entry to enforce test execution.
    • Refactored test skip conditions to use module-level markers for improved test organization.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 71aa2660-dab5-489e-ab2f-31d3a7e8e861

📥 Commits

Reviewing files that changed from the base of the PR and between 4517988 and 8a95750.

📒 Files selected for processing (2)
  • tests/integration/test_lists/waives.txt
  • tests/unittest/llmapi/apps/_test_disagg_serving_multi_nodes.py
💤 Files with no reviewable changes (1)
  • tests/integration/test_lists/waives.txt

📝 Walkthrough

Walkthrough

This PR migrates node-count validation from an inline runtime check to a module-level pytest marker in a multi-node disaggregated test, then removes the corresponding waived test entry from the integration waivers list. The test now skips execution at pytest collection time when exactly 2 nodes are not available.

Changes

Disaggregated Multi-Node Test Refactoring

Layer / File(s) Summary
Node-count validation migration and waiver removal
tests/unittest/llmapi/apps/_test_disagg_serving_multi_nodes.py, tests/integration/test_lists/waives.txt
Module-level pytestmark is expanded to include a skipif marker checking NODE_LIST length; the inline guard in test_completion that performed this check is removed; the integration waiver entry for test_openai_disagg_multi_nodes_completion[ctx_tp1pp2-gen_tp1pp2] is deleted.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Suggested reviewers

  • jieli-matrix
  • ZhanruiSunCh
  • mzweilz
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description explains the root cause, the fix, and references the bug, but lacks explicit test coverage details and PR checklist confirmation required by the template. Add a more explicit Test Coverage section listing the specific tests affected, and confirm checklist items (API changes, dependencies, CODEOWNERS, documentation) for completeness.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title correctly identifies the main change—promoting multi-node guard to module-level pytest.mark.skipif—with proper NVBugs reference and [fix] type annotation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


Comment @coderabbitai help to get the list of available commands and usage tips.

@tensorrt-cicd
tensorrt-cicd force-pushed the repair-bot-bug6190759 branch 4 times, most recently from 90c739b to f0f4da6 Compare June 3, 2026 16:25
@tensorrt-cicd
tensorrt-cicd force-pushed the repair-bot-bug6190759 branch from f0f4da6 to 1361aca Compare June 9, 2026 07:28
…ode and unwaive

The disagg_server fixture calls get_the_other_host(0), which asserts
len(NODE_LIST) >= 2. On single-node CI runners (empty SLURM_NODELIST),
this fires during fixture setup before test_completion's pytest.skip
can run, so the test errors instead of skipping.

Promote the multi-node guard to a module-level pytest.mark.skipif so
the skip happens during collection, before any fixture runs. Drop the
now-redundant in-test skip block. Remove the stale waive entry that
the prior UCX_TLS fix (90694fe) missed.

Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
@trtllm-agent
trtllm-agent force-pushed the repair-bot-bug6190759 branch from 1361aca to 2a9de1b Compare July 15, 2026 08:24
@trtllm-agent
trtllm-agent requested a review from a team as a code owner July 15, 2026 08:24
@schetlur-nv

Copy link
Copy Markdown
Collaborator

Closing based on bug status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants