Skip to content

fix(distributed): synchronize convergence across ranks - #150

Open
atulcthakur wants to merge 4 commits into
NVIDIA:0.2.0-rcfrom
atulcthakur:test/dd-convergence-regressions
Open

fix(distributed): synchronize convergence across ranks#150
atulcthakur wants to merge 4 commits into
NVIDIA:0.2.0-rcfrom
atulcthakur:test/dd-convergence-regressions

Conversation

@atulcthakur

Copy link
Copy Markdown
Collaborator

ALCHEMI Toolkit Pull Request

Description

DomainParallel treated convergence indices as Boolean values. Because tensor([0]) means “system 0 converged” but converts to False, convergence could be missed.
Ranks could also disagree locally and enter different collectives, causing a hang.
The DD run and pipeline paths also failed to stop or retire the converged system.

Fix

  • Preserve convergence as system-index tensors.
  • Reduce a per-system convergence mask across every domain rank.
  • Stop completed DD runs and retire converged pipeline systems.
  • Avoid convergence collectives when no convergence hook is configured.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Performance improvement
  • Documentation update
  • Refactoring (no functional changes)
  • CI/CD or infrastructure change

Related Issues

Changes Made

  • nvalchemi/distributed/domain_parallel.py

    • Fix distributed convergence reduction in step().
    • Add convergence-based early exit to run().
    • Fix pipeline completion and completion-reason checks.
  • test/distributed/test_domain_parallel_convergence.py

    • Test preservation of tensor([0]).
    • Test rank disagreement without hanging.
    • Test that fixed-step dynamics adds no convergence collective.
    • Test run() early exit.
    • Test pipeline-stage retirement.

Testing

  • Unit tests pass locally (make pytest)
  • Linting passes (make lint)
  • New tests added for new functionality meets coverage expectations?

Checklist

  • I have read and understand the Contributing Guidelines
  • I have updated the CHANGELOG.md
  • I have performed a self-review of my code
  • I have added docstrings to new functions/classes
  • I have updated the documentation (if applicable)

Additional Notes

Tip

This repository uses Greptile, an AI code review service, to help conduct
pull request reviews. We encourage contributors to read and consider suggestions
made by Greptile, but note that human maintainers will provide the necessary
reviews for merging: Greptile's comments are not a qualitative judgement
of your code, nor is it an indication that the PR will be accepted/rejected.
We encourage the use of emoji reactions to Greptile comments, depending on
their usefulness and accuracy.

Preserve converged system indices, keep convergence collectives rank-symmetric, stop completed runs, and retire converged pipeline systems.

Signed-off-by: atulcthakur <atthakur@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes three inter-related correctness bugs in DomainParallel's convergence handling and adds a dedicated regression-test suite to cover them.

  • Falsy-zero bug: system index tensor([0]) evaluated to False in the old boolean-reduction path; the new code keeps convergence as a per-system index tensor and reduces a (num_graphs,) integer mask with ReduceOp.MIN.
  • Rank-desync / hang: convergence was previously only checked when the local rank already had a non-None result, so ranks could skip the collective and deadlock; convergence_due is now evaluated uniformly so every rank always enters the same all_reduce.
  • Missing early-exit paths: run() never broke on convergence, and pipeline stages never retired a converged system; both are now handled with the correct numel() == num_graphs predicate.

Important Files Changed

Filename Overview
nvalchemi/distributed/domain_parallel.py Three convergence bugs fixed: tensor([0]) falsy-boolean, rank-disagree hang, and pipeline retirement; logic is correct throughout.
test/distributed/test_domain_parallel_convergence.py New regression-test suite covering all fixed paths; one grouped-pipeline test is appropriately skipped with a Gloo-reliability rationale.

Reviews (4): Last reviewed commit: "test: skip unreliable grouped Gloo pipel..." | Re-trigger Greptile

Comment thread nvalchemi/distributed/domain_parallel.py
Comment thread nvalchemi/distributed/domain_parallel.py Outdated
@atulcthakur

Copy link
Copy Markdown
Collaborator Author

/ok to test 6a34c19

Comment thread nvalchemi/distributed/domain_parallel.py
Comment thread nvalchemi/distributed/domain_parallel.py
Comment thread test/distributed/test_domain_parallel_convergence.py
Comment thread test/distributed/test_domain_parallel_convergence.py
Comment thread nvalchemi/distributed/domain_parallel.py Outdated
Comment thread nvalchemi/distributed/domain_parallel.py Outdated
Signed-off-by: atulcthakur <atthakur@nvidia.com>
laserkelvin added a commit to laserkelvin/nvalchemi-toolkit that referenced this pull request Jul 27, 2026
…orkaround

Regenerating the lockfile on this branch silently upgraded warp-lang from
1.14.0 to 1.15.0. That upgrade, not the documentation, caused the cudagraph
failures: the CI runs differ in exactly one package.

    PR NVIDIA#150 (passes):  torch 2.13.0+cu130, triton 3.7.1, warp-lang 1.14.0
    this branch (fails): torch 2.13.0+cu130, triton 3.7.1, warp-lang 1.15.0

The failing test compiles scatter_reduce_per_graph, whose _segmented_sum
is a custom_op wrapping wp.from_torch and a Warp kernel launch. Under
1.15.0 that path leaves an allocation live in the cudagraph memory pool which
is not tracked as a graph output, so the next capture fails
check_memory_pool. warp-lang >= 1.13.0 has no upper bound, so only the
lockfile pinned it.

Restore the lockfile from 0.2.0-rc and re-lock, which adds only
autodoc-pydantic (plus its pydantic-settings dependency) for the docs
build and leaves every other pin untouched. Revert the conftest reset fixture
added earlier: it was masking this dependency change rather than fixing a test
isolation bug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
Signed-off-by: atulcthakur <atthakur@nvidia.com>
@atulcthakur

Copy link
Copy Markdown
Collaborator Author

/ok to test 63c6161

Signed-off-by: atulcthakur <atthakur@nvidia.com>
@atulcthakur

Copy link
Copy Markdown
Collaborator Author

/ok to test 369a08b

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