Fix Qwen3 attention mask NaNs#714
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplaces ChangesAttention Masking Fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request adds a new serving test to the GitHub Actions workflow and updates the Qwen3 14B model's decode and prefill forward layers to replace pl.fillpad with pl.full and pl.assemble when preparing attention scores. The review feedback points out that because scores is now a scratch tensor (TensorType), it must be loaded back as a Vec tile using pl.slice before calling vector instructions like pl.row_max and pl.row_expand_sub.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| cur_mi = pl.row_max(scores) | ||
| exp_scores = pl.exp(pl.row_expand_sub(scores, cur_mi)) |
There was a problem hiding this comment.
In PyPTO, vector instructions such as pl.row_max and pl.row_expand_sub require their arguments to be a Vec TileType, not a TensorType. Since scores is initialized via pl.full and updated via pl.assemble, it represents a TensorType (scratch tensor). To use it in vector operations, you must first load it back as a Vec tile using pl.slice.
| cur_mi = pl.row_max(scores) | |
| exp_scores = pl.exp(pl.row_expand_sub(scores, cur_mi)) | |
| scores_tile = pl.slice(scores, [Q_HEAD_PAD, SEQ_TILE], [0, 0]) | |
| cur_mi = pl.row_max(scores_tile) | |
| exp_scores = pl.exp(pl.row_expand_sub(scores_tile, cur_mi)) |
References
- In PyPTO, vector instructions require a Vec TileType rather than a TensorType. When assembling to a scratch tensor, the value must be loaded back as a Vec tile before being consumed by vector operations.
| cur_mi = pl.row_max(scores) | ||
| exp_scores = pl.exp(pl.row_expand_sub(scores, cur_mi)) |
There was a problem hiding this comment.
In PyPTO, vector instructions such as pl.row_max and pl.row_expand_sub require their arguments to be a Vec TileType, not a TensorType. Since scores is initialized via pl.full and updated via pl.assemble, it represents a TensorType (scratch tensor). To use it in vector operations, you must first load it back as a Vec tile using pl.slice.
| cur_mi = pl.row_max(scores) | |
| exp_scores = pl.exp(pl.row_expand_sub(scores, cur_mi)) | |
| scores_tile = pl.slice(scores, [Q_HEAD_BATCH_PAD, SEQ_TILE], [0, 0]) | |
| cur_mi = pl.row_max(scores_tile) | |
| exp_scores = pl.exp(pl.row_expand_sub(scores_tile, cur_mi)) |
References
- In PyPTO, vector instructions require a Vec TileType rather than a TensorType. When assembling to a scratch tensor, the value must be loaded back as a Vec tile before being consumed by vector operations.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
models/qwen3/14b/prefill_fwd.py (1)
121-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstant duplicated across two files.
ATTN_MASK_NEG_INFis defined identically here and indecode_layer.py. Worth hoisting to a shared module to avoid future drift between the two sentinel values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/qwen3/14b/prefill_fwd.py` at line 121, The ATT N mask sentinel is duplicated in both prefill_fwd.py and decode_layer.py, so hoist ATT N_MASK_NEG_INF into a shared module and update both callers to import the same constant. Use the existing ATT N_MASK_NEG_INF symbol in prefill_fwd.py and decode_layer.py as the reference points, and ensure both paths read the value from one source so the sentinel cannot drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@models/qwen3/14b/prefill_fwd.py`:
- Line 121: The ATT N mask sentinel is duplicated in both prefill_fwd.py and
decode_layer.py, so hoist ATT N_MASK_NEG_INF into a shared module and update
both callers to import the same constant. Use the existing ATT N_MASK_NEG_INF
symbol in prefill_fwd.py and decode_layer.py as the reference points, and ensure
both paths read the value from one source so the sentinel cannot drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f908f12-0f7e-4738-bcf8-7f86dda05a18
📒 Files selected for processing (3)
.github/actions/pypto-serving-tests/action.ymlmodels/qwen3/14b/decode_layer.pymodels/qwen3/14b/prefill_fwd.py
d00c0a5 to
6e6faef
Compare
|
Update: I withdrew the model-level workaround from this PR after validating the CI failure locally. Evidence:
Conclusion: this should be fixed in PyPTO core fillpad/PadValue lowering, not with a Qwen3 model-side rewrite in pypto-lib. The branch is now an empty diff against |
6e6faef to
d31081a
Compare
|
Update: re-tested the minimal clamp fix without Current patch:
Validation:
This keeps the original fillpad valid-shape path and only prevents the |
d31081a to
c724ae0
Compare
Keep the existing fillpad(PadValue.min) path, but clamp padded FP32 attention scores to the finite lowest FP32 value before row-wise softmax. This prevents fully padded rows from evaluating -inf - -inf and producing NaNs while preserving the original fillpad valid-shape behavior. Do not inject PTO2_RING_HEAP, PTO2_RING_TASK_WINDOW, or PTO2_RING_DEP_POOL in the pypto-serving CI action; the Qwen3 serving validation passes without those overrides and the overrides can trigger AICore 507018.
## Summary Fix intermittent pypto wheel-build failures on the self-hosted NPU jobs. ### Problem `a2a3` and `serving` both `runs-on: [self-hosted, linux, arm64, npu]`, only `needs: detect-changes`, so they **run concurrently** — and both sync + build pypto into the **same** `ci-cache/pypto-src` dir. When they reach the build stage together, one job's `reset --hard` / `rm -rf build` corrupts the other's in-flight libbacktrace `configure`, surfacing as **intermittent** wheel-build failures on unrelated PRs: - `configure: error: C compiler cannot create executables` - `configure: error: cannot run C compiled programs` - `elf.c:149: error: #error "Unknown BACKTRACE_ELF_SIZE"` Seen in [run 28866217182](https://github.com/hw-native-sys/pypto-lib/actions/runs/28866217182) (main) and [run 28867986356](https://github.com/hw-native-sys/pypto-lib/actions/runs/28867986356) (PR #714) — same failure, unrelated diffs. ### Fix - **Per-job build-tree isolation** via a new required `build-namespace` input on `setup-npu-device-job`; `ci-cache/pypto-src` becomes `ci-cache/pypto-src-<namespace>` (`a2a3` / `serving` / `a2a3-daily`), mirroring the sim job's per-platform isolation. ccache stays shared (content-addressed, concurrency-safe). - **Retry the native build** (3 attempts) so a transient conda `compiler_compat` hiccup self-heals. The retry purges the retrying target's own `build/` and `_skbuild/` first so a half-written configure state can't poison it. `build-namespace` is required so any new device-job caller must consciously pick an isolated namespace.
Summary
Related Issues
Fixes #713