Support attn sink in fa3#1415
Conversation
risemeup1111
left a comment
There was a problem hiding this comment.
已完成初轮审查,发现需要先修复的阻塞问题。具体问题已放在行级评论中,主要涉及新增 FA3 sink 模块的导入可靠性、训练时 attention dropout 语义,以及 FA3 FlashMask 反向的 Paddle 版本兼容性。
CI 当前通过,但这些路径依赖特定设备/配置,建议修复后补充对应覆盖。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。上轮我指出的导入保护、FA3 dense dropout 显式拒绝、以及 FlashMask v2 grad 签名兼容性在当前代码里看起来已经修复;不过当前账号没有权限关闭已有 review 线程。
这轮发现 FA3 FlashMask/startend 分支的非零 dropout 仍会进入自定义 PyLayer,但 backward 没有对应 dropout 状态,详细说明已放在新的行级评论中。该问题建议合入前修复。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。上轮我指出的 FA3 FlashMask/startend 训练期非零 attention_dropout 问题已通过显式 guard 修复,本轮没有新增 Nyanpasu 行级评论。
当前仍有独立的 KV-cache decode 风险由已有行级讨论覆盖,我没有重复开新线程;请以该现有讨论继续处理。
04a308c to
5436109
Compare
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。Nyanpasu 之前提出的导入保护、FA3 grad 签名兼容和两处 dropout 静默语义问题在当前代码中已处理;但这轮发现两个仍需要修复后再合入的问题,细节已放在行级评论中。
请针对 P1 评论提交新的修复 commit;CI 当前通过,但这些路径需要对应回归测试覆盖。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。上轮 Nyanpasu 提出的 KV-cache decode fallback 和 additive dense mask 解析两个 P1 问题,在当前代码中均已通过新 commit 修复,并补充了对应回归覆盖。
本轮未发现新的需要阻塞合入的问题;CI 当前通过。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。本次变更只移除了 FA3 sink 实现中的调试注释,不改变已检查过的 fallback、mask 解析或 backward 逻辑。
上轮 Nyanpasu 已确认修复的 P1 问题在当前 head 仍保持修复状态;本轮未发现新的需要阻塞合入的问题,CI 当前通过。
819daa9 to
dd211fd
Compare
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。此前 Nyanpasu 提出的 additive mask 解析问题仍保持修复状态,但当前 KV-cache decode fallback 会在启用 attention sink 时静默退回到不带 sink 的 SDPA,导致 prefill/decode 语义不一致;我已在行级评论中给出需要修复的位置和建议方向。
请针对 P1 评论提交新的修复 commit 后再继续复查。CI 当前通过,但该路径还需要对应的 decode 语义覆盖。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。上轮指出的 KV-cache decode 静默丢 sink 已改为显式拒绝,方向是对的;但当前测试仍断言旧的 fallback 行为,且 FA3 sink 的 value head_dim padding 还缺少 v_head_dim > q_head_dim 边界处理。
新的 P1 问题已放在行级评论中,请提交修复 commit 后再复查。CI 当前通过,但这些路径需要对应覆盖保持一致。
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。上轮 Nyanpasu 提出的测试旧 fallback 断言和 v_head_dim > q_head_dim 边界问题,当前都已通过新 commit 修复,并补充了对应覆盖。
本轮未发现新的需要阻塞合入的问题;CI 当前通过。
PaddleFleet Log Analysis
日志分析报告
失败的测试case: 根本原因分析: PR #1415 新增了 FA3 attention sink 实现( 修复建议:
🔄 每次 Re-run 后自动更新 |
0db4394 to
1d354de
Compare
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。此前 KV-cache decode 测试旧断言仍保持修复;但 v_head_dim > q_head_dim 的 P1 边界在当前 DotProductAttention padding 条件里重新出现,我已在原有 head_dim 线程补充了当前证据和建议修复形态。
请针对该 P1 推送修复 commit 后再复查。CI 当前通过,但这个路径依赖 FA3/特定 head_dim 配置,现有结果不能覆盖该回归。
1d354de to
c2b2f5e
Compare
risemeup1111
left a comment
There was a problem hiding this comment.
已复查最新提交。此前 KV-cache decode 测试旧断言仍保持修复,但 v_head_dim > q_head_dim 的 P1 边界还没有被修复;这次提交只调整了相关代码格式,当前两个 padding 分支仍可能在显式 head_dim 拒绝之前进入 q_head_dim - v_head_dim 的错误 padding。
我已在原有 head_dim 讨论中补充当前证据。请针对该 P1 推送修复 commit 后再复查。
liuhao2638
left a comment
There was a problem hiding this comment.
已复查最新提交。此前 KV-cache decode 测试旧断言仍保持修复,但 head_dim padding 的 P1 仍未完成;另外这次提交在标准 SDPA 路径引入了新的未赋值 attn_output 回归,详细位置和建议修复已放在新的行级评论中。
请针对这些 P1 问题推送修复 commit 后再复查。CI 当前通过,但这些路径没有被现有检查覆盖到。
Co-authored-by: liuhao2638 <2284903223@qq.com>
PaddlePaddle-bot
left a comment
There was a problem hiding this comment.
🤖 Paddle-CI-Agent | pr_review |
2026-07-14 16:34:21
📋 Review 摘要
PR 概述:新增 FA3 attention sink 的 1F1B correction 实现,并在 DotProductAttention 中接入 FA3 sink 分支。
变更范围:transformer/dot_product_attention.py、新增 transformer/sink_impl.py、FA3/FA4 sink 单卡测试。
影响面 Tag:Transformer Attention
问题
| 级别 | 文件 | 概述 |
|---|---|---|
| 🟡 建议 | src/paddlefleet/transformer/dot_product_attention.py:734 |
dense/SDPA 场景仍绕过新增 FA3 sink 分发,新增 dense 用例没有覆盖新路径 |
历史 Findings 修复情况
| Finding | 问题 | 状态 |
|---|---|---|
| F1 | runtime 输入校验依赖 assert |
|
| F2 | FA3 sink 分支永远不会被触发 | ✅ 已修复 |
| F3 | use_startend=False 用例不会覆盖新增 FA3 sink 分支 |
📝 PR 规范检查
标题缺少规范 Tag;描述已包含 checklist §D2 要求的 PR Category、PR Types、Description 且均有内容。
标题建议(可直接复制):
[New features] Support FA3 attention sink总体评价
FA3 sink 的 FlashMask/startend 路径已经有实际分发和数值测试覆盖;但 dense/SDPA 场景仍停留在旧实现,与 PR 描述和新增测试类名不一致,建议补齐真实分发或收窄声明与测试。
| else: | ||
| flashmask_attention_func = flashmask_attention | ||
|
|
||
| use_fa3_sink, attn_mask_startend_row_indices = ( |
There was a problem hiding this comment.
🟡 建议 这里的 FA3 sink 分发只在 attn_mask_startend_row_indices is not None 的 FlashMask 分支里执行;当 attn_mask_startend_row_indices=None 时,上面的 SDPA 分支已经直接返回,新增的 sink_attention_forward 不会被调用。
这会导致 PR 描述里的 SDPA/dense 场景仍然走旧的 scaled_dot_product_attention_with_softmax_offset,prepare_fa3_sink_attention 里的 dense mask 转换也没有入口;新增的 dense causal 用例只能验证旧路径能跑,不能覆盖 FA3 sink 分支。
建议修复方式:把 FA3 sink 判定前移到 SDPA 分支内(传入 attention_mask/attn_mask_kv 并在 prepare_fa3_sink_attention 中转换 dense causal/full mask),或抽成两个分支共用的分发逻辑;同时让 dense 用例断言实际调用了 sink_attention_forward。
liuhao2638
left a comment
There was a problem hiding this comment.
已复查最新提交。上轮指出的标准 SDPA 路径 attn_output 未赋值问题已经通过新 commit 修复;KV-cache decode 测试旧断言也仍保持修复。
当前仍剩一个 P1:FA3 sink 的 v_head_dim > q_head_dim padding 边界还没有修完,我已在原有 head_dim 讨论中补充当前 head 的证据和建议修复形态。请针对该 P1 推送修复 commit 后再复查。
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (46.27%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #1415 +/- ##
==========================================
Coverage ? 46.27%
==========================================
Files ? 2
Lines ? 309
Branches ? 74
==========================================
Hits ? 143
Misses ? 137
Partials ? 29
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR Category
Operator Mechanism
PR Types
New features
Description
新增 FA3 attention sink 实现,并在 DotProductAttention 的 SDPA/FlashMask 路径中按条件分发到 FA3 sink 分支
是否引起精度变化
否