Skip to content

[None][test] Guard NCCL scope work and tensor ownership - #21230

Open
ruodil wants to merge 3 commits into
NVIDIA:mainfrom
ruodil:user/ruodil/test-module-nccl-scopes
Open

ruodil wants to merge 3 commits into
NVIDIA:mainfrom
ruodil:user/ruodil/test-module-nccl-scopes

Conversation

@ruodil

@ruodil ruodil commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Guard peerless forwards against unnecessary NCCL lease-scope work, and preserve activation/output ownership without repeatedly traversing shared metadata, including nested containers and failure unwinding.

Split the nccl module coverage out of #21095 so it can be reviewed independently. This PR contains only its tests and any associated CI-list changes.

The production prerequisite #21095 merged into main as ff1aa0f7d35c on 2026-11-03. This test-only PR now includes that fix through a normal merge of main (fee6cfd872bc) and has passed current-head L0 CI; Code Owner review remains required.

Test Coverage

  • The preserved module coverage passed in a fresh run of the original PR snapshot: 57 CPU, zero failures/errors/skips. The complete original snapshot passed 443 CPU + 9 GPU cases on RTX PRO 6000 Blackwell; this is not a runtime result for the rebased PR head.

  • Current merged head 478282ef8776: all applicable pre-commit hooks passed, including Ruff, formatting, generated-config checks and repository-wide test-list validation; git diff --check passed. Independent review found no current API mismatch. The NCCL model-call test follows the upstream rename to tests/unittest/_torch/pyexecutor/engine/; upstream graph tests and their updated collector fixture are preserved.

  • Current-head remote validation (478282ef8776): L0 pipeline #67850 completed SUCCESS. This supersedes the earlier local collection limitation caused by the host lacking libcuda.so.1. Required GitHub checks are passing; Code Owner approval is still required.

  • The seven split patches were checked for complete coverage of the original 22-file diff and exact recombination after the required upstream API compatibility changes.

  • CI selection verified on main: CPU coverage is selected by the existing unittest/_torch/pyexecutor directory entry and unittest/_torch/test_nccl_window_graph.py entry in l0_cpu.yml; both changed modules are marked cpu_only. Directory selectors are expanded by tests/integration/defs/test_unittests.py, and Jenkins applies cpu_only / not cpu_only by stage.

PR Checklist

  • Reviewed the applicable repository PR checklist items; the commit includes a DCO sign-off.

Merge readiness (2026-11-06)

Current head 478282ef8776 has passing blossom-ci and DCO checks, no unresolved review threads, and no merge conflict with main in the GitHub mergeability check. GitHub still reports REVIEW_REQUIRED; the existing Code Owner review requests are the remaining merge gate. No source update or CI rerun is needed for the current passing head.

Signed-off-by: Ruodi Lu <ruodil@users.noreply.github.com>
ruodil and others added 2 commits October 30, 2026 10:11
Signed-off-by: Ruodi Lu <200874449+ruodil@users.noreply.github.com>
Signed-off-by: Ruodi Lu <ruodil@users.noreply.github.com>
@ruodil
ruodil marked this pull request as ready for review November 3, 2026 06:21
@ruodil

ruodil commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82223 [ run ] triggered by Bot. Commit: 478282e Link to invocation

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The pull request adds CPU-only tests for NCCL window tensor collection and scope behavior. The tests cover eager module calls, ModelEngine peer configurations, nested scopes, and failure recovery.

Changes

NCCL Window Tensor Scopes

Layer / File(s) Summary
Eager module tensor collection
tests/unittest/_torch/test_nccl_window_graph.py
Tests input and output collection for positional and keyword calls, including quantized wrappers, nested containers, aliases, cycles, dataclass metadata, and nested scopes.
ModelEngine scope lifecycle
tests/unittest/_torch/pyexecutor/engine/test_nccl_window_model_call.py
Tests scope and request-ID behavior across peer modes, nested layers, and request counts. A failure-path test checks failed scope endings and a successful subsequent forward.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Merge Risk: Minimal · up to 47828

The new tests are registered for CPU CI, and no merge-blocking defect was identified. Current-head runtime results remain pending.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed

Check name Status Explanation
Title check Passed The title follows the required [None][type] format, clearly identifies a test change, and summarizes the NCCL scope and tensor ownership coverage.
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.
Description check Passed The description explains the purpose, test coverage, and validation status. It includes the required checklist.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. 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 14 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

ast-grep (0.51.1)

ast-grep could not parse rule config: /ast-grep-rules/ast-grep-essentials/rules/rust/security/reqwest-accept-invalid-rust.yml

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between fee6cfd and 478282e.

Files selected for processing: 2

  • tests/unittest/_torch/pyexecutor/engine/test_nccl_window_model_call.py
  • tests/unittest/_torch/test_nccl_window_graph.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #82223 [ run ] completed with state SUCCESS. Commit: 478282e
/LLM/main/L0_MergeRequest_PR pipeline #67850 completed with status: 'SUCCESS'

CI Report

Link to invocation

@ruodil

ruodil commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

Merge-readiness check (2026-11-06): current head 478282ef8776 passed L0 #67850, blossom-ci and DCO are green, all review threads are resolved, and GitHub reports no merge conflict. The existing CPU directory selector covers the model-call test; the graph test also has an explicit CPU-list entry.

Updated the description to replace superseded local-environment/CI-list uncertainty with the verified current result. GitHub still reports REVIEW_REQUIRED; @QiJune @asfiyab-nvidia, could you complete the pending Code Owner review? No source changes or additional CI rerun are needed for this head.

This branch has not been deployed

No deployments
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