Skip to content

[DO NOT MERGE][TEST ONLY] Create AGENTS.md#3281

Draft
Stonepia wants to merge 5 commits intomainfrom
tong/agents
Draft

[DO NOT MERGE][TEST ONLY] Create AGENTS.md#3281
Stonepia wants to merge 5 commits intomainfrom
tong/agents

Conversation

@Stonepia
Copy link
Copy Markdown
Contributor

@Stonepia Stonepia commented Apr 8, 2026

This PR is for test only

@laifenxiawucha
Copy link
Copy Markdown

@laifenxiawucha 1

@mengfei25 mengfei25 added the disable_all Disable all ci test jobs for the PR, just keep basic lint check label Apr 8, 2026
@laifenxiawucha
Copy link
Copy Markdown

@laifenxiawucha review

@laifenxiawucha

This comment has been minimized.

@laifenxiawucha

This comment has been minimized.

@laifenxiawucha
Copy link
Copy Markdown

@laifenxiawucha review

@laifenxiawucha

This comment has been minimized.

@laifenxiawucha
Copy link
Copy Markdown

@laifenxiawucha 1

@laifenxiawucha

This comment was marked as resolved.

@laifenxiawucha

This comment was marked as resolved.

@laifenxiawucha

This comment was marked as off-topic.

@laifenxiawucha

This comment was marked as resolved.

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as duplicate.

@laifenxiawucha

This comment has been minimized.

@laifenxiawucha

This comment was marked as resolved.

@laifenxiawucha
Copy link
Copy Markdown

@laifenxiawucha review

@laifenxiawucha

This comment was marked as resolved.

@laifenxiawucha

This comment was marked as duplicate.

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha
Copy link
Copy Markdown

@laifenxiawucha review

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as off-topic.

@laifenxiawucha

This comment was marked as resolved.

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as off-topic.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment was marked as resolved.

@laifenxiawucha

This comment was marked as duplicate.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment has been minimized.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment was marked as abuse.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment was marked as off-topic.

@laifenxiawucha
Copy link
Copy Markdown

Requested in this mention.

I see a ping from @laifenxiawucha but the message "@laifenxiawucha 1" doesn't clearly indicate what you'd like me to help with.

Could you please clarify what you need? For example:

  • A code review of this AGENTS.md PR?
  • Feedback on the AGENTS.md content or structure?
  • Something else specific?

@laifenxiawucha

This comment was marked as off-topic.

@laifenxiawucha

This comment was marked as spam.

@laifenxiawucha

This comment has been minimized.

@laifenxiawucha
Copy link
Copy Markdown

@laifenxiawucha review

@laifenxiawucha
Copy link
Copy Markdown

laifenxiawucha commented Apr 9, 2026

reply with skills:
Requested in this mention.

Status: Request Changes

High (Must-Fix)

Missing dispatch target will cause build/runtime failure

  • Problem: Function log_softmax_sparse_xpu is removed but remains registered in dispatch configuration
  • Evidence: log_softmax_sparse_xpu deleted from src/ATen/native/sparse/xpu/SparseSoftmax.cpp:23-26 while yaml/native/native_functions.yaml:line неизвестен still contains SparseXPU: log_softmax_sparse_xpu
  • Why it matters: Dispatch lookup will fail at runtime when log_softmax is called on sparse XPU tensors, causing undefined linker error or missing function crash
  • Suggestion: Restore log_softmax_sparse_xpu function or remove/update the yaml entry to match

OR-incorrect error message after dimension check change

  • Problem: Error message text does not match the actual dimension being checked
  • Evidence: src/ATen/native/transformers/Attention.cpp:147-150 checks qkv_weight.dim() == 4 but error message reads "expected 2-D \qkv_weight`"`
  • Why it matters: Developers and users will be misleading, failing quickly or inconsistently, handling wrong shapes leads to incorrect buffer interprets
  • Suggestion: Change error message from "expected 2-D\qkv_weight`"to"expected 4-D`qkv_weight`"`

Inconsistent backward/forward status

  • Problem: Backward function log_softmax_backward_sparse_xpu remains while its forward counterpart is removed
  • Evidence: Forward removal at src/ATen/native/sparse/xpu/SparseSoftmax.cpp:23-26, backward present at src/ATen/native/sparse/xpu/SparseSoftmax.cpp:37-40
  • Why it matters: Calling backward without a forward path is nonsensical; this breaks expected lifecycle for log_softmax and may leave dead code
  • Suggestion: Remove log_softmax_backward_sparse_xpu as well or restore the forward to maintain the API contract

Medium

Safety guard removed without justification

  • Problem: NestedTensor-with-mask restriction check removed, comments misaligned
  • Evidence: Guard TORCH_CHECK(!mask || !query.is_nested(), "NestedTensor with mask is not supported yet") removed from src/ATen/native/transformers/Attention.cpp:128-130 with no analysis
  • Why it matters: Removes documented safety envelope; could enable unsupported behavior that yields incorrect results; inconsistent with CUDA/backend assumptions
  • Suggestion: Restore the check and prove that the unsupported case is truly handled or add test + comment explaining why it’s safe

