Skip to content

surface comm overlap rank errors - #3384

Open
sudhakarsingh27 wants to merge 4 commits into
NVIDIA:mainfrom
sudhakarsingh27:sudhakars/surface-comm-overlap-rank-errors
Open

surface comm overlap rank errors#3384
sudhakarsingh27 wants to merge 4 commits into
NVIDIA:mainfrom
sudhakarsingh27:sudhakars/surface-comm-overlap-rank-errors

Conversation

@sudhakarsingh27

Copy link
Copy Markdown
Member

Description

Please include a brief summary of the changes, relevant motivation and context.

Fixes # (issue)

Type of change

  • Documentation change (change only to the documentation, either a fix or a new content)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Infra/Build change
  • Code refactoring

Changes

Please list the changes introduced in this PR:

  • Change A
  • Change B

Checklist:

  • I have read and followed the contributing guidelines
  • The functionality is complete
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

sudhakarsingh27 and others added 3 commits August 14, 2026 13:41
Comm-overlap launchers captured child stdout and stderr but raised only stderr, which could leave the originating rank context out of pytest and JUnit failures. Preserve the existing result predicates while attaching bounded tails of both streams, and record layer-worker exceptions so torchrun can report the rank-local traceback.

Signed-off-by: Sudhakar Singh <sudhakars@nvidia.com>
The launcher mutated os.environ and then called os.unsetenv, which leaves Python's environment mapping unchanged. Conditional backend flags could therefore leak into later parameterized children. Build a child-only environment instead so each launch gets its intended overrides while preserving the parent process environment.

Signed-off-by: Sudhakar Singh <sudhakars@nvidia.com>
@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR improves failure reporting for distributed communication-overlap tests and isolates subprocess-specific environment overrides.

  • Records rank-level exceptions in the distributed layer runner.
  • Centralizes subprocess result validation with bounded stdout and stderr diagnostics.
  • Prevents test-specific environment settings from leaking between parametrized cases.
  • Disables FlashAttention v4 at the distributed QA suite boundary.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope.

No blocking failure remains.

Important Files Changed

Filename Overview
qa/L1_pytorch_distributed_unittest/test.sh Moves the FlashAttention v4 disablement to a suite-wide export and removes redundant per-command assignments.
tests/pytorch/distributed/run_layer_with_overlap.py Adds Torch Elastic exception recording so distributed rank failures provide structured diagnostics.
tests/pytorch/distributed/test_comm_gemm_overlap.py Adds centralized subprocess validation, text-mode output capture, bounded diagnostics, and isolated child-process environments.

Reviews (2): Last reviewed commit: "Keep FA4 out of L1 distributed tests" | Re-trigger Greptile

@sudhakarsingh27 sudhakarsingh27 changed the title Sudhakars/surface comm overlap rank errors surface comm overlap rank errors Aug 15, 2026
The moving PyTorch image installs FA4 by default, implicitly expanding an L1 suite that historically covered earlier attention backends. Export the FA4 selector at the suite boundary so every pytest and torchrun child retains the intended backend scope while dedicated attention suites own FA4 coverage.

Signed-off-by: Sudhakar Singh <sudhakars@nvidia.com>
@sudhakarsingh27

Copy link
Copy Markdown
Member Author

/te-ci pytorch L1

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.

1 participant