Skip to content

Reject negative process grid dimensions#141

Merged
romerojosh merged 2 commits into
NVIDIA:mainfrom
fallintoplace:fix/reject-negative-pdims
Jun 23, 2026
Merged

Reject negative process grid dimensions#141
romerojosh merged 2 commits into
NVIDIA:mainfrom
fallintoplace:fix/reject-negative-pdims

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

Reject negative pdims values during grid descriptor validation before checking the process-grid product.

Previously, checkConfig only used product semantics for explicit process grids, so a configuration such as [-2, -2] could pass on four ranks. The later process-grid indexing code assumes positive dimensions for division, modulo, communicator setup, and pencil decomposition.

This also widens the product calculation to int64_t so validation does not overflow before comparing with the rank count.

Tests

  • git diff --check
  • Added API regression coverage for negative pdims pairs whose product equals the test rank count

Local build/test execution was not available in this checkout because clang-format, nvcc, and mpicxx are not installed.

Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>

@romerojosh romerojosh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @fallintoplace! I've left a comment to address but otherwise this looks good. Definitely covering a rare edge case, but better to catch this user error than not.

Comment thread tests/ctest/api_tests.cc Outdated
Signed-off-by: Josh Romero <joshr@nvidia.com>
@romerojosh

Copy link
Copy Markdown
Collaborator

/build

@github-actions

Copy link
Copy Markdown

🚀 Build workflow triggered! View run

@github-actions

Copy link
Copy Markdown

✅ Build workflow passed! View run

@romerojosh romerojosh merged commit d6ae560 into NVIDIA:main Jun 23, 2026
4 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.

2 participants