Repository navigation
fix: Fix some issues - #4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses runtime errors and incorrect NVSHMEM peer selection in the PyTorch context-parallel attention implementation that uses NVSHMEM for KV exchange.
Changes:
- Persist
nvshmem_kvon the autograd context so it is available inbackward(). - Switch NVSHMEM
peerargument from a global rank mapping to the CP-group rank (owner_idx) for correct addressing when CP is combined with other parallelism modes. - Remove the now-unused global-rank mapping usage around NVSHMEM gets.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
The NVSHMEM fallback path is effectively unreachable here: init_nvshmem_backend() / create_nvshmem_tensor() will raise if NVSHMEM isn't available, but the try/except is commented out and later code still checks if nvshmem_kv is not None to decide between NVSHMEM vs P2P. Either restore the guarded fallback (so nvshmem_kv can legitimately be None) or remove the if nvshmem_kv is not None branches and related commented code to keep behavior and comments consistent.
| fa_backward_kwargs["softcap"] = 0.0 | ||
|
|
||
| nvshmem_kv = ctx.nvshmem_kv | ||
| cp_global_ranks = ctx.cp_global_ranks |
There was a problem hiding this comment.
cp_global_ranks = ctx.cp_global_ranks is now unused in backward() after switching NVSHMEM peer addressing to owner_idx. Please remove this assignment (or use it) to avoid dead code and confusion about whether peer should be global rank or process-group rank.
| cp_global_ranks = ctx.cp_global_ranks |
…into fix-some-issues
Description
Type of change
Changes
Checklist: