Skip to content

[None][perf] template NVLink A2A fault tolerance checks#16025

Draft
bobboli wants to merge 3 commits into
NVIDIA:mainfrom
bobboli:bli/pr15524-a2a-mask-cleanup
Draft

[None][perf] template NVLink A2A fault tolerance checks#16025
bobboli wants to merge 3 commits into
NVIDIA:mainfrom
bobboli:bli/pr15524-a2a-mask-cleanup

Conversation

@bobboli

@bobboli bobboli commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Compile-time specialize NVLink one-sided A2A dispatch and combine kernels with ENABLE_FAULT_TOLERANCE.
  • Keep is_rank_active() as a plain bit-test helper and place every call inside if constexpr (ENABLE_FAULT_TOLERANCE), so non-FT kernels contain no active-rank checks.
  • Preserve FT route and payload protection while the runtime does not yet guarantee that routing excludes inactive ranks.
  • Scope inactive-peer handling for counters, EPLB stats, completion signaling, and completion waits to the FT kernel instantiations.
  • Pass an explicit enable_fault_tolerance mode from the construction-time ep_group_health state; active_rank_mask is validated and copied only after the mode is selected and no longer chooses the kernel specialization.

CUDA-graph lifecycle handling for changing active-rank masks remains follow-up work and is not addressed by this PR.

Testing

  • pre-commit run --files for all changed C++, CUDA, Python, and test files
  • python -m py_compile for the changed Python files
  • git diff --check

Not run: GPU/MPI tests locally.

@bobboli
bobboli force-pushed the bli/pr15524-a2a-mask-cleanup branch from 709ea27 to 9732564 Compare July 7, 2026 08:07
@bobboli bobboli changed the title [None][test] cover NVLink A2A mask epoch validation [None][perf] template NVLink A2A fault tolerance checks Jul 7, 2026
@bobboli
bobboli requested a review from chienchunhung July 7, 2026 08:27
@bobboli
bobboli force-pushed the bli/pr15524-a2a-mask-cleanup branch 7 times, most recently from 4287f6c to bb18c31 Compare July 7, 2026 09:23
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
@bobboli
bobboli force-pushed the bli/pr15524-a2a-mask-cleanup branch from bb18c31 to 2816175 Compare July 7, 2026 09:27

@chienchunhung chienchunhung 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 your PR and raising the concerns! Really appreciate that you look out for the issues.

Compile-time specialization is a valid improvement and addresses concern (b).

However, the removal of FT route and payload checks assumes concern (a) is already an enforced runtime invariant. It is not: #15525 supplies only the EPLB reconfiguration primitive, while there are 4 planned PRs on the road towards the goal; #15895 covers only in-flight abort.

On the other hand, I am not sure if PR #16025 fully addresses concern (c): the mask remains captured by value and no CUDA-graph lifecycle is changed. Full graph handling is planned in my MVP scope as later PR.

My suggestion: Please keep the compile-time FT/non-FT specialization, but remove the FT-specialized route/payload checks for now. They, as for addressing concern (a) and (c), require a dependency-ordered follow-up after the routing, atomic-commit, failed-epoch, and CUDA-graph contracts land and are tested. Until the MVP fully lands, FT must explicitly disable or reject CUDA graphs.

bobboli added 2 commits July 10, 2026 01:00
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
@bobboli

bobboli commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for your PR and raising the concerns! Really appreciate that you look out for the issues.

Compile-time specialization is a valid improvement and addresses concern (b).

However, the removal of FT route and payload checks assumes concern (a) is already an enforced runtime invariant. It is not: #15525 supplies only the EPLB reconfiguration primitive, while there are 4 planned PRs on the road towards the goal; #15895 covers only in-flight abort.

On the other hand, I am not sure if PR #16025 fully addresses concern (c): the mask remains captured by value and no CUDA-graph lifecycle is changed. Full graph handling is planned in my MVP scope as later PR.

My suggestion: Please keep the compile-time FT/non-FT specialization, but remove the FT-specialized route/payload checks for now. They, as for addressing concern (a) and (c), require a dependency-ordered follow-up after the routing, atomic-commit, failed-epoch, and CUDA-graph contracts land and are tested. Until the MVP fully lands, FT must explicitly disable or reject CUDA graphs.

I have updated this PR, and now
(a) No longer assuming that the routed rank must be alive.
(b) Addressed, FT code paths all guarded by the template arg ENABLE_FAULT_TOLERENCE.
(c) Still no conclusion. My feeling is that the runtime should populate maskTensor on GPU, and directly passes its pointer to the kernel. resolveActiveRankMask should be deleted in the future.

Feel free to continue on this PR, or pick the changes into your WIP PRs.

@chienchunhung

Copy link
Copy Markdown
Collaborator

Thanks for your PR and raising the concerns! Really appreciate that you look out for the issues.
Compile-time specialization is a valid improvement and addresses concern (b).
However, the removal of FT route and payload checks assumes concern (a) is already an enforced runtime invariant. It is not: #15525 supplies only the EPLB reconfiguration primitive, while there are 4 planned PRs on the road towards the goal; #15895 covers only in-flight abort.
On the other hand, I am not sure if PR #16025 fully addresses concern (c): the mask remains captured by value and no CUDA-graph lifecycle is changed. Full graph handling is planned in my MVP scope as later PR.
My suggestion: Please keep the compile-time FT/non-FT specialization, but remove the FT-specialized route/payload checks for now. They, as for addressing concern (a) and (c), require a dependency-ordered follow-up after the routing, atomic-commit, failed-epoch, and CUDA-graph contracts land and are tested. Until the MVP fully lands, FT must explicitly disable or reject CUDA graphs.

I have updated this PR, and now (a) No longer assuming that the routed rank must be alive. (b) Addressed, FT code paths all guarded by the template arg ENABLE_FAULT_TOLERENCE. (c) Still no conclusion. My feeling is that the runtime should populate maskTensor on GPU, and directly passes its pointer to the kernel. resolveActiveRankMask should be deleted in the future.

Feel free to continue on this PR, or pick the changes into your WIP PRs.

Thank you @bobboli ! I also created #16200 in correspondence to an NVBUG (perf regression). Feel free to review it.

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