Skip to content

Fix THD P2P pad detection and tail-zero gating - #3506

Closed
RPalmr wants to merge 13 commits into
NVIDIA:mainfrom
RPalmr:fix/thd-p2p-pad-detect
Closed

RPalmr wants to merge 13 commits into
NVIDIA:mainfrom
RPalmr:fix/thd-p2p-pad-detect

Conversation

@RPalmr

@RPalmr RPalmr commented Sep 10, 2026

Copy link
Copy Markdown

Fixes #3487.

Signed-off-by: Reece Palmer rxp528@student.bham.ac.uk

Fixes #3487.

Signed-off-by: Reece Palmer <rxp528@student.bham.ac.uk>
@RPalmr
RPalmr requested a review from cyanguwa as a code owner September 10, 2026 19:58
@github-actions github-actions Bot added the community-contribution PRs from external contributor outside the core maintainers, representing community-driven work. label Sep 10, 2026
@greptile-apps

greptile-apps Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The attention fixes appear sound, but the PR is not safe to merge until the release wheel path mismatch caused by the version spelling is corrected.

Summary

This PR corrects THD inter-sequence padding detection and limits padding cleanup to cases that actually contain inter-sequence padding.

  • Caches eager padding decisions for CUDA graph capture using tensor identity and mutation versions.
  • Uses capture-safe masks while retaining efficient eager tail-zeroing.
  • Gates context-parallel internal-padding cleanup on pad_between_seqs.
  • Also changes the development version spelling, which breaks assumptions in the release wheel repackaging script.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Read raw VERSION.txt<br/>2.21.0-dev0] --> B[setuptools builds wheel]
    B --> C[Packaging normalizes version<br/>2.21.0.dev0]
    C --> D[wheel unpack creates normalized directory]
    A --> E[build_wheels.sh constructs raw hyphenated path]
    D --> F{Paths match?}
    E --> F
    F -->|No| G[METADATA edit or directory move fails]
Loading

Reviews (9) · Last reviewed commit: "Merge branch 'main' into fix/thd-p2p-pad..."

@timmoon10 timmoon10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand this code very well, but at a glance it looks reasonable aside from some nits.

Comment on lines +116 to +117
_PAD_BETWEEN_SEQS_EAGER_CACHE_LIMIT = 64
_pad_between_seqs_eager_cache = {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

functools.lru_cache would be simpler and less error-prone than implementing the cache from scratch. That said, we already follow that anti-pattern with _thd_policy_validation_cache, so there's something to be said about maintaining consistency.

Comment on lines +123 to +130
def _key(t):
if t is None:
return (0, 0, 0, 0)
try:
version = t._version
except AttributeError:
version = 0
return (t.data_ptr(), tuple(t.shape), t.dtype, version)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it safe to assume that two tensors with the same buffer will have the same padding requirements? If cu_seqlens_* are not persistent, the same buffers may be reused in different steps or different layers. Perhaps it would be safer to do something like:

return (id(t), version)

cu_seqlens_kv_padded,
)
if cache_key in _pad_between_seqs_eager_cache:
pad_between_seqs = _pad_between_seqs_eager_cache[cache_key]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Noting that the cache is not populated during graph capture. In order to take advantage of the cache, we assume that the cache is populated in the warmup steps.

Fixes #3487.

Signed-off-by: Reece Palmer <rxp528@student.bham.ac.uk>
@RPalmr

RPalmr commented Sep 15, 2026

Copy link
Copy Markdown
Author

Thanks for looking. Context: #3487 regressed THD P2P when padded cu_seqlens are value equal but separate objects. Identity only detection forced the padded path. This restores value comparison in eager runs. It reuses that answer during capture with a small id plus version cache. Warmup hits reuse it. Misses use the safe fallback. It also skips tail and padding cleanup when there is nothing to clear. Your nits are addressed in the staged update.

RPalmr and others added 2 commits September 15, 2026 22:11
Signed-off-by: Reece Palmer <rxp528@student.bham.ac.uk>
@greptile-apps

greptile-apps Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Want your agent to iterate on Greptile's feedback? Try greploops.

@RPalmr RPalmr closed this Sep 29, 2026
@RPalmr
RPalmr deleted the fix/thd-p2p-pad-detect branch September 29, 2026 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

attention community-contribution PRs from external contributor outside the core maintainers, representing community-driven work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[PyTorch][Attention] THD P2P context-parallel regression when padded cu_seqlens are value-equal but not object-identical

3 participants