Untested logic changes

  • Problem: Logic changes (dimension check and guard removal) lack corresponding tests
  • Evidence: No new tests added for native_multi_head_attention_xpu dimension changes; no tests for removed guard covering when mask + nested query appear
  • Why it matters: CI will not catch regressions; behavior changes may affect multi-head attention correctness on XPU
  • Suggestion: Add a test that verifies 4-D qkv_weight shape validation; add or update test exercising mask + nested query if indeed supported now

Low / Suggestion

Clarify function renaming

  • Problem: Kernel name changed from softmax_sparse_xpu_kernel to softmax_forward_sparse_xpu_kernel without explanation
  • Evidence: src/ATen/native/sparse/xpu/SparseSoftmax.cpp:18 changed to call softmax_forward_sparse_xpu_kernel instead of softmax_sparse_xpu_kernel
  • Why it matters: Unclear whether this is a deliberate refactoring or a local inconsistency; hampers readability
  • Suggestion: Add inline comment or PR description line explaining rename and noting that backward kernels remained unchanged

Residual Risk

  • Dispatch reachability for sparse softmax/log_softmax not verified for all paths; may affect features beyond the changed site
  • Dimension behavior impact on Windows/accelerate pipelines not verified; missing blast radius analysis
  • No explanation of why 4-D qkv_weight is now expected (e.g., whether this adds a new head/grouped-attention mode or just aligns with CUDA semantics)

NOTES

  • This PR is tagged [DO NOT MERGE][TEST ONLY], suggesting it may be a test harness rather than a ready change. Regardless, the comments above reflect concrete correctness issues in the current diffs and should be addressed before any merge.

@laifenxiawucha
Copy link
Copy Markdown

@laifenxiawucha 你有什么skills,怎么才能用这些skills

@laifenxiawucha
Copy link
Copy Markdown

laifenxiawucha commented Apr 9, 2026

Requested in this mention.

你好!目前我支持以下 skills,它们会根据你的 @mention 内容自动触发:

可用 skills 及适用场景:

  • general-pr-review - 通用代码审查:适用于 PyTorch/C++/Python PR 或单个提交的代码 review,关注正确性、回归风险、潜在 bug 和测试覆盖
  • xpu-pr-review - XPU/SYCL 专用代码审查:适用于 Intel GPU 相关改动,检查 CUDA 对等性、dispatch 注册、fallback 正确性、XPU 特定测试覆盖等
  • xpu-issue-triage - Issue 分类处理:帮助你判断 issue 是否可执行、缺少什么信息、如何分类问题及下一步建议
  • xpu-docstring - 文档编写/审查:帮助撰写或改进 XPU API 的 docstring 和简短说明
  • xpu-at-dispatch-v2 - Dispatch 迁移:处理 AT_DISPATCH_V2 迁移、宏清理或验证 XPU dispatch 转换是否正确
  • xpu-add-uint-support - 无符号整数支持:添加或审查 uint16/uint32/uint64 支持,检查缺失的 unsigned dispatch 覆盖

如何使用:

在 PR、issue 或 commit 评论中 @我,直接用自然语言提出你的需求即可,例如:

系统会根据你的问题自动选择最合适的 skill 来处理。如果问题跨越多个主题(比如既要 review 又要改 docstring),我会组合使用多个 skills(然后输出独立的检查项)。

@laifenxiawucha
Copy link
Copy Markdown

laifenxiawucha commented Apr 9, 2026

### no skills reply:
Thanks for the review request. Looking at this PR:

Overall Assessment

Despite being marked as "DO NOT MERGE [TEST ONLY]", this PR includes non-trivial code changes that introduce regressions and should not be in a test-only branch.

Issues Found

src/ATen/native/sparse/xpu/SparseSoftmax.cpp

  • The log_softmax_sparse_xpu function has been completely removed. There's no replacement visible in this diff, which deletes functionality.
  • The kernel call changed from softmax_sparse_xpu_kernel to softmax_forward_sparse_xpu_kernel - this API change needs to be verified against the kernel implementation.
  • This suggests an incomplete refactor that breaks the log_softmax API.

src/ATen/native/transformers/Attention.cpp

  • Removed the TORCH_CHECK that validated NestedTensor with mask is not supported. This removes a safety guard rather than adding the missing feature.
  • Changed qkv_weight.dim() validation from 2D to 4D. The comment "This aligns with CUDA" suggests this might be correct, but this appears to change the API contract without seeing callers updated.
  • These changes together seem like they're attempting to add NestedTensor support with 4D qkv_weight, but the implementation appears incomplete.

Recommendation

Either:

  1. Remove all code changes and keep only the AGENTS.md file (if this is truly a test PR), or
  2. Complete the refactor properly: restore log_softmax_sparse_xpu, update all callers of softmax_sparse_xpu, add proper NestedTensor mask support instead of just removing the check, and verify the 4D qkv_weight change propagates correctly through the stack.

The current state is not mergeable as-is.

@laifenxiawucha

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disable_all Disable all ci test jobs for the PR, just keep basic lint check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants