fix(qwen3-14b): accept pypto-lib's renamed BATCH_PAD constant - #119
fix(qwen3-14b): accept pypto-lib's renamed BATCH_PAD constant#119lyfne123 wants to merge 1 commit into
Conversation
pypto-lib split the conflated `BATCH` constant when decode's public batch
became dynamic: `BATCH_PAD` is the padded pipeline width (the M of every
matmul), while the public batch is now a runtime value read from the
`seq_lens` descriptor. Against a post-rename pypto-lib the executor dies
during validation with
AttributeError: module '_pypto_lib_qwen3_14b_decode_fwd'
has no attribute 'BATCH'
Resolve the constant through `_kernel_batch_pad`, which accepts either
spelling, so this file works against a pypto-lib from either side of the
rename and the two repos can be updated independently.
Decode's check becomes an upper bound *only* when the kernel advertises
`BATCH_PAD`. A pre-rename decode writes exactly `BATCH` rows whatever the
runtime batch, so relaxing it unconditionally would admit a smaller
max_batch_size and overrun the buffers sized from it; that case keeps the
exact-match check. greedy_sample_fwd was not converted and stays an exact
match either way -- it carries the new spelling but is still fixed-batch,
so the spelling alone must not be read as "takes a dynamic batch".
No behaviour change against the pypto-lib pinned here: decode still
receives batch-padded tensors. Exploiting the dynamic batch -- dropping the
row-0 replication padding and sizing block_table by the active batch -- is
left to a follow-up.
📝 WalkthroughWalkthroughThe Qwen NPU executor now detects kernel batch widths from either ChangesQwen kernel batch validation
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pypto_serving/model/qwen/npu_executor.py (1)
217-232: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the batch-boundary contract.
Please test
BATCH_PADpreference, legacyBATCHfallback,max_batch_size == padded_width,max_batch_size > padded_width, and legacy exact-match rejection. These cases protect the compatibility behavior and the distinction between dynamic decode and fixed-batch kernels.🤖 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 `@pypto_serving/model/qwen/npu_executor.py` around lines 217 - 232, Add regression tests covering the batch-validation logic around _kernel_batch_pad/qwen3_decode_fwd: verify BATCH_PAD takes precedence, legacy BATCH is used when padding is absent, dynamic kernels accept max_batch_size equal to padded width but reject larger values, and fixed-batch kernels reject non-exact max_batch_size values while accepting an exact match.
🤖 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 `@pypto_serving/model/qwen/npu_executor.py`:
- Around line 44-45: Update the _kernel_batch_pad docstring so its boolean
return value is documented as indicating whether BATCH_PAD was found, matching
the helper’s True/False behavior; retain the padded row count description.
---
Nitpick comments:
In `@pypto_serving/model/qwen/npu_executor.py`:
- Around line 217-232: Add regression tests covering the batch-validation logic
around _kernel_batch_pad/qwen3_decode_fwd: verify BATCH_PAD takes precedence,
legacy BATCH is used when padding is absent, dynamic kernels accept
max_batch_size equal to padded width but reject larger values, and fixed-batch
kernels reject non-exact max_batch_size values while accepting an exact match.
🪄 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 Plus
Run ID: fcffe7be-2d47-4bef-832f-52f954da2566
📒 Files selected for processing (1)
pypto_serving/model/qwen/npu_executor.py
| def _kernel_batch_pad(kernel_module: object) -> tuple[int, bool]: | ||
| """Return ``(padded row count, module predates the BATCH_PAD rename)``. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the documented boolean polarity.
The helper returns True when BATCH_PAD is present and False when it falls back to legacy BATCH, but the docstring says the boolean indicates that the module predates the rename. Update the contract to state that it reports whether BATCH_PAD was found.
Proposed fix
def _kernel_batch_pad(kernel_module: object) -> tuple[int, bool]:
- """Return ``(padded row count, module predates the BATCH_PAD rename)``.
+ """Return ``(padded row count, whether the module exposes BATCH_PAD)``.📝 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.
| def _kernel_batch_pad(kernel_module: object) -> tuple[int, bool]: | |
| """Return ``(padded row count, module predates the BATCH_PAD rename)``. | |
| def _kernel_batch_pad(kernel_module: object) -> tuple[int, bool]: | |
| """Return ``(padded row count, whether the module exposes BATCH_PAD)``. |
🤖 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 `@pypto_serving/model/qwen/npu_executor.py` around lines 44 - 45, Update the
_kernel_batch_pad docstring so its boolean return value is documented as
indicating whether BATCH_PAD was found, matching the helper’s True/False
behavior; retain the padded row count description.
1 participant
Summary
pypto-lib split the conflated
BATCHconstant when decode's public batch became dynamic (hw-native-sys/pypto-lib#841):BATCH_PADis the padded pipeline width (the M of every matmul), while the public batch is now a runtime value read from theseq_lensdescriptor. Against a post-rename pypto-lib the executor dies during validation with:_kernel_batch_pad, which accepts either spelling, so this file works against a pypto-lib from either side of the rename and the two repos can be updated independently. This has to land first: pypto-lib CI checks out pypto-serving atref: mainand swaps in the pypto-lib under test, so Add: dynamic public batch (1..16) for Qwen3-14B decode pypto-lib#841 cannot go green untilmainhere tolerates the new name.BATCH_PAD. A pre-rename decode writes exactlyBATCHrows whatever the runtime batch, so relaxing it unconditionally would admit a smallermax_batch_sizeand overrun the buffers sized from it; that case keeps the exact-match check.greedy_sample_fwdwas not converted and stays an exact match either way. It carries the new spelling but is still fixed-batch, so the spelling alone must not be read as "takes a dynamic batch" — the helper's docstring says so explicitly.No behaviour change against the pypto-lib pinned here: decode still receives batch-padded tensors. Exploiting the dynamic batch — dropping the row-0 replication padding in
_prepare_decode_inputsand sizingblock_tableby the active batch — is left to a follow-up.Verification
_kernel_batch_padresolved against the real kernel modules of both generations, each in its own process:BATCH-> 16, exact-match pathBATCH-> 16BATCH_PAD-> 16, bound pathBATCH_PAD-> 16tests/test_qwen3_accuracy.pygets past executor validation (theAttributeError/ValueErrorare gone) but cannot complete on my box: both local pypto environments are independently broken right now — one has a Python/native skew (DataType.FP8E8M0missing from a freshly rebuiltpypto_core.so), the other has pto-isa header drift inbuild_output/_deps(TStore.hpp: template parameter redefines default argument). Neither is related to this change; the remaining failure is in prefill kernel compilation. CI runs the accuracy test in its own pinned environment.