Optimize Qwen3 device sampling outputs#57
Conversation
📝 WalkthroughWalkthroughThis PR modifies the Qwen3-14B NPU runner to keep large decode/prefill output tensors (logits, next-hidden) resident on the worker device instead of transferring to host, gated by an environment variable and allow_device_greedy_sampling flag. Adds a caching layer for these buffers and a validating unit test. ChangesWorker-resident output tensors
Estimated code review effort: 3 (Moderate) | ~25 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 optimizes performance during greedy generation by keeping large output tensors (such as logits and next hidden states) resident on the NPU worker, thereby avoiding unnecessary host-device transfers. A test case has also been added to verify this behavior. The feedback identifies a potential device mismatch issue in the _result_logits helper, where returning an empty placeholder on the NPU device instead of the CPU could lead to downstream runtime errors. It is recommended to instantiate this placeholder directly on the CPU.
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_device_sampling_submission.py (1)
71-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only greps source text; doesn't exercise runtime behavior.
This test asserts literal substrings exist in the runner's source file rather than exercising
_use_worker_resident_outputs,_output_kernel_argcaching, or_result_logits's actual output for bothDeviceTensorand host-tensor inputs. It will pass even if the gating logic is broken (e.g., wrong boolean condition, env var check inverted) as long as the strings remain present, and will break on harmless refactors (renames, reformatting) that don't change behavior. Since the PR description cites this test as the primary regression coverage for this optimization, consider adding a behavioral unit test that mocks the worker (similar to theWorkerTensor/alloc_tensorpattern already used intests/test_batching.py) to verify:_use_worker_resident_outputsrespectsQWEN14B_DEVICE_OUTPUTS=0/1/unset,_output_kernel_argreturns a cached instance on repeat calls with the same key, and_result_logitsreturns the correct empty vs. sliced tensor for each input type.🤖 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 `@tests/test_device_sampling_submission.py` around lines 71 - 79, The current test only checks for source substrings and does not verify the actual device-output behavior. Replace or supplement test_device_greedy_keeps_large_outputs_worker_resident with a behavioral test that exercises _use_worker_resident_outputs, _output_kernel_arg, and _result_logits on npu_runner.py using a mocked worker/tensor setup like the existing WorkerTensor/alloc_tensor patterns in tests/test_batching.py. Cover QWEN14B_DEVICE_OUTPUTS unset/0/1, confirm _output_kernel_arg caches per key, and assert _result_logits returns the expected empty or sliced tensor for both DeviceTensor and host-tensor inputs.
🤖 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 `@tests/test_device_sampling_submission.py`:
- Around line 71-79: The current test only checks for source substrings and does
not verify the actual device-output behavior. Replace or supplement
test_device_greedy_keeps_large_outputs_worker_resident with a behavioral test
that exercises _use_worker_resident_outputs, _output_kernel_arg, and
_result_logits on npu_runner.py using a mocked worker/tensor setup like the
existing WorkerTensor/alloc_tensor patterns in tests/test_batching.py. Cover
QWEN14B_DEVICE_OUTPUTS unset/0/1, confirm _output_kernel_arg caches per key, and
assert _result_logits returns the expected empty or sliced tensor for both
DeviceTensor and host-tensor inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bb22df9-dda9-48c3-8aa4-fd82de3fc8a1
📒 Files selected for processing (2)
examples/model/qwen3_14b/runner/npu_runner.pytests/test_device_sampling_submission.py
6a3e55b to
83eec9b
Compare
|
Addressed the review feedback in 83eec9b:
Validation: |
9ade2f5 to
2982c5a
Compare
2982c5a to
8aed8a1
Compare
Summary
sampled_token_idsis already produced on devicenext_hiddenon the original compiled host/shared buffer path to minimize the behavioral difflogits/sampled_token_idsargumentsTest
Result:
Qwen3 serving guard was also run locally with
zm_pypto:Benchmark
Qwen3-14B, full 40-layer, model path
/data/models/Qwen3-14B, A2A3, prompt北京故宫是, 128 generated tokens, offlinenpu_generate.py --profile.Key raw timing: