Skip to content

fix(distributed): leave neighbor-list arrays out of atom migration - #254

Merged
laserkelvin merged 3 commits into
NVIDIA:mainfrom
dallasfoster:dallasf/fix-migration-nl-fields
Nov 14, 2026
Merged

laserkelvin merged 3 commits into
NVIDIA:mainfrom
dallasfoster:dallasf/fix-migration-nl-fields

Conversation

@dallasfoster

Copy link
Copy Markdown
Collaborator

ALCHEMI Toolkit Pull Request

Description

apply_migration resharded every per-atom field, including the neighbor matrix a NeighborListHook wrote. Each rank sizes that width on its own, so ranks that had grown max_neighbors differently sent rows of different widths in one all_to_all and every rank stalled.

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

None.

Changes Made

  • Migration skips the neighbor-list arrays, as the scatter path already does; they are rebuilt after the move.
  • Regression tests in test/distributed/test_strategy.py.

Testing

Rebased on current main. make pytest (Python 3.12, extras mace aimnet ase pymatgen) gave 9254 passed, 122 skipped, 0 failed on the preceding main (cf391676). After rebasing onto 36a7fc85, make lint passes and test/distributed/test_strategy.py gives 11 passed.

  • 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

Found while validating domain-decomposed runs; independent of other open work.

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.

apply_migration resharded every per-atom field, including the neighbor matrix
a NeighborListHook had written into the owned batch. Each rank sizes that
matrix's width on its own, so ranks that had grown max_neighbors differently
sent rows of different widths in one all_to_all and every rank stalled. The
arrays are rebuilt after the move and the scatter path already skips them, so
migration now skips them too.

Signed-off-by: Dallas Foster <dallasf@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Nov 5, 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 Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in Greptile

[Medium impact] The PR appears safe to merge.

Summary

Atom migration now skips temporary neighbor arrays, matching the scatter path.

  • Atom migration leaves rank-sized neighbor lists out of the exchange.

Reviews (3) · Last reviewed commit: "Merge branch 'main' into dallasf/fix-mig..." · Reviewed by Greptile

Signed-off-by: Moritz R Schäfer <mschafer@nvidia.com>
@M-R-Schaefer

Copy link
Copy Markdown
Collaborator

/ok to test a18d81a

Signed-off-by: Kelvin Lee <kinlongkelvi@nvidia.com>
@laserkelvin

Copy link
Copy Markdown
Collaborator

/ok to test b131140

@laserkelvin
laserkelvin added this pull request to the merge queue Nov 14, 2026
Merged via the queue into NVIDIA:main with commit 5c6eb57 Nov 14, 2026
10 checks passed
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