feat(dsv4): split v4 into v4-flash + v4-pro, add dedicated A5 daily CI - #816
Merged
Merged
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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:
📝 WalkthroughWalkthroughDeepSeek V4 Flash and Pro implementations are added, including attention, compression, indexing, MoE, MTP, distributed forward paths, metadata utilities, golden test harnesses, documentation, and CI coverage with dedicated A5 Pro model sweeps. ChangesDeepSeek V4 implementation
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
models/deepseek/v4-pro/prefill_mtp.py (1)
330-350: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHarden the projection weight/scale cache against re-init and ordering.
init_e_proj_w(andinit_h_proj_w) unconditionally regeneratee_proj_cache, whereas the paired_scaleinitializers are lazy (if ... is None). Consistency of the INT8 weight and its dequant scale therefore depends on the_winitializer running exactly once and strictly before the_scaleinitializer. If the harness ever re-invokes aninit_value, or the spec order changes,_wproduces a fresh random pair while the already-materialized_scaleretains the old pair, silently desyncing weight/scale and breaking the golden match.Make the
_winitializer lazy and symmetric so weight and scale always come from the same generated pair:♻️ Proposed fix
def init_e_proj_w(): nonlocal e_proj_cache - e_proj_cache = init_proj_pair() + if e_proj_cache is None: + e_proj_cache = init_proj_pair() return e_proj_cache[0](apply the same to
init_h_proj_w)🤖 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/deepseek/v4-pro/prefill_mtp.py` around lines 330 - 350, Make init_e_proj_w and init_h_proj_w lazy like their paired scale initializers: only call init_proj_pair and populate the cache when the corresponding cache is None, then return the cached weight. Preserve the existing scale initializer behavior so repeated or reordered initialization always returns the weight and scale from the same generated pair.models/deepseek/v4-flash/decode_metadata.py (1)
186-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
paged_slot_mappingduplicatesori_slot_mappingverbatim.The body of
paged_slot_mapping(Lines 192-200) is identical toori_slot_mapping(Lines 175-183). Consider having one delegate to the other (or keeping a single implementation with an alias) so the paged-KV lowering contract stays defined in one place.♻️ Suggested consolidation
def paged_slot_mapping( positions: torch.Tensor, table: torch.Tensor, *, block_size: int = BLOCK_SIZE, ) -> torch.Tensor: - positions_i64 = positions.to(torch.int64) - table_i64 = table.to(device=positions.device, dtype=torch.int64) - logical_blk = positions_i64 // block_size - intra = positions_i64 % block_size - in_bounds = logical_blk < table_i64.shape[1] - clamped_blk = torch.clamp(logical_blk, max=table_i64.shape[1] - 1) - blk = torch.gather(table_i64, 1, clamped_blk) - valid = in_bounds & (blk >= 0) - return torch.where(valid, blk * block_size + intra, -1) + return ori_slot_mapping(positions, table, block_size=block_size)🤖 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/deepseek/v4-flash/decode_metadata.py` around lines 186 - 200, Consolidate the duplicate slot-mapping logic in paged_slot_mapping and ori_slot_mapping so only one function owns the implementation. Make the other function delegate to or alias that implementation while preserving the existing arguments, tensor behavior, and paged-KV lowering contract.
🤖 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.
Inline comments:
In @.github/workflows/daily_ci.yml:
- Around line 288-301: Add a least-privilege permissions configuration for the
model-tests-a5 job, granting only contents: read at job level alongside its
runs-on settings. Prefer a top-level permissions block if it can safely cover
all device jobs without changing required access.
In `@models/deepseek/v4-flash/decode_sparse_attn_hca.py`:
- Line 688: Update the online-softmax merge zip calls to pass strict=True,
preserving the existing sliced iterables:
models/deepseek/v4-flash/decode_sparse_attn_hca.py:688 and
models/deepseek/v4-flash/decode_sparse_attn_swa.py:594. Apply the same change at
both sites to enforce equal-length inputs and prevent silent truncation.
In `@models/deepseek/v4-flash/decode_sparse_attn.py`:
- Around line 735-741: Update the zip call in the score accumulation loop over
block_mi, block_li, and block_oi to pass strict=True, preserving the existing
iteration and equal-length invariant.
In `@models/deepseek/v4-pro/decode_sparse_attn_hca.py`:
- Line 688: Update the zip call in the loop over block_mi, block_li, and
block_oi to pass strict=True, preserving the existing lockstep iteration while
explicitly enforcing equal lengths and satisfying Ruff B905.
---
Nitpick comments:
In `@models/deepseek/v4-flash/decode_metadata.py`:
- Around line 186-200: Consolidate the duplicate slot-mapping logic in
paged_slot_mapping and ori_slot_mapping so only one function owns the
implementation. Make the other function delegate to or alias that implementation
while preserving the existing arguments, tensor behavior, and paged-KV lowering
contract.
In `@models/deepseek/v4-pro/prefill_mtp.py`:
- Around line 330-350: Make init_e_proj_w and init_h_proj_w lazy like their
paired scale initializers: only call init_proj_pair and populate the cache when
the corresponding cache is None, then return the cached weight. Preserve the
existing scale initializer behavior so repeated or reordered initialization
always returns the weight and scale from the same generated pair.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7efb2201-32ff-4504-9421-10ea3aa8660f
📒 Files selected for processing (83)
.claude/skills/test-with-golden/SKILL.md.github/workflows/ci.yml.github/workflows/daily_ci.ymlREADME.mddocs/compile-runtime-workflow.mddocs/debugging.mddocs/performance-tuning.mdmodels/deepseek/v4-flash/config.pymodels/deepseek/v4-flash/decode_attention_csa.pymodels/deepseek/v4-flash/decode_attention_hca.pymodels/deepseek/v4-flash/decode_attention_swa.pymodels/deepseek/v4-flash/decode_compressor_ratio128.pymodels/deepseek/v4-flash/decode_compressor_ratio4.pymodels/deepseek/v4-flash/decode_fwd.pymodels/deepseek/v4-flash/decode_indexer.pymodels/deepseek/v4-flash/decode_indexer_compressor.pymodels/deepseek/v4-flash/decode_layer.pymodels/deepseek/v4-flash/decode_metadata.pymodels/deepseek/v4-flash/decode_mtp.pymodels/deepseek/v4-flash/decode_sparse_attn.pymodels/deepseek/v4-flash/decode_sparse_attn_hca.pymodels/deepseek/v4-flash/decode_sparse_attn_swa.pymodels/deepseek/v4-flash/expert_routed.pymodels/deepseek/v4-flash/expert_shared.pymodels/deepseek/v4-flash/gate.pymodels/deepseek/v4-flash/hc_head.pymodels/deepseek/v4-flash/hc_post.pymodels/deepseek/v4-flash/hc_pre.pymodels/deepseek/v4-flash/lm_head.pymodels/deepseek/v4-flash/moe.pymodels/deepseek/v4-flash/mtp_projection.pymodels/deepseek/v4-flash/prefill_attention_csa.pymodels/deepseek/v4-flash/prefill_attention_hca.pymodels/deepseek/v4-flash/prefill_attention_swa.pymodels/deepseek/v4-flash/prefill_compressor_ratio128.pymodels/deepseek/v4-flash/prefill_compressor_ratio4.pymodels/deepseek/v4-flash/prefill_fwd.pymodels/deepseek/v4-flash/prefill_indexer.pymodels/deepseek/v4-flash/prefill_indexer_compressor.pymodels/deepseek/v4-flash/prefill_layer.pymodels/deepseek/v4-flash/prefill_mtp.pymodels/deepseek/v4-flash/prefill_sparse_attn.pymodels/deepseek/v4-flash/qkv_proj_rope.pymodels/deepseek/v4-flash/rmsnorm.pymodels/deepseek/v4-flash/rope_tables.pymodels/deepseek/v4-pro/config.pymodels/deepseek/v4-pro/decode_attention_csa.pymodels/deepseek/v4-pro/decode_attention_hca.pymodels/deepseek/v4-pro/decode_attention_swa.pymodels/deepseek/v4-pro/decode_compressor_ratio128.pymodels/deepseek/v4-pro/decode_compressor_ratio4.pymodels/deepseek/v4-pro/decode_fwd.pymodels/deepseek/v4-pro/decode_indexer.pymodels/deepseek/v4-pro/decode_indexer_compressor.pymodels/deepseek/v4-pro/decode_layer.pymodels/deepseek/v4-pro/decode_metadata.pymodels/deepseek/v4-pro/decode_mtp.pymodels/deepseek/v4-pro/decode_sparse_attn.pymodels/deepseek/v4-pro/decode_sparse_attn_hca.pymodels/deepseek/v4-pro/decode_sparse_attn_swa.pymodels/deepseek/v4-pro/expert_routed.pymodels/deepseek/v4-pro/expert_shared.pymodels/deepseek/v4-pro/gate.pymodels/deepseek/v4-pro/hc_head.pymodels/deepseek/v4-pro/hc_post.pymodels/deepseek/v4-pro/hc_pre.pymodels/deepseek/v4-pro/lm_head.pymodels/deepseek/v4-pro/moe.pymodels/deepseek/v4-pro/mtp_projection.pymodels/deepseek/v4-pro/prefill_attention_csa.pymodels/deepseek/v4-pro/prefill_attention_hca.pymodels/deepseek/v4-pro/prefill_attention_swa.pymodels/deepseek/v4-pro/prefill_compressor_ratio128.pymodels/deepseek/v4-pro/prefill_compressor_ratio4.pymodels/deepseek/v4-pro/prefill_fwd.pymodels/deepseek/v4-pro/prefill_indexer.pymodels/deepseek/v4-pro/prefill_indexer_compressor.pymodels/deepseek/v4-pro/prefill_layer.pymodels/deepseek/v4-pro/prefill_mtp.pymodels/deepseek/v4-pro/prefill_sparse_attn.pymodels/deepseek/v4-pro/qkv_proj_rope.pymodels/deepseek/v4-pro/rmsnorm.pymodels/deepseek/v4-pro/rope_tables.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (2)
models/deepseek/v4-pro/prefill_mtp.py (1)
330-350: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHarden the projection weight/scale cache against re-init and ordering.
init_e_proj_w(andinit_h_proj_w) unconditionally regeneratee_proj_cache, whereas the paired_scaleinitializers are lazy (if ... is None). Consistency of the INT8 weight and its dequant scale therefore depends on the_winitializer running exactly once and strictly before the_scaleinitializer. If the harness ever re-invokes aninit_value, or the spec order changes,_wproduces a fresh random pair while the already-materialized_scaleretains the old pair, silently desyncing weight/scale and breaking the golden match.Make the
_winitializer lazy and symmetric so weight and scale always come from the same generated pair:♻️ Proposed fix
def init_e_proj_w(): nonlocal e_proj_cache - e_proj_cache = init_proj_pair() + if e_proj_cache is None: + e_proj_cache = init_proj_pair() return e_proj_cache[0](apply the same to
init_h_proj_w)🤖 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/deepseek/v4-pro/prefill_mtp.py` around lines 330 - 350, Make init_e_proj_w and init_h_proj_w lazy like their paired scale initializers: only call init_proj_pair and populate the cache when the corresponding cache is None, then return the cached weight. Preserve the existing scale initializer behavior so repeated or reordered initialization always returns the weight and scale from the same generated pair.models/deepseek/v4-flash/decode_metadata.py (1)
186-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
paged_slot_mappingduplicatesori_slot_mappingverbatim.The body of
paged_slot_mapping(Lines 192-200) is identical toori_slot_mapping(Lines 175-183). Consider having one delegate to the other (or keeping a single implementation with an alias) so the paged-KV lowering contract stays defined in one place.♻️ Suggested consolidation
def paged_slot_mapping( positions: torch.Tensor, table: torch.Tensor, *, block_size: int = BLOCK_SIZE, ) -> torch.Tensor: - positions_i64 = positions.to(torch.int64) - table_i64 = table.to(device=positions.device, dtype=torch.int64) - logical_blk = positions_i64 // block_size - intra = positions_i64 % block_size - in_bounds = logical_blk < table_i64.shape[1] - clamped_blk = torch.clamp(logical_blk, max=table_i64.shape[1] - 1) - blk = torch.gather(table_i64, 1, clamped_blk) - valid = in_bounds & (blk >= 0) - return torch.where(valid, blk * block_size + intra, -1) + return ori_slot_mapping(positions, table, block_size=block_size)🤖 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/deepseek/v4-flash/decode_metadata.py` around lines 186 - 200, Consolidate the duplicate slot-mapping logic in paged_slot_mapping and ori_slot_mapping so only one function owns the implementation. Make the other function delegate to or alias that implementation while preserving the existing arguments, tensor behavior, and paged-KV lowering contract.
🤖 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.
Inline comments:
In @.github/workflows/daily_ci.yml:
- Around line 288-301: Add a least-privilege permissions configuration for the
model-tests-a5 job, granting only contents: read at job level alongside its
runs-on settings. Prefer a top-level permissions block if it can safely cover
all device jobs without changing required access.
In `@models/deepseek/v4-flash/decode_sparse_attn_hca.py`:
- Line 688: Update the online-softmax merge zip calls to pass strict=True,
preserving the existing sliced iterables:
models/deepseek/v4-flash/decode_sparse_attn_hca.py:688 and
models/deepseek/v4-flash/decode_sparse_attn_swa.py:594. Apply the same change at
both sites to enforce equal-length inputs and prevent silent truncation.
In `@models/deepseek/v4-flash/decode_sparse_attn.py`:
- Around line 735-741: Update the zip call in the score accumulation loop over
block_mi, block_li, and block_oi to pass strict=True, preserving the existing
iteration and equal-length invariant.
In `@models/deepseek/v4-pro/decode_sparse_attn_hca.py`:
- Line 688: Update the zip call in the loop over block_mi, block_li, and
block_oi to pass strict=True, preserving the existing lockstep iteration while
explicitly enforcing equal lengths and satisfying Ruff B905.
---
Nitpick comments:
In `@models/deepseek/v4-flash/decode_metadata.py`:
- Around line 186-200: Consolidate the duplicate slot-mapping logic in
paged_slot_mapping and ori_slot_mapping so only one function owns the
implementation. Make the other function delegate to or alias that implementation
while preserving the existing arguments, tensor behavior, and paged-KV lowering
contract.
In `@models/deepseek/v4-pro/prefill_mtp.py`:
- Around line 330-350: Make init_e_proj_w and init_h_proj_w lazy like their
paired scale initializers: only call init_proj_pair and populate the cache when
the corresponding cache is None, then return the cached weight. Preserve the
existing scale initializer behavior so repeated or reordered initialization
always returns the weight and scale from the same generated pair.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7efb2201-32ff-4504-9421-10ea3aa8660f
📒 Files selected for processing (83)
.claude/skills/test-with-golden/SKILL.md.github/workflows/ci.yml.github/workflows/daily_ci.ymlREADME.mddocs/compile-runtime-workflow.mddocs/debugging.mddocs/performance-tuning.mdmodels/deepseek/v4-flash/config.pymodels/deepseek/v4-flash/decode_attention_csa.pymodels/deepseek/v4-flash/decode_attention_hca.pymodels/deepseek/v4-flash/decode_attention_swa.pymodels/deepseek/v4-flash/decode_compressor_ratio128.pymodels/deepseek/v4-flash/decode_compressor_ratio4.pymodels/deepseek/v4-flash/decode_fwd.pymodels/deepseek/v4-flash/decode_indexer.pymodels/deepseek/v4-flash/decode_indexer_compressor.pymodels/deepseek/v4-flash/decode_layer.pymodels/deepseek/v4-flash/decode_metadata.pymodels/deepseek/v4-flash/decode_mtp.pymodels/deepseek/v4-flash/decode_sparse_attn.pymodels/deepseek/v4-flash/decode_sparse_attn_hca.pymodels/deepseek/v4-flash/decode_sparse_attn_swa.pymodels/deepseek/v4-flash/expert_routed.pymodels/deepseek/v4-flash/expert_shared.pymodels/deepseek/v4-flash/gate.pymodels/deepseek/v4-flash/hc_head.pymodels/deepseek/v4-flash/hc_post.pymodels/deepseek/v4-flash/hc_pre.pymodels/deepseek/v4-flash/lm_head.pymodels/deepseek/v4-flash/moe.pymodels/deepseek/v4-flash/mtp_projection.pymodels/deepseek/v4-flash/prefill_attention_csa.pymodels/deepseek/v4-flash/prefill_attention_hca.pymodels/deepseek/v4-flash/prefill_attention_swa.pymodels/deepseek/v4-flash/prefill_compressor_ratio128.pymodels/deepseek/v4-flash/prefill_compressor_ratio4.pymodels/deepseek/v4-flash/prefill_fwd.pymodels/deepseek/v4-flash/prefill_indexer.pymodels/deepseek/v4-flash/prefill_indexer_compressor.pymodels/deepseek/v4-flash/prefill_layer.pymodels/deepseek/v4-flash/prefill_mtp.pymodels/deepseek/v4-flash/prefill_sparse_attn.pymodels/deepseek/v4-flash/qkv_proj_rope.pymodels/deepseek/v4-flash/rmsnorm.pymodels/deepseek/v4-flash/rope_tables.pymodels/deepseek/v4-pro/config.pymodels/deepseek/v4-pro/decode_attention_csa.pymodels/deepseek/v4-pro/decode_attention_hca.pymodels/deepseek/v4-pro/decode_attention_swa.pymodels/deepseek/v4-pro/decode_compressor_ratio128.pymodels/deepseek/v4-pro/decode_compressor_ratio4.pymodels/deepseek/v4-pro/decode_fwd.pymodels/deepseek/v4-pro/decode_indexer.pymodels/deepseek/v4-pro/decode_indexer_compressor.pymodels/deepseek/v4-pro/decode_layer.pymodels/deepseek/v4-pro/decode_metadata.pymodels/deepseek/v4-pro/decode_mtp.pymodels/deepseek/v4-pro/decode_sparse_attn.pymodels/deepseek/v4-pro/decode_sparse_attn_hca.pymodels/deepseek/v4-pro/decode_sparse_attn_swa.pymodels/deepseek/v4-pro/expert_routed.pymodels/deepseek/v4-pro/expert_shared.pymodels/deepseek/v4-pro/gate.pymodels/deepseek/v4-pro/hc_head.pymodels/deepseek/v4-pro/hc_post.pymodels/deepseek/v4-pro/hc_pre.pymodels/deepseek/v4-pro/lm_head.pymodels/deepseek/v4-pro/moe.pymodels/deepseek/v4-pro/mtp_projection.pymodels/deepseek/v4-pro/prefill_attention_csa.pymodels/deepseek/v4-pro/prefill_attention_hca.pymodels/deepseek/v4-pro/prefill_attention_swa.pymodels/deepseek/v4-pro/prefill_compressor_ratio128.pymodels/deepseek/v4-pro/prefill_compressor_ratio4.pymodels/deepseek/v4-pro/prefill_fwd.pymodels/deepseek/v4-pro/prefill_indexer.pymodels/deepseek/v4-pro/prefill_indexer_compressor.pymodels/deepseek/v4-pro/prefill_layer.pymodels/deepseek/v4-pro/prefill_mtp.pymodels/deepseek/v4-pro/prefill_sparse_attn.pymodels/deepseek/v4-pro/qkv_proj_rope.pymodels/deepseek/v4-pro/rmsnorm.pymodels/deepseek/v4-pro/rope_tables.py
🛑 Comments failed to post (4)
.github/workflows/daily_ci.yml (1)
288-301: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add an explicit least-privilege
permissions:block tomodel-tests-a5.The new job runs with the workflow's default
GITHUB_TOKENpermissions (nopermissions:block), which can be broad depending on repo settings. This job only needs to read the repo and write artifacts, so scope it down.🔒 Suggested job-level permissions
model-tests-a5: runs-on: [self-hosted, linux, arm64, npu-a5] permissions: contents: read timeout-minutes: 120Note: the other device jobs in this file share the same gap; consider setting a top-level
permissions:once instead.🧰 Tools
🪛 zizmor (1.26.1)
[warning] 288-425: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/daily_ci.yml around lines 288 - 301, Add a least-privilege permissions configuration for the model-tests-a5 job, granting only contents: read at job level alongside its runs-on settings. Prefer a top-level permissions block if it can safely cover all device jobs without changing required access.Source: Linters/SAST tools
models/deepseek/v4-flash/decode_sparse_attn_hca.py (1)
688-688: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add explicit
strict=to the online-softmax mergezip()(B905). Both golden references iterateblock_mi[1:],block_li[1:],block_oi[1:]with a barezip(); the lists are same-length today, butstrict=Truedocuments that invariant and prevents silent truncation on future refactors.
models/deepseek/v4-flash/decode_sparse_attn_hca.py#L688-L688: changezip(block_mi[1:], block_li[1:], block_oi[1:])to passstrict=True.models/deepseek/v4-flash/decode_sparse_attn_swa.py#L594-L594: apply the samestrict=Trueon the identicalzip(...).🧰 Tools
🪛 Ruff (0.15.21)
[warning] 688-688:
zip()without an explicitstrict=parameterAdd explicit value for parameter
strict=(B905)
📍 Affects 2 files
models/deepseek/v4-flash/decode_sparse_attn_hca.py#L688-L688(this comment)models/deepseek/v4-flash/decode_sparse_attn_swa.py#L594-L594🤖 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/deepseek/v4-flash/decode_sparse_attn_hca.py` at line 688, Update the online-softmax merge zip calls to pass strict=True, preserving the existing sliced iterables: models/deepseek/v4-flash/decode_sparse_attn_hca.py:688 and models/deepseek/v4-flash/decode_sparse_attn_swa.py:594. Apply the same change at both sites to enforce equal-length inputs and prevent silent truncation.Source: Linters/SAST tools
models/deepseek/v4-flash/decode_sparse_attn.py (1)
735-741: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add explicit
strict=tozip()(Ruff B905).The three lists are equal-length by construction, so
strict=Truedocuments that invariant and satisfies the linter.- for mi_cur, li_cur, oi_cur in zip(block_mi[1:], block_li[1:], block_oi[1:]): + for mi_cur, li_cur, oi_cur in zip(block_mi[1:], block_li[1:], block_oi[1:], strict=True):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for mi_cur, li_cur, oi_cur in zip(block_mi[1:], block_li[1:], block_oi[1:], strict=True): score_max_new = torch.maximum(score_max, mi_cur) alpha = torch.exp(score_max - score_max_new) beta = torch.exp(mi_cur - score_max_new) li = alpha * li + beta * li_cur oi_num = alpha * oi_num + beta * oi_cur score_max = score_max_new🧰 Tools
🪛 Ruff (0.15.21)
[warning] 735-735:
zip()without an explicitstrict=parameterAdd explicit value for parameter
strict=(B905)
🤖 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/deepseek/v4-flash/decode_sparse_attn.py` around lines 735 - 741, Update the zip call in the score accumulation loop over block_mi, block_li, and block_oi to pass strict=True, preserving the existing iteration and equal-length invariant.Source: Linters/SAST tools
models/deepseek/v4-pro/decode_sparse_attn_hca.py (1)
688-688: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add
strict=Truetozip(). Ruff (B905) flags this call. The three lists are built in lockstep so lengths match; making it explicit both silences the lint and documents the invariant.Proposed fix
- for mi_cur, li_cur, oi_cur in zip(block_mi[1:], block_li[1:], block_oi[1:]): + for mi_cur, li_cur, oi_cur in zip(block_mi[1:], block_li[1:], block_oi[1:], strict=True):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for mi_cur, li_cur, oi_cur in zip(block_mi[1:], block_li[1:], block_oi[1:], strict=True):🧰 Tools
🪛 Ruff (0.15.21)
[warning] 688-688:
zip()without an explicitstrict=parameterAdd explicit value for parameter
strict=(B905)
🤖 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/deepseek/v4-pro/decode_sparse_attn_hca.py` at line 688, Update the zip call in the loop over block_mi, block_li, and block_oi to pass strict=True, preserving the existing lockstep iteration while explicitly enforcing equal lengths and satisfying Ruff B905.Source: Linters/SAST tools
lwDavid
force-pushed
the
fix/hc_pre-a5-guard
branch
2 times, most recently
from
July 22, 2026 08:02
0ae7d25 to
e3eff9c
Compare
Rename models/deepseek/v4 -> v4-flash (git mv, history preserved) and add models/deepseek/v4-pro as a byte-identical copy -- the A5 (Ascend 950) targeted variant. Internal imports are bare module names resolved per-directory, so the rename is import-safe; the generic detect_changes.py walker auto-discovers v4-flash. Update every external path reference to v4-flash (README, docs, test-with-golden skill). The ci.yml serving-test arming regex now matches ^models/deepseek/v4-(flash|pro)/ so a PR touching either variant arms the e2e serving test. Logical names (model-name deepseek-v4, build_deepseek_v4_rope_tables) are unchanged. v4-pro is the A5-only variant, so detect_changes.py excludes it from PR sim/a2a3 selection (PR CI has no A5 runner; on 910B it would just duplicate v4-flash). It is covered by the dedicated daily A5 job instead. daily_ci.yml: add model-tests-a5 -- a real Ascend 950 device job on the npu-a5 self-hosted runner that runs -p a5 over only models/deepseek/v4-pro, on the shared setup-ci-job (needs-device: true). v4-pro is excluded from the a2a3/a2a3sim/a5sim sweeps. The summary gains a second "DeepSeek V4-Pro (A5)" table (best-effort; the summary does not depend on model-tests-a5, so daily CI keeps producing until the npu-a5 runner exists). extract_perf mirrors the a2a3 job (effective_us mean=). Also lift the hc_pre A5 guard: the "separate" impl (data-derived pl.spmd(work_count) + CORE_GROUP + atomic-add, same pattern as hc_head/hc_post) is core-count-agnostic, so allow it on A5 and only reject the 910B-specific "syncall" impl there.
lwDavid
force-pushed
the
fix/hc_pre-a5-guard
branch
from
July 22, 2026 08:27
e3eff9c to
3372ad9
Compare
lwDavid
added a commit
that referenced
this pull request
Jul 30, 2026
model-tests-a5 has never executed a test since it was added in #816: every scheduled run died in ~25s in setup-ci-job's first step. Behind that error the npu-a5-1 runner turned out to violate six points of the setup contract, each invisible until the previous one was cleared. Five are fixed here, all verified on that runner (a full 35-case V4-Pro sweep now completes on the device): - npu-smi is a diagnostic, not a gate. It exits 187 on A5 — that driver generation ships no DCMI component, so it can never succeed there — and the step ran under `bash -e`. Warn and continue; device work goes through task-submit, which does its own locking. - The conda env name is overridable via CI_CONDA_ENV instead of being pinned to py310-lib, and a miss now lists the envs that do exist. - A missing ccache drops the compile cache rather than breaking the build. With CMAKE_*_COMPILER_LAUNCHER set and no ccache, CMake's compiler check reports "C compiler cannot create executables" — the same symptom the retry loop exists to paper over, with no hint of the real cause. - Add needs-sim-toolchain, so a device-only job need not supply the GCC >= 15 that simpler hardcodes for simulator kernels. kernel_compiler.py maps a2a3/a5 to CCEC and only a2a3sim/a5sim to HOST_GXX_15, and the runtime builds through GxxToolchain at C++17, so the requirement excluded hosts for nothing. It defaults to 'true', leaving every existing caller strict. - Seed simpler's managed pto-isa checkout from the clone this action already makes at the same pin. simpler keeps it at <simpler-src>/build/pto-isa and re-clones it over the network whenever it is absent, which is every run since the pypto sync step wipes $PYPTO_SRC/runtime/build. Seeding removes that clone everywhere, and is required on the A5 host: activate.sh prepends the conda env's lib dir to LD_LIBRARY_PATH for ptoas's GLIBCXX, conda's libcrypto then shadows the system OpenSSL that /usr/lib64/libldap.so.2 resolves against, and git-remote-https dies with `undefined symbol: EVP_md2`. The sixth is host-side and cannot be fixed here: CANN_ROOT names a toolkit that is not installed. A wrong value now lists the toolkits that are, so the .env fix is copy-pasteable. The sweep is report-only for now. A measured run puts V4-Pro at 13/35 on A5 — 13 aclrtSync device faults, 2 rtMalloc failures, 7 numeric — all pypto/device side rather than defects in this repo, so gating on them would paint the workflow red nightly, which is exactly what has made model-tests-sim's daily failure invisible. The V4-Pro table is published either way, and summary now depends on the job so a missing artifact is reported instead of rendering as an empty section.
lwDavid
added a commit
that referenced
this pull request
Jul 31, 2026
model-tests-a5 died in setup on every run, most recently at Check NPU with
npu-smi exiting 187 and "dcmi module initialize failed. ret is -8005".
That is permission denial, not a missing DCMI component. Since the A5 host's
driver upgrade the device nodes are crw-rw---- HwHiAiUser:HwHiAiUser and the CI
user is not in that group, so dcmi_init cannot open the character device —
npu-smi even lists "Failed to open the character device" among its own
candidate causes. task-submit drops privileges into the HwHiAiUser group, which
is how every model run reaches a card in the first place.
Direct invocation is still tried first, so hosts whose device nodes are
world-readable — the 910B runners — behave exactly as before and borrow no
card.
Two details the retry needs, both found by running it:
- The payload ends on `rc=$?; exit $rc` rather than on `npu-smi info`. With
the probe as the bare last command task-submit reports 215 even though
npu-smi returned 0 (reproduced 3/3 on an A5 host); setting the status
explicitly makes the outer code npu-smi's own. Verified to pass 0, 9 and
127 through unchanged, so an absent card still fails the step.
- task-submit leases a real card, so a busy queue makes it give up on the
wait and exit 1 having run nothing. That says nothing about the NPU, and it
failed the job just as the original probe did. The NPUSMI_RC sentinel
separates the two: present means the probe ran and its status is
authoritative, absent means the task never got scheduled, which warns and
continues. Every model run goes through task-submit anyway and fails
loudly when there is no device.
With this the A5 sweep completes for the first time since it was added in #816.
zhangqi-chen
pushed a commit
that referenced
this pull request
Jul 31, 2026
`model-tests-a5` has not executed a single test since it was added in #816 — every scheduled run died in setup. This gets the DeepSeek V4-Pro sweep running and its results into the daily report. ## Why it never ran The job failed in `setup-ci-job` on every run, and each failure hid the next one behind it. Most were host provisioning and have since been fixed on the `npu-a5-1` runner (`CANN_ROOT` pointed at a CANN that was not installed; the conda env, GCC 15 and ccache were absent). One is a real repo bug and is fixed here. **`Check NPU` ran `npu-smi info` directly, which cannot work on that host.** It exits 187 with `dcmi module initialize failed. ret is -8005`, and the step runs under `bash -e`. That reads like a missing DCMI component, but it is permission denial: since the driver upgrade the device nodes are `crw-rw---- HwHiAiUser:HwHiAiUser` and the CI user is not in that group, so `dcmi_init` cannot open the character device. npu-smi even lists *"Failed to open the character device"* among its own candidate causes. ``` $ ls -l /dev/davinci0 crw-rw---- 1 HwHiAiUser HwHiAiUser 235, 0 /dev/davinci0 $ npu-smi info -> exit 187 $ task-submit --run 'npu-smi info' -> exit 0, lists all 8 NPUs ``` `task-submit` drops privileges into the HwHiAiUser group — which is how every model run reaches a card in the first place — so the check now falls back to it. **Direct invocation is tried first, so the 910B runners behave exactly as before and borrow no card.** Two details the retry needs, both found by running it rather than reasoning about it: - The payload ends on `rc=$?; exit $rc` rather than on `npu-smi info`. With the probe as the bare last command, task-submit reports **215** even though npu-smi returned 0 (reproduced 3/3). Setting the status explicitly makes the outer code npu-smi's own — verified to pass 0, 9 and 127 through unchanged, so an absent card still fails the step. - task-submit leases a real card, so a busy queue makes it give up on the wait and exit 1 having run nothing. That says nothing about the NPU, yet it failed the job just as the original probe did. An `NPUSMI_RC` sentinel separates the two: present means the probe ran and its status is authoritative; absent means the task never got scheduled, which warns and continues. ## Getting the results into the report Even once the sweep ran, its results did not appear. The summary did not `needs` the job, so it was built as soon as the sim and a2a3 sweeps finished — and A5 is the longest of the four: | Job | finished | | --- | -------- | | `model-tests-sim` (both) | 01:32 / 01:33 | | `model-tests-a2a3` | 01:57 | | `Aggregate results summary` | **02:23** ⬅️ built here | | `model-tests-a5` | **02:47** | So `results-a5` never existed in time and the A5 section always rendered its "no artifact" placeholder. Adding it to `needs` fixes that (`always()` still lets the summary run when a sweep fails); the summary now starts three seconds after the A5 job ends. A5 also gets **its own table** rather than a column in the shared one. The case sets are disjoint by construction — `model-tests-a5` sweeps only `models/deepseek/v4-pro` and the other three jobs exclude it — so a single matrix made every row half filler, and no row is worth comparing across both. ``` ## Daily CI Model Test Results ### a2a3 and simulators | Case | a2a3 | a2a3 effective (us) | a2a3sim | a5sim | ... Passed: **a2a3** 48/48, **a2a3sim** 20/41, **a5sim** 17/41 ### a5 (DeepSeek V4-Pro) | Case | a5 | a5 effective (us) | | `models/deepseek/v4-pro/rmsnorm.py` | ✅ | 43.4 | | `models/deepseek/v4-pro/gate.py` | ❌ | 93.2 | ... Passed: **a5** 13/35 ``` Each table carries a per-platform passed count and names any platform whose artifact is missing, so an absent sweep is never mistaken for a set of skipped cases — which is precisely what fooled us here. ## Report-only, for now The A5 job is `continue-on-error: true`. A measured sweep puts V4-Pro at **13/35**: 13 cases die with `sync_stream_pair` device faults, 2 with `rtMalloc failed: 207001`, 7 on numerics. Those are pypto/device-side rather than defects in this repo, so gating on them would paint the workflow red every night — exactly what has made `model-tests-sim`'s daily failure invisible. The table is published either way, so the coverage is real even though the check does not block. Flip it off once the A5 blockers land. ## Verification Exercised end to end on the real `npu-a5-1` runner across several runs: - The full 35-case sweep completes in ~44-58 min against the job's 120-minute budget; slowest case `decode_fwd` at 417s, inside `--max-time 900`, and nothing hit the cap — **no timeout changes needed**. - `13/35` reproduced on three separate runs. - `--device auto --device-num 2` schedules fine here (`got cards: 2,3` in 2s), so the eight `# ci: devices=2` V4-Pro files are safe. - Summary rendering checked against the real results of runs 30595513890 and 30599291163, and with the `a5`, `a5sim`, and all artifacts removed in turn. - `ci.yml` is untouched; the 910B `sim` / `a2a3` jobs ran green through the modified shared action. --- Supersedes #859, which GitHub closed when its branch was renamed.
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Splits
models/deepseek/v4into two variants and gives the A5 (Ascend 950) variant its own daily on-device CI. Built on top of latestmain(includes #800'ssetup-ci-jobrestructure and #803'sdsv4/moedep-edge change — the rename carries both intov4-flashcleanly).Changes
models/deepseek/v4→v4-flash(git mv, history preserved) and addmodels/deepseek/v4-proas a byte-identical copy — the A5-targeted variant. Internal imports are bare module names resolved per-directory, so the rename is import-safe; the genericdetect_changes.pyauto-discovers both dirs.v4-flash(README, docs, test-with-golden skill).ci.ymlserving-test arming regex →^models/deepseek/v4-(flash|pro)/so a PR touching either variant arms the e2e serving test. Logical names (model-name deepseek-v4,tests/test_deepseek_v4_accuracy.py,build_deepseek_v4_rope_tables) are unchanged.daily_ci.ymlmodel-tests-a5: a real Ascend 950 device job on thenpu-a5self-hosted runner that runs-p a5over onlymodels/deepseek/v4-pro, built on the sharedsetup-ci-job(needs-device: true, no hardcoded paths — the A5 host's.envsuppliesCANN_ROOT/cache roots).v4-prois excluded from the a2a3/a2a3sim/a5sim sweeps to avoid running the A5 variant on 910B. The summary gains a second "DeepSeek V4-Pro (A5)" table; it is best-effort — the summary job does not depend onmodel-tests-a5, so daily CI keeps producing and the table fills in once the runner exists.Also includes
cb150c6 fix(hc_pre): lift A5 guard for the core-count-agnostic separate impl(already on this branch).Prerequisite
A
npu-a5self-hosted runner must be registered — its.envprovidingCONDA_ROOT/CI_CACHE_ROOT/CANN_ROOT, the same provisioning contract as the 910B runners — beforemodel-tests-a5can run. Until then the job stays queued and the A5 report table shows a "no results" note; the rest of daily CI is unaffected